-
Notifications
You must be signed in to change notification settings - Fork 15
chore: remove old architecture event listener callbacks #546
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,4 @@ | ||
| import { | ||
| NativeEventEmitter, | ||
| type EventSubscription, | ||
| type TurboModule, | ||
| } from 'react-native'; | ||
| import { type EventSubscription, type TurboModule } from 'react-native'; | ||
| import { InlineInAppMessageView } from './components'; | ||
| import { NativeLoggerListener } from './native-logger-listener'; | ||
| import NativeCustomerIOMessagingInApp, { | ||
|
|
@@ -11,22 +7,13 @@ import NativeCustomerIOMessagingInApp, { | |
| import type { InAppMessageEventType } from './types'; | ||
| import { callNativeModule, ensureNativeModule } from './utils/native-bridge'; | ||
|
|
||
| // Constant value used for emitting all events for in-app from native modules | ||
| const InAppEventListenerEventName = 'InAppEventListener'; | ||
|
|
||
| /** | ||
| * Ensures all methods defined in codegen spec are implemented by the public module | ||
| * | ||
| * @internal | ||
| */ | ||
| interface NativeInAppSpec | ||
| extends Omit< | ||
| CodegenSpec, | ||
| | keyof TurboModule | ||
| | 'onInAppEventReceived' | ||
| | 'addListener' | ||
| | 'removeListeners' | ||
| > {} | ||
| extends Omit<CodegenSpec, keyof TurboModule | 'onInAppEventReceived'> {} | ||
|
|
||
| // Reference to the native CustomerIO Data Pipelines module for SDK operations | ||
| const nativeModule = ensureNativeModule(NativeCustomerIOMessagingInApp); | ||
|
|
@@ -59,16 +46,22 @@ class CustomerIOInAppMessaging implements NativeInAppSpec { | |
|
|
||
| return withNativeModule((native) => { | ||
| try { | ||
| // Try new arch TurboModule event listener (not available in old arch) | ||
| // Register TurboModule event listener and return subscription. | ||
| // This method is generated by codegen in the native modules. | ||
| // Wrapped in try-catch due to previous reports of crashes on certain Android architectures. | ||
mrehan27 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return native.onInAppEventReceived(emitter); | ||
| } catch { | ||
| } catch (error) { | ||
| NativeLoggerListener.warn( | ||
| 'Falling back to legacy event emitter (likely using old architecture). ' + | ||
| 'Switch to new architecture for better performance and event handling.' | ||
| 'Failed to attach in-app event listener:', | ||
| error | ||
| ); | ||
| // Fallback to old arch NativeEventEmitter when new arch method fails | ||
| const eventEmitter = new NativeEventEmitter(native); | ||
| return eventEmitter.addListener(InAppEventListenerEventName, emitter); | ||
| // Return a no-op subscription to maintain backwards compatibility | ||
| return { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apps using old arch won't compile so the failure wouldn't be silent. |
||
| remove: () => {}, | ||
| eventType: '', | ||
| key: 0, | ||
| subscriber: null as any, | ||
| } as EventSubscription; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On old architecture: Once marked initialized, the system never attempts to reconnect, is this the intent here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Old arch is removed in next PRs in stack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job cleaning this up!