From c6a2419b15af0467ca7cd92d800d82394428b7d7 Mon Sep 17 00:00:00 2001 From: sherwinski Date: Tue, 28 Oct 2025 10:10:51 -0700 Subject: [PATCH 1/3] perf: prevent duplicate native handler registrations Added flags to ensure addEventListener() only registers each native handler once per event type, eliminating unnecessary `cordova.exec` calls. This fix is applied to the InAppMessages, Notifications, User, and PushSubscription namespaces. --- www/InAppMessagesNamespace.ts | 156 +++++++++++++++++++------------ www/NotificationsNamespace.ts | 90 +++++++++++------- www/PushSubscriptionNamespace.ts | 32 ++++--- www/UserNamespace.ts | 28 ++++-- 4 files changed, 187 insertions(+), 119 deletions(-) diff --git a/www/InAppMessagesNamespace.ts b/www/InAppMessagesNamespace.ts index aaf339e8..b5071f7b 100644 --- a/www/InAppMessagesNamespace.ts +++ b/www/InAppMessagesNamespace.ts @@ -26,6 +26,13 @@ export default class InAppMessages { event: InAppMessageDidDismissEvent, ) => void)[] = []; + // Track whether native handlers have been registered to avoid duplicate registrations + private _clickHandlerRegistered = false; + private _willDisplayHandlerRegistered = false; + private _didDisplayHandlerRegistered = false; + private _willDismissHandlerRegistered = false; + private _didDismissHandlerRegistered = false; + private _processFunctionList( array: ((event: T) => void)[], param: T, @@ -49,86 +56,111 @@ export default class InAppMessages { this._inAppMessageClickListeners.push( listener as (event: InAppMessageClickEvent) => void, ); - const inAppMessageClickListener = (json: InAppMessageClickEvent) => { - this._processFunctionList(this._inAppMessageClickListeners, json); - }; - window.cordova.exec( - inAppMessageClickListener, - noop, - 'OneSignalPush', - 'setInAppMessageClickHandler', - [], - ); + + // Only register the native handler once + if (!this._clickHandlerRegistered) { + this._clickHandlerRegistered = true; + const inAppMessageClickListener = (json: InAppMessageClickEvent) => { + this._processFunctionList(this._inAppMessageClickListeners, json); + }; + window.cordova.exec( + inAppMessageClickListener, + noop, + 'OneSignalPush', + 'setInAppMessageClickHandler', + [], + ); + } } else if (event === 'willDisplay') { this._willDisplayInAppMessageListeners.push( listener as (event: InAppMessageWillDisplayEvent) => void, ); - const willDisplayCallBackProcessor = ( - event: InAppMessageWillDisplayEvent, - ) => { - this._processFunctionList( - this._willDisplayInAppMessageListeners, - event, + + // Only register the native handler once + if (!this._willDisplayHandlerRegistered) { + this._willDisplayHandlerRegistered = true; + const willDisplayCallBackProcessor = ( + event: InAppMessageWillDisplayEvent, + ) => { + this._processFunctionList( + this._willDisplayInAppMessageListeners, + event, + ); + }; + window.cordova.exec( + willDisplayCallBackProcessor, + noop, + 'OneSignalPush', + 'setOnWillDisplayInAppMessageHandler', + [], ); - }; - window.cordova.exec( - willDisplayCallBackProcessor, - noop, - 'OneSignalPush', - 'setOnWillDisplayInAppMessageHandler', - [], - ); + } } else if (event === 'didDisplay') { this._didDisplayInAppMessageListeners.push( listener as (event: InAppMessageDidDisplayEvent) => void, ); - const didDisplayCallBackProcessor = ( - event: InAppMessageDidDisplayEvent, - ) => { - this._processFunctionList(this._didDisplayInAppMessageListeners, event); - }; - window.cordova.exec( - didDisplayCallBackProcessor, - noop, - 'OneSignalPush', - 'setOnDidDisplayInAppMessageHandler', - [], - ); + + // Only register the native handler once + if (!this._didDisplayHandlerRegistered) { + this._didDisplayHandlerRegistered = true; + const didDisplayCallBackProcessor = ( + event: InAppMessageDidDisplayEvent, + ) => { + this._processFunctionList(this._didDisplayInAppMessageListeners, event); + }; + window.cordova.exec( + didDisplayCallBackProcessor, + noop, + 'OneSignalPush', + 'setOnDidDisplayInAppMessageHandler', + [], + ); + } } else if (event === 'willDismiss') { this._willDismissInAppMessageListeners.push( listener as (event: InAppMessageWillDismissEvent) => void, ); - const willDismissInAppMessageProcessor = ( - event: InAppMessageWillDismissEvent, - ) => { - this._processFunctionList( - this._willDismissInAppMessageListeners, - event, + + // Only register the native handler once + if (!this._willDismissHandlerRegistered) { + this._willDismissHandlerRegistered = true; + const willDismissInAppMessageProcessor = ( + event: InAppMessageWillDismissEvent, + ) => { + this._processFunctionList( + this._willDismissInAppMessageListeners, + event, + ); + }; + window.cordova.exec( + willDismissInAppMessageProcessor, + noop, + 'OneSignalPush', + 'setOnWillDismissInAppMessageHandler', + [], ); - }; - window.cordova.exec( - willDismissInAppMessageProcessor, - noop, - 'OneSignalPush', - 'setOnWillDismissInAppMessageHandler', - [], - ); + } } else if (event === 'didDismiss') { this._didDismissInAppMessageListeners.push( listener as (event: InAppMessageDidDismissEvent) => void, ); - const didDismissInAppMessageCallBackProcessor = ( - event: InAppMessageDidDismissEvent, - ) => { - this._processFunctionList(this._didDismissInAppMessageListeners, event); - }; - window.cordova.exec( - didDismissInAppMessageCallBackProcessor, - noop, - 'OneSignalPush', - 'setOnDidDismissInAppMessageHandler', - [], - ); + + // Only register the native handler once + if (!this._didDismissHandlerRegistered) { + this._didDismissHandlerRegistered = true; + const didDismissInAppMessageCallBackProcessor = ( + event: InAppMessageDidDismissEvent, + ) => { + this._processFunctionList(this._didDismissInAppMessageListeners, event); + }; + window.cordova.exec( + didDismissInAppMessageCallBackProcessor, + noop, + 'OneSignalPush', + 'setOnDidDismissInAppMessageHandler', + [], + ); + } } } diff --git a/www/NotificationsNamespace.ts b/www/NotificationsNamespace.ts index 87e06703..342f331a 100644 --- a/www/NotificationsNamespace.ts +++ b/www/NotificationsNamespace.ts @@ -24,6 +24,11 @@ export default class Notifications { event: NotificationWillDisplayEvent, ) => void)[] = []; + // Track whether native handlers have been registered to avoid duplicate registrations + private _clickHandlerRegistered = false; + private _foregroundWillDisplayHandlerRegistered = false; + private _permissionChangeHandlerRegistered = false; + private _processFunctionList( array: ((event: T) => void)[], param: T, @@ -177,51 +182,66 @@ export default class Notifications { this._notificationClickedListeners.push( listener as (event: NotificationClickEvent) => void, ); - const clickParsingHandler = (json: NotificationClickEvent) => { - this._processFunctionList(this._notificationClickedListeners, json); - }; - window.cordova.exec( - clickParsingHandler, - noop, - 'OneSignalPush', - 'addNotificationClickListener', - [], - ); + + // Only register the native handler once + if (!this._clickHandlerRegistered) { + this._clickHandlerRegistered = true; + const clickParsingHandler = (json: NotificationClickEvent) => { + this._processFunctionList(this._notificationClickedListeners, json); + }; + window.cordova.exec( + clickParsingHandler, + noop, + 'OneSignalPush', + 'addNotificationClickListener', + [], + ); + } } else if (event === 'foregroundWillDisplay') { this._notificationWillDisplayListeners.push( listener as (event: NotificationWillDisplayEvent) => void, ); - const foregroundParsingHandler = (notification: OSNotification) => { - this._notificationWillDisplayListeners.forEach((listener) => { - listener(new NotificationWillDisplayEvent(notification)); - }); + + // Only register the native handler once + if (!this._foregroundWillDisplayHandlerRegistered) { + this._foregroundWillDisplayHandlerRegistered = true; + const foregroundParsingHandler = (notification: OSNotification) => { + this._notificationWillDisplayListeners.forEach((listener) => { + listener(new NotificationWillDisplayEvent(notification)); + }); + window.cordova.exec( + noop, + noop, + 'OneSignalPush', + 'proceedWithWillDisplay', + [notification.notificationId], + ); + }; window.cordova.exec( - noop, + foregroundParsingHandler, noop, 'OneSignalPush', - 'proceedWithWillDisplay', - [notification.notificationId], + 'addForegroundLifecycleListener', + [], ); - }; - window.cordova.exec( - foregroundParsingHandler, - noop, - 'OneSignalPush', - 'addForegroundLifecycleListener', - [], - ); + } } else if (event === 'permissionChange') { this._permissionObserverList.push(listener as (event: boolean) => void); - const permissionCallBackProcessor = (state: boolean) => { - this._processFunctionList(this._permissionObserverList, state); - }; - window.cordova.exec( - permissionCallBackProcessor, - noop, - 'OneSignalPush', - 'addPermissionObserver', - [], - ); + + // Only register the native handler once + if (!this._permissionChangeHandlerRegistered) { + this._permissionChangeHandlerRegistered = true; + const permissionCallBackProcessor = (state: boolean) => { + this._processFunctionList(this._permissionObserverList, state); + }; + window.cordova.exec( + permissionCallBackProcessor, + noop, + 'OneSignalPush', + 'addPermissionObserver', + [], + ); + } } } diff --git a/www/PushSubscriptionNamespace.ts b/www/PushSubscriptionNamespace.ts index 0f9a8cfd..4726b087 100644 --- a/www/PushSubscriptionNamespace.ts +++ b/www/PushSubscriptionNamespace.ts @@ -21,6 +21,9 @@ export default class PushSubscription { event: PushSubscriptionChangedState, ) => void)[] = []; + // Track whether native handler has been registered to avoid duplicate registrations + private _changeHandlerRegistered = false; + private _processFunctionList( array: ((event: PushSubscriptionChangedState) => void)[], param: PushSubscriptionChangedState, @@ -175,18 +178,23 @@ export default class PushSubscription { this._subscriptionObserverList.push( listener as (event: PushSubscriptionChangedState) => void, ); - const subscriptionCallBackProcessor = ( - state: PushSubscriptionChangedState, - ) => { - this._processFunctionList(this._subscriptionObserverList, state); - }; - window.cordova.exec( - subscriptionCallBackProcessor, - noop, - 'OneSignalPush', - 'addPushSubscriptionObserver', - [], - ); + + // Only register the native handler once + if (!this._changeHandlerRegistered) { + this._changeHandlerRegistered = true; + const subscriptionCallBackProcessor = ( + state: PushSubscriptionChangedState, + ) => { + this._processFunctionList(this._subscriptionObserverList, state); + }; + window.cordova.exec( + subscriptionCallBackProcessor, + noop, + 'OneSignalPush', + 'addPushSubscriptionObserver', + [], + ); + } } /** diff --git a/www/UserNamespace.ts b/www/UserNamespace.ts index 7ae2edd5..ea45ec81 100644 --- a/www/UserNamespace.ts +++ b/www/UserNamespace.ts @@ -17,6 +17,9 @@ export default class User { private _userStateObserverList: ((event: UserChangedState) => void)[] = []; + // Track whether native handler has been registered to avoid duplicate registrations + private _changeHandlerRegistered = false; + private _processFunctionList( array: ((event: UserChangedState) => void)[], param: UserChangedState, @@ -196,16 +199,21 @@ export default class User { this._userStateObserverList.push( listener as (event: UserChangedState) => void, ); - const userCallBackProcessor = (state: UserChangedState) => { - this._processFunctionList(this._userStateObserverList, state); - }; - window.cordova.exec( - userCallBackProcessor, - noop, - 'OneSignalPush', - 'addUserStateObserver', - [], - ); + + // Only register the native handler once + if (!this._changeHandlerRegistered) { + this._changeHandlerRegistered = true; + const userCallBackProcessor = (state: UserChangedState) => { + this._processFunctionList(this._userStateObserverList, state); + }; + window.cordova.exec( + userCallBackProcessor, + noop, + 'OneSignalPush', + 'addUserStateObserver', + [], + ); + } } /** From 5e6257714de97335c42b4f950b03232b1ffba8ee Mon Sep 17 00:00:00 2001 From: sherwinski Date: Tue, 28 Oct 2025 10:10:57 -0700 Subject: [PATCH 2/3] test: add tests for native handler registration --- www/InAppMessagesNamespace.test.ts | 73 +++++++++++++++++++++++++++ www/NotificationsNamespace.test.ts | 56 +++++++++++++++++++- www/PushSubscriptionNamespace.test.ts | 29 +++++++++++ www/UserNamespace.test.ts | 29 +++++++++++ 4 files changed, 185 insertions(+), 2 deletions(-) diff --git a/www/InAppMessagesNamespace.test.ts b/www/InAppMessagesNamespace.test.ts index 6713d325..7eff847d 100644 --- a/www/InAppMessagesNamespace.test.ts +++ b/www/InAppMessagesNamespace.test.ts @@ -89,6 +89,79 @@ describe('InAppMessages', () => { expect(window.cordova.exec).not.toHaveBeenCalled(); }); + + test('should only register native handler once for multiple listeners of same event type', () => { + const handler1 = vi.fn(); + const handler2 = vi.fn(); + const handler3 = vi.fn(); + + // Add first listener - should register native handler + inAppMessages.addEventListener('click', handler1); + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + + // Add second listener - should NOT register native handler again + inAppMessages.addEventListener('click', handler2); + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + + // Add third listener - should still be only one registration + inAppMessages.addEventListener('click', handler3); + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + + // Trigger the event with click data + const clickData = { + message: { messageId: 'test' }, + result: { closingMessage: true, actionId: 'test' }, + }; + mockExec.mock.calls[0][0](clickData); + + // All handlers should execute exactly once + expect(handler1).toHaveBeenCalledTimes(1); + expect(handler1).toHaveBeenCalledWith(clickData); + expect(handler2).toHaveBeenCalledTimes(1); + expect(handler2).toHaveBeenCalledWith(clickData); + expect(handler3).toHaveBeenCalledTimes(1); + expect(handler3).toHaveBeenCalledWith(clickData); + }); + + test('should only register native handler once for willDisplay event', () => { + const handler1 = vi.fn(); + const handler2 = vi.fn(); + + inAppMessages.addEventListener('willDisplay', handler1); + inAppMessages.addEventListener('willDisplay', handler2); + + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + }); + + test('should only register native handler once for didDisplay event', () => { + const handler1 = vi.fn(); + const handler2 = vi.fn(); + + inAppMessages.addEventListener('didDisplay', handler1); + inAppMessages.addEventListener('didDisplay', handler2); + + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + }); + + test('should only register native handler once for willDismiss event', () => { + const handler1 = vi.fn(); + const handler2 = vi.fn(); + + inAppMessages.addEventListener('willDismiss', handler1); + inAppMessages.addEventListener('willDismiss', handler2); + + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + }); + + test('should only register native handler once for didDismiss event', () => { + const handler1 = vi.fn(); + const handler2 = vi.fn(); + + inAppMessages.addEventListener('didDismiss', handler1); + inAppMessages.addEventListener('didDismiss', handler2); + + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + }); }); describe('removeEventListener', () => { diff --git a/www/NotificationsNamespace.test.ts b/www/NotificationsNamespace.test.ts index 0a7b5a14..7cef8dae 100644 --- a/www/NotificationsNamespace.test.ts +++ b/www/NotificationsNamespace.test.ts @@ -243,13 +243,15 @@ describe('Notifications', () => { [], ); - mockExec.mockClear(); + // Capture the callback before adding second listener + const foregroundCallback = mockExec.mock.calls[0][0]; + const mockListener2 = vi.fn(); notifications.addEventListener('foregroundWillDisplay', mockListener2); // can call display event listeners const notificationData = mockNotification(); - mockExec.mock.calls[0][0](notificationData); + foregroundCallback(notificationData); const displayEvent = new NotificationWillDisplayEvent(notificationData); expect(mockListener).toHaveBeenCalledWith(displayEvent); @@ -292,6 +294,56 @@ describe('Notifications', () => { expect(window.cordova.exec).not.toHaveBeenCalled(); }); + + test('should only register native handler once for multiple click listeners', () => { + const handler1 = vi.fn(); + const handler2 = vi.fn(); + const handler3 = vi.fn(); + + // Add first listener - should register native handler + notifications.addEventListener('click', handler1); + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + + // Add second listener - should NOT register native handler again + notifications.addEventListener('click', handler2); + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + + // Add third listener - should still be only one registration + notifications.addEventListener('click', handler3); + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + + // Trigger the event + const clickData = mockNotificationClickEvent(); + mockExec.mock.calls[0][0](clickData); + + // All handlers should execute exactly once + expect(handler1).toHaveBeenCalledTimes(1); + expect(handler1).toHaveBeenCalledWith(clickData); + expect(handler2).toHaveBeenCalledTimes(1); + expect(handler2).toHaveBeenCalledWith(clickData); + expect(handler3).toHaveBeenCalledTimes(1); + expect(handler3).toHaveBeenCalledWith(clickData); + }); + + test('should only register native handler once for multiple foregroundWillDisplay listeners', () => { + const handler1 = vi.fn(); + const handler2 = vi.fn(); + + notifications.addEventListener('foregroundWillDisplay', handler1); + notifications.addEventListener('foregroundWillDisplay', handler2); + + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + }); + + test('should only register native handler once for multiple permissionChange listeners', () => { + const handler1 = vi.fn(); + const handler2 = vi.fn(); + + notifications.addEventListener('permissionChange', handler1); + notifications.addEventListener('permissionChange', handler2); + + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + }); }); describe('removeEventListener', () => { diff --git a/www/PushSubscriptionNamespace.test.ts b/www/PushSubscriptionNamespace.test.ts index bc328cc6..9fe5171f 100644 --- a/www/PushSubscriptionNamespace.test.ts +++ b/www/PushSubscriptionNamespace.test.ts @@ -245,6 +245,35 @@ describe('PushSubscription', () => { expect(mockListener).toHaveBeenCalledWith(SUB_CHANGED_STATE); expect(mockListener2).toHaveBeenCalledWith(SUB_CHANGED_STATE); }); + + test('should only register native handler once for multiple listeners', () => { + const handler1 = vi.fn(); + const handler2 = vi.fn(); + const handler3 = vi.fn(); + + // Add first listener - should register native handler + pushSubscription.addEventListener('change', handler1); + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + + // Add second listener - should NOT register native handler again + pushSubscription.addEventListener('change', handler2); + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + + // Add third listener - should still be only one registration + pushSubscription.addEventListener('change', handler3); + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + + // Trigger the event + mockExec.mock.calls[0][0](SUB_CHANGED_STATE); + + // All handlers should execute exactly once + expect(handler1).toHaveBeenCalledTimes(1); + expect(handler1).toHaveBeenCalledWith(SUB_CHANGED_STATE); + expect(handler2).toHaveBeenCalledTimes(1); + expect(handler2).toHaveBeenCalledWith(SUB_CHANGED_STATE); + expect(handler3).toHaveBeenCalledTimes(1); + expect(handler3).toHaveBeenCalledWith(SUB_CHANGED_STATE); + }); }); describe('removeEventListener', () => { diff --git a/www/UserNamespace.test.ts b/www/UserNamespace.test.ts index 81bddae3..f1d17779 100644 --- a/www/UserNamespace.test.ts +++ b/www/UserNamespace.test.ts @@ -349,6 +349,35 @@ describe('User', () => { expect(mockListener2).toHaveBeenCalledWith(USER_CHANGED_STATE); expect(mockListener3).toHaveBeenCalledWith(USER_CHANGED_STATE); }); + + test('should only register native handler once for multiple listeners', () => { + const handler1 = vi.fn(); + const handler2 = vi.fn(); + const handler3 = vi.fn(); + + // Add first listener - should register native handler + user.addEventListener('change', handler1); + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + + // Add second listener - should NOT register native handler again + user.addEventListener('change', handler2); + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + + // Add third listener - should still be only one registration + user.addEventListener('change', handler3); + expect(window.cordova.exec).toHaveBeenCalledTimes(1); + + // Trigger the event + mockExec.mock.calls[0][0](USER_CHANGED_STATE); + + // All handlers should execute exactly once + expect(handler1).toHaveBeenCalledTimes(1); + expect(handler1).toHaveBeenCalledWith(USER_CHANGED_STATE); + expect(handler2).toHaveBeenCalledTimes(1); + expect(handler2).toHaveBeenCalledWith(USER_CHANGED_STATE); + expect(handler3).toHaveBeenCalledTimes(1); + expect(handler3).toHaveBeenCalledWith(USER_CHANGED_STATE); + }); }); describe('removeEventListener', () => { From 7c3b55a03f71be5df573ff8a0de16ec8d364a025 Mon Sep 17 00:00:00 2001 From: sherwinski Date: Tue, 28 Oct 2025 11:15:03 -0700 Subject: [PATCH 3/3] format: sync with prettier --- www/InAppMessagesNamespace.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/www/InAppMessagesNamespace.ts b/www/InAppMessagesNamespace.ts index b5071f7b..b89cd772 100644 --- a/www/InAppMessagesNamespace.ts +++ b/www/InAppMessagesNamespace.ts @@ -106,7 +106,10 @@ export default class InAppMessages { const didDisplayCallBackProcessor = ( event: InAppMessageDidDisplayEvent, ) => { - this._processFunctionList(this._didDisplayInAppMessageListeners, event); + this._processFunctionList( + this._didDisplayInAppMessageListeners, + event, + ); }; window.cordova.exec( didDisplayCallBackProcessor, @@ -151,7 +154,10 @@ export default class InAppMessages { const didDismissInAppMessageCallBackProcessor = ( event: InAppMessageDidDismissEvent, ) => { - this._processFunctionList(this._didDismissInAppMessageListeners, event); + this._processFunctionList( + this._didDismissInAppMessageListeners, + event, + ); }; window.cordova.exec( didDismissInAppMessageCallBackProcessor,