Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: prevent duplicate native handler registrations#1085

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
sherwinski wants to merge3 commits intomain
base:main
Choose a base branch
Loading
fromperf/duplicate-native-handlers

Conversation

@sherwinski
Copy link
Contributor

@sherwinskisherwinski commentedOct 28, 2025
edited
Loading

Description

One Line Summary

Added flags to ensureaddEventListener() only registers each native handler once per event type, eliminating unnecessarycordova.exec calls.

Details

Motivation

I noticed that we may be creating too many native handlers, once per each invocation ofaddEventListener(). This could cause a potential memory leak in the consuming applications.

Scope

AnyaddEventListener function across theInAppMessages,Notifications,User, andPushSubscription namespaces.

Testing

Unit testing

One tested added that ensures thatcordova.exec is only called once, no matter how many event listeners are added.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out allREQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

Added flags to ensure addEventListener() only registers each nativehandler once per event type, eliminating unnecessary `cordova.exec` calls.This fix is applied to the InAppMessages, Notifications, User, and PushSubscription namespaces.
@fadi-georgefadi-georgeforce-pushed thefg/more-tests branch 2 times, most recently fromd608ec1 to9d144c1CompareOctober 28, 2025 18:23
@sherwinski
Copy link
ContributorAuthor

I'm not sure if this is a realistic concern but thought I'd raise it anyways for discussion.
I also looked into a way to remove a native handler when all event listeners were removed but couldn't figure out what API exists for that.

Base automatically changed fromfg/more-tests tomainOctober 28, 2025 19:33
@fadi-georgefadi-georgeforce-pushed themain branch 8 times, most recently fromf9ef46e to80ffffaCompareOctober 30, 2025 17:26
@fadi-georgefadi-george requested a review froma team as acode ownerOctober 30, 2025 18:20
@fadi-georgefadi-georgeforce-pushed themain branch 10 times, most recently from3c70db9 to0997dcfCompareOctober 30, 2025 22:32
@fadi-georgefadi-georgeforce-pushed themain branch 10 times, most recently frome9bf279 to234f334CompareNovember 3, 2025 23:54
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@jkasten2jkasten2Awaiting requested review from jkasten2

@nan-linan-liAwaiting requested review from nan-li

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sherwinski

[8]ページ先頭

©2009-2025 Movatter.jp