Skip to content

Commit

Permalink
Check existence of native methods before calling them in NativeEventE…
Browse files Browse the repository at this point in the history
…mitter

Summary:
Check the existence of `addListener` and `removeListeners` in the native module passed to `NativeEventEmitter` to determine if it can be used.

Changelog: [General][Changed] Show warning when native module without `addListener` or `removeListeners` is passed to `NativeEventEmitter`

Reviewed By: yungsters

Differential Revision: D27851425

fbshipit-source-id: c0ad3ba65a9239f5bf84548dab36e8dfbc51058a
  • Loading branch information
rubennorte authored and facebook-github-bot committed Apr 19, 2021
1 parent 1fc1873 commit 114be1d
Showing 1 changed file with 20 additions and 1 deletion.
21 changes: 20 additions & 1 deletion Libraries/EventEmitter/NativeEventEmitter.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,26 @@ export default class NativeEventEmitter<TEventToArgsMap: {...}>
'`new NativeEventEmitter()` requires a non-null argument.',
);
}
this._nativeModule = nativeModule;

const hasAddListener =
!!nativeModule && typeof nativeModule.addListener === 'function';
const hasRemoveListeners =
!!nativeModule && typeof nativeModule.removeListeners === 'function';

if (nativeModule && hasAddListener && hasRemoveListeners) {
this._nativeModule = nativeModule;
} else {
if (!hasAddListener) {
console.warn(
'`new NativeEventEmitter()` was called with a non-null argument without the required `addListener` method.',
);
}
if (!hasRemoveListeners) {
console.warn(
'`new NativeEventEmitter()` was called with a non-null argument without the required `removeListeners` method.',
);
}
}
}

addListener<TEvent: $Keys<TEventToArgsMap>>(
Expand Down

6 comments on commit 114be1d

@satya164
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @rubennorte, can you share some details about the reasoning for these warnings?

@rubennorte
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@satya164 sure. The native module passed to this constructor would be used to notify that subscriptions were added or removed to this event emitter. If the native module doesn't implement those methods there's no reason to pass it to the constructor in the first place. This should help identifying missing methods in the event emitter or unnecessary native modules being passed to the constructor (it should be null instead).

@satya164
Copy link
Contributor

@satya164 satya164 commented on 114be1d Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rubennorte thanks for the response. this argument is required on iOS, so this would require to write code like:

const eventEmitter = new NativeEventEmitter(Platform.OS === 'ios' ? NativeModules.MyModule : null);

which doesn't seem ideal. what people have been doing in libraries to suppress the warning is to add dummy methods for these - and someone recently sent this PR to docs: https://github.com/facebook/react-native-website/pull/2791/files

@rubennorte
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We introduced this change to align the behavior in iOS and Android. Before we would only use that parameter on iOS, but now it's also supported on Android (with the same semantics). We added the warning to make sure you handled this change correctly, either making it explicit that the behavior is different in Android and iOS (like in your example) or implementing the same behavior in Android.

what people have been doing in libraries to suppress the warning is to add dummy methods for these

I think it's ok to decide that but it's also a chance to decide if they could have a better logic in Android. For example, they could use these methods to know when they need to subscribe to a specific native event (when there's at least 1 listener in JS), like we've been doing generally on iOS.

and someone recently sent this PR to docs: https://github.com/facebook/react-native-website/pull/2791/files

I should've done a better job at surfacing this in the docs. Thanks for pointing that out. I'll take a look at that.

@sfuqua
Copy link

@sfuqua sfuqua commented on 114be1d Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rubennorte - I'm seeing these warnings a lot after updating to RN 0.65 as it impacts a lot of libraries my app uses, and I'm not really sure what to do about it.

Notably React Native implements this functionality out-of-the-box for iOS in RCTEventEmitter.m, but ReactContextBaseJavaModule.java does not - which means by default, any Android module that exposes events will hit these but iOS modules will not, which is why the ecosystem is just stubbing these out for Android modules as far as I can tell.

Should the framework provide a default implementation here? After going over this change and several third-party lib Github discussions I'm still not really sure what the "right" way is to handle this in an Android native module. It looks like on iOS it's basically used as a ref-counting mechanism to power startObserving and stopObserving and there's simply no Android equivalent at the moment.

Should the warning couple these together? Assume that if you have one, you intended to implement both, but no-op otherwise? Right now it seems like the best path forward is for apps to just suppress this warning entirely on Android.

edit: After digging around some more, it feels like the right thing to do would be to update ReactContextBaseJavaModule to implement these methods transparently to applications and provide the same startObserving and stopObserving methods to Java that iOS already exposes, with default no-op implementations.

@NiuGuohui
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not friendly to existing libraries in the community.Many libraries often trigger this warning.

Please sign in to comment.