From 64f3a1803f25cd843c76d77156d9d4b6d7be21c7 Mon Sep 17 00:00:00 2001 From: Oskar Eichler <62393985+OskarEichler@users.noreply.github.com> Date: Fri, 28 Aug 2026 02:45:32 +0300 Subject: [PATCH 1/3] fix: stabilize observer lifecycle and asynchronous state snapshots --- src/events/EventManager.ts | 6 +++-- src/index.ts | 49 +++++++++++++++++++++++++++----------- 2 files changed, 39 insertions(+), 16 deletions(-) diff --git a/src/events/EventManager.ts b/src/events/EventManager.ts index e2cfc9f9..f93d32e0 100644 --- a/src/events/EventManager.ts +++ b/src/events/EventManager.ts @@ -61,7 +61,7 @@ export default class EventManager { } setupListeners() { - if (this.RNOneSignal == null) return; + if (this.RNOneSignal == null || this.nativeSubscriptions.length > 0) return; this.nativeSubscriptions.push( this.RNOneSignal.onPermissionChanged((payload) => { @@ -156,9 +156,11 @@ export default class EventManager { private dispatchHandlers(eventName: string, payload: unknown) { const handlerArray = this.eventListenerArrayMap.get(eventName); if (handlerArray) { - handlerArray.forEach((handler) => { + handlerArray.slice().forEach((handler) => { handler(payload); }); + } else if (eventName === NOTIFICATION_WILL_DISPLAY) { + (payload as NotificationWillDisplayEvent).getNotification().display(); } } } diff --git a/src/index.ts b/src/index.ts index 1b4770a7..52258627 100644 --- a/src/index.ts +++ b/src/index.ts @@ -39,8 +39,8 @@ import type { UserChangedState, UserState } from './types/user'; const RNOneSignal = NativeOneSignal; const GLOBAL_KEY = '__oneSignalEventManager'; -const prev = (globalThis as Record)[GLOBAL_KEY]; -if (prev instanceof EventManager) { +const prev = (globalThis as Record)[GLOBAL_KEY] as EventManager | undefined; +if (typeof prev?.clearListeners === 'function') { prev.clearListeners(); } const eventManager = new EventManager(RNOneSignal); @@ -58,6 +58,10 @@ export enum LogLevel { } let notificationPermission = false; +let permissionObserverAdded = false; +let subscriptionObserverAdded = false; +let permissionVersion = 0; +let subscriptionVersion = 0; let pushSub: PushSubscriptionState = { id: '', @@ -66,21 +70,37 @@ let pushSub: PushSubscriptionState = { }; async function _addPermissionObserver() { - OneSignal.Notifications.addEventListener('permissionChange', (granted: boolean) => { - notificationPermission = granted; - }); + const version = ++permissionVersion; + if (!permissionObserverAdded) { + OneSignal.Notifications.addEventListener('permissionChange', (granted: boolean) => { + permissionVersion++; + notificationPermission = granted; + }); + permissionObserverAdded = true; + } - notificationPermission = await RNOneSignal.hasNotificationPermission(); + const permission = await RNOneSignal.hasNotificationPermission(); + if (version === permissionVersion) notificationPermission = permission; } async function _addPushSubscriptionObserver() { - OneSignal.User.pushSubscription.addEventListener('change', (subscriptionChange) => { - pushSub = subscriptionChange.current; - }); + const version = ++subscriptionVersion; + if (!subscriptionObserverAdded) { + OneSignal.User.pushSubscription.addEventListener('change', (subscriptionChange) => { + subscriptionVersion++; + pushSub = subscriptionChange.current; + }); + subscriptionObserverAdded = true; + } - pushSub.id = (await RNOneSignal.getPushSubscriptionId()) ?? undefined; - pushSub.token = (await RNOneSignal.getPushSubscriptionToken()) ?? undefined; - pushSub.optedIn = await RNOneSignal.getOptedIn(); + const [id, token, optedIn] = await Promise.all([ + RNOneSignal.getPushSubscriptionId(), + RNOneSignal.getPushSubscriptionToken(), + RNOneSignal.getOptedIn(), + ]); + if (version === subscriptionVersion) { + pushSub = { id: id ?? undefined, token: token ?? undefined, optedIn }; + } } export namespace OneSignal { @@ -90,8 +110,9 @@ export namespace OneSignal { RNOneSignal.initialize(appId); - void _addPermissionObserver(); - void _addPushSubscriptionObserver(); + void Promise.all([_addPermissionObserver(), _addPushSubscriptionObserver()]).catch((error) => { + console.warn('OneSignal: failed to read initial state', error); + }); } /** From b71ced8d7ed30812d829c38462b71e749bb72c53 Mon Sep 17 00:00:00 2001 From: Fadi George Date: Tue, 1 Sep 2026 15:43:11 -0700 Subject: [PATCH 2/3] fix: adopt the realm's existing event manager instead of replacing it Replacing the manager stored on the global tears down the native subscriptions of whichever module instance created it, so listeners registered through that instance stop receiving events for the lifetime of the process. Reuse that manager instead, which also keeps a duplicate install from double-subscribing to the native emitters. Drops the default-display fallback from dispatchHandlers: foreground events no longer route through it, and the native callback already displays by default in its finally block. Adds regression tests for the manager lookup, the startup read races, listener mutation during dispatch, setupListeners idempotence, and clearListeners. Co-authored-by: Cursor --- src/events/EventManager.test.ts | 46 ++++++++++++++++ src/events/EventManager.ts | 2 - src/index.test.ts | 96 +++++++++++++++++++++++++++++++++ src/index.ts | 22 ++++++-- 4 files changed, 159 insertions(+), 7 deletions(-) diff --git a/src/events/EventManager.test.ts b/src/events/EventManager.test.ts index f8e7af2d..30a54443 100644 --- a/src/events/EventManager.test.ts +++ b/src/events/EventManager.test.ts @@ -95,6 +95,29 @@ describe('EventManager', () => { new EventManager(null as never); expect(freshModule.onPermissionChanged).not.toHaveBeenCalled(); }); + + test('should not subscribe again when called on an already subscribed manager', () => { + eventManager.setupListeners(); + + expect(mockModule.onPermissionChanged).toHaveBeenCalledOnce(); + expect(mockModule.onNotificationWillDisplay).toHaveBeenCalledOnce(); + expect(eventManager['nativeSubscriptions'].length).toBe(10); + }); + }); + + describe('clearListeners', () => { + test('should remove native subscriptions and drop handlers', () => { + const subscriptions = eventManager['nativeSubscriptions'].slice(); + eventManager.addEventListener(PERMISSION_CHANGED, vi.fn()); + + eventManager.clearListeners(); + + subscriptions.forEach((sub) => { + expect(sub.remove).toHaveBeenCalledOnce(); + }); + expect(eventManager['nativeSubscriptions'].length).toBe(0); + expect(eventManager['eventListenerArrayMap'].size).toBe(0); + }); }); describe('addEventListener', () => { @@ -513,6 +536,29 @@ describe('EventManager', () => { expect(handler2).not.toHaveBeenCalled(); expect(handler3).toHaveBeenCalledWith(pushChangedPayload); }); + + test('should still reach later handlers when one removes itself mid-dispatch', () => { + const selfRemoving = vi.fn(() => { + eventManager.removeEventListener(SUBSCRIPTION_CHANGED, selfRemoving); + }); + const next = vi.fn(); + + eventManager.addEventListener(SUBSCRIPTION_CHANGED, selfRemoving); + eventManager.addEventListener(SUBSCRIPTION_CHANGED, next); + + const emitCallback = callbacks.get('onSubscriptionChanged')!; + emitCallback(pushChangedPayload); + + expect(selfRemoving).toHaveBeenCalledOnce(); + expect(next).toHaveBeenCalledWith(pushChangedPayload); + + selfRemoving.mockClear(); + next.mockClear(); + emitCallback(pushChangedPayload); + + expect(selfRemoving).not.toHaveBeenCalled(); + expect(next).toHaveBeenCalledOnce(); + }); }); }); diff --git a/src/events/EventManager.ts b/src/events/EventManager.ts index f93d32e0..9cd81680 100644 --- a/src/events/EventManager.ts +++ b/src/events/EventManager.ts @@ -159,8 +159,6 @@ export default class EventManager { handlerArray.slice().forEach((handler) => { handler(payload); }); - } else if (eventName === NOTIFICATION_WILL_DISPLAY) { - (payload as NotificationWillDisplayEvent).getNotification().display(); } } } diff --git a/src/index.test.ts b/src/index.test.ts index ef6a3a88..77777c16 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -41,6 +41,17 @@ const filterEventListener = ( )[0][1] as EventListenerMap[K]; }; +const GLOBAL_KEY = '__oneSignalEventManager'; + +// The SDK's own observer is the first handler registered for these events, so it survives +// the mock resets that clear the spy call history. +const internalObserver = (eventName: K): EventListenerMap[K] => { + const manager = (globalThis as Record)[GLOBAL_KEY] as EventManager; + return manager['eventListenerArrayMap'].get(eventName)![0] as EventListenerMap[K]; +}; + +const flushPromises = () => new Promise((resolve) => setTimeout(resolve, 0)); + describe('OneSignal', () => { beforeEach(() => { mockPlatform.OS = 'ios'; @@ -104,6 +115,91 @@ describe('OneSignal', () => { OneSignal.initialize(APP_ID); expect(mockRNOneSignal.initialize).not.toHaveBeenCalled(); }); + + test('should keep a permission event that arrives before the startup read resolves', async () => { + let resolveStartupRead: ((granted: boolean) => void) | undefined; + vi.mocked(mockRNOneSignal.hasNotificationPermission).mockReturnValueOnce( + new Promise((resolve) => { + resolveStartupRead = resolve; + }), + ); + + OneSignal.initialize(APP_ID); + internalObserver(PERMISSION_CHANGED)(true); + resolveStartupRead!(false); + await flushPromises(); + + expect(OneSignal.Notifications.hasPermission()).toBe(true); + + internalObserver(PERMISSION_CHANGED)(false); + }); + + test('should keep a subscription event that arrives before the startup reads resolve', async () => { + let resolveStartupRead: ((id: string) => void) | undefined; + vi.mocked(mockRNOneSignal.getPushSubscriptionId).mockReturnValueOnce( + new Promise((resolve) => { + resolveStartupRead = resolve; + }), + ); + + OneSignal.initialize(APP_ID); + internalObserver(SUBSCRIPTION_CHANGED)({ + previous: { id: '', token: '', optedIn: false }, + current: { id: PUSH_ID, token: PUSH_TOKEN, optedIn: true }, + }); + resolveStartupRead!('stale-id'); + await flushPromises(); + + expect(OneSignal.User.pushSubscription.getPushSubscriptionId()).toBe(PUSH_ID); + + internalObserver(SUBSCRIPTION_CHANGED)({ + previous: { id: PUSH_ID, token: PUSH_TOKEN, optedIn: true }, + current: { id: '', token: '', optedIn: false }, + }); + }); + + test('should warn instead of rejecting when a startup read fails', async () => { + vi.mocked(mockRNOneSignal.hasNotificationPermission).mockRejectedValueOnce( + new Error('native failure'), + ); + + OneSignal.initialize(APP_ID); + await flushPromises(); + + expect(console.warn).toHaveBeenCalledWith( + 'OneSignal: failed to read initial state', + expect.any(Error), + ); + }); + }); + + describe('event manager resolution', () => { + test('should adopt the manager another module instance left on the global', async () => { + const globals = globalThis as Record; + const ownManager = globals[GLOBAL_KEY]; + const foreignManager = { + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + clearListeners: vi.fn(), + }; + globals[GLOBAL_KEY] = foreignManager; + + try { + vi.resetModules(); + const duplicateCopy = await import('./index'); + const listener = vi.fn(); + duplicateCopy.OneSignal.Notifications.addEventListener('click', listener); + + expect(foreignManager.clearListeners).not.toHaveBeenCalled(); + expect(foreignManager.addEventListener).toHaveBeenCalledWith( + NOTIFICATION_CLICKED, + listener, + ); + expect(globals[GLOBAL_KEY]).toBe(foreignManager); + } finally { + globals[GLOBAL_KEY] = ownManager; + } + }); }); describe('login', () => { diff --git a/src/index.ts b/src/index.ts index 52258627..f2e2a058 100644 --- a/src/index.ts +++ b/src/index.ts @@ -39,12 +39,24 @@ import type { UserChangedState, UserState } from './types/user'; const RNOneSignal = NativeOneSignal; const GLOBAL_KEY = '__oneSignalEventManager'; -const prev = (globalThis as Record)[GLOBAL_KEY] as EventManager | undefined; -if (typeof prev?.clearListeners === 'function') { - prev.clearListeners(); + +// A reloaded module or a duplicate install gets its own class object, so `instanceof` +// cannot recognize the manager already holding this realm's native subscriptions. Adopt +// that manager rather than replacing it: replacing it strands the listeners the other +// module instance registered on a manager that no longer receives native events. +function resolveEventManager(): EventManager { + const globals = globalThis as Record; + const existing = globals[GLOBAL_KEY] as EventManager | undefined; + if (typeof existing?.addEventListener === 'function') { + return existing; + } + + const created = new EventManager(RNOneSignal); + globals[GLOBAL_KEY] = created; + return created; } -const eventManager = new EventManager(RNOneSignal); -(globalThis as Record)[GLOBAL_KEY] = eventManager; + +const eventManager = resolveEventManager(); /// An enum that declares different types of log levels you can use with the OneSignal SDK, going from the least verbose (none) to verbose (print all comments). export enum LogLevel { From 7176375eae847ed2e28ecf41e7b7875092752c80 Mon Sep 17 00:00:00 2001 From: Fadi George Date: Tue, 1 Sep 2026 16:12:47 -0700 Subject: [PATCH 3/3] fix: restore scoped event manager reload cleanup Co-authored-by: Cursor --- src/index.test.ts | 67 ++++++++++++----------------------------------- src/index.ts | 22 ++++------------ 2 files changed, 22 insertions(+), 67 deletions(-) diff --git a/src/index.test.ts b/src/index.test.ts index 77777c16..99bc865f 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -33,21 +33,19 @@ const isValidCallbackSpy = vi.spyOn(helpers, 'isValidCallback'); const addEventManagerListenerSpy = vi.spyOn(EventManager.prototype, 'addEventListener'); const removeEventManagerListenerSpy = vi.spyOn(EventManager.prototype, 'removeEventListener'); -const filterEventListener = ( - eventName: K, -): EventListenerMap[K] => { - return addEventManagerListenerSpy.mock.calls.filter( - (call: [string, unknown]) => call[0] === eventName, - )[0][1] as EventListenerMap[K]; -}; - const GLOBAL_KEY = '__oneSignalEventManager'; -// The SDK's own observer is the first handler registered for these events, so it survives -// the mock resets that clear the spy call history. -const internalObserver = (eventName: K): EventListenerMap[K] => { +const dispatchEvent = ( + eventName: K, + payload: Parameters[0], +) => { const manager = (globalThis as Record)[GLOBAL_KEY] as EventManager; - return manager['eventListenerArrayMap'].get(eventName)![0] as EventListenerMap[K]; + manager['eventListenerArrayMap'] + .get(eventName) + ?.slice() + .forEach((handler) => { + handler(payload); + }); }; const flushPromises = () => new Promise((resolve) => setTimeout(resolve, 0)); @@ -80,8 +78,7 @@ describe('OneSignal', () => { expect(mockRNOneSignal.initialize).toHaveBeenCalledWith(APP_ID); // test permission change listener - const changeFn = filterEventListener(PERMISSION_CHANGED); - changeFn(true); + dispatchEvent(PERMISSION_CHANGED, true); const permission = OneSignal.Notifications.hasPermission(); expect(permission).toBe(true); @@ -98,13 +95,12 @@ describe('OneSignal', () => { optedIn: true, }, }; - const subscriptionChangeFn = filterEventListener(SUBSCRIPTION_CHANGED); - subscriptionChangeFn(pushData); + dispatchEvent(SUBSCRIPTION_CHANGED, pushData); const pushSubscription = OneSignal.User.pushSubscription.getPushSubscriptionId(); expect(pushSubscription).toBe('subscription-id'); // reset push subscription - subscriptionChangeFn({ + dispatchEvent(SUBSCRIPTION_CHANGED, { ...pushData, current: { id: '', token: '', optedIn: false }, }); @@ -125,13 +121,13 @@ describe('OneSignal', () => { ); OneSignal.initialize(APP_ID); - internalObserver(PERMISSION_CHANGED)(true); + dispatchEvent(PERMISSION_CHANGED, true); resolveStartupRead!(false); await flushPromises(); expect(OneSignal.Notifications.hasPermission()).toBe(true); - internalObserver(PERMISSION_CHANGED)(false); + dispatchEvent(PERMISSION_CHANGED, false); }); test('should keep a subscription event that arrives before the startup reads resolve', async () => { @@ -143,7 +139,7 @@ describe('OneSignal', () => { ); OneSignal.initialize(APP_ID); - internalObserver(SUBSCRIPTION_CHANGED)({ + dispatchEvent(SUBSCRIPTION_CHANGED, { previous: { id: '', token: '', optedIn: false }, current: { id: PUSH_ID, token: PUSH_TOKEN, optedIn: true }, }); @@ -152,7 +148,7 @@ describe('OneSignal', () => { expect(OneSignal.User.pushSubscription.getPushSubscriptionId()).toBe(PUSH_ID); - internalObserver(SUBSCRIPTION_CHANGED)({ + dispatchEvent(SUBSCRIPTION_CHANGED, { previous: { id: PUSH_ID, token: PUSH_TOKEN, optedIn: true }, current: { id: '', token: '', optedIn: false }, }); @@ -173,35 +169,6 @@ describe('OneSignal', () => { }); }); - describe('event manager resolution', () => { - test('should adopt the manager another module instance left on the global', async () => { - const globals = globalThis as Record; - const ownManager = globals[GLOBAL_KEY]; - const foreignManager = { - addEventListener: vi.fn(), - removeEventListener: vi.fn(), - clearListeners: vi.fn(), - }; - globals[GLOBAL_KEY] = foreignManager; - - try { - vi.resetModules(); - const duplicateCopy = await import('./index'); - const listener = vi.fn(); - duplicateCopy.OneSignal.Notifications.addEventListener('click', listener); - - expect(foreignManager.clearListeners).not.toHaveBeenCalled(); - expect(foreignManager.addEventListener).toHaveBeenCalledWith( - NOTIFICATION_CLICKED, - listener, - ); - expect(globals[GLOBAL_KEY]).toBe(foreignManager); - } finally { - globals[GLOBAL_KEY] = ownManager; - } - }); - }); - describe('login', () => { test('should login with externalId', () => { OneSignal.login('external-123'); diff --git a/src/index.ts b/src/index.ts index f2e2a058..212299d1 100644 --- a/src/index.ts +++ b/src/index.ts @@ -39,24 +39,12 @@ import type { UserChangedState, UserState } from './types/user'; const RNOneSignal = NativeOneSignal; const GLOBAL_KEY = '__oneSignalEventManager'; - -// A reloaded module or a duplicate install gets its own class object, so `instanceof` -// cannot recognize the manager already holding this realm's native subscriptions. Adopt -// that manager rather than replacing it: replacing it strands the listeners the other -// module instance registered on a manager that no longer receives native events. -function resolveEventManager(): EventManager { - const globals = globalThis as Record; - const existing = globals[GLOBAL_KEY] as EventManager | undefined; - if (typeof existing?.addEventListener === 'function') { - return existing; - } - - const created = new EventManager(RNOneSignal); - globals[GLOBAL_KEY] = created; - return created; +const prev = (globalThis as Record)[GLOBAL_KEY]; +if (prev instanceof EventManager) { + prev.clearListeners(); } - -const eventManager = resolveEventManager(); +const eventManager = new EventManager(RNOneSignal); +(globalThis as Record)[GLOBAL_KEY] = eventManager; /// An enum that declares different types of log levels you can use with the OneSignal SDK, going from the least verbose (none) to verbose (print all comments). export enum LogLevel {