Skip to content
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

[expo] add getNativeModuleIfExists #18913

Merged
merged 4 commits into from
Sep 12, 2022
Merged

Conversation

esamelson
Copy link
Contributor

@esamelson esamelson commented Aug 30, 2022

Why

follow up from #18892 -- we need a way for module authors to be able to do feature detection and bypass our errors

(also added missing ExpoRandom module to exclude list from the react-native-get-random-values lib)

How

Add getNativeModuleIfExists export from the expo package. If the native module doesn't exist, this method just returns null without throwing an error.

An alternative could be patching a method onto the NativeModules object itself, like NativeModules.getIfExists, which would mean external module authors wouldn't need to (perhaps conditionally) import this method from the expo package.

Test Plan

Tested by calling the following things in a test app (running in Expo Go):

  • NativeModules.NonexistentModule -> throws error
  • getNativeModuleIfExists('NonexistentModule') -> returns null
  • getNativeModuleIfExists('LinkingManager') -> returns module

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Aug 30, 2022
* @returns Corresponding native module object, or null if it doesn't exist
*/
export function getNativeModuleIfExists(moduleName: string): any {
return originalNativeModules[moduleName];
Copy link
Member

Choose a reason for hiding this comment

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

Viewing this file (on mobile) it looks like we don't overwrite the NativeModules variable in this file. A benefit of not introducing originalNativeModules is that it becomes possible to call this new method even if createProxyForNativeModules hasn't yet been called.

Suggested change
return originalNativeModules[moduleName];
return NativeModules[moduleName];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't quite work that well, unfortunately, because Object.defineProperty overwrites NativeModules everywhere, including this file if we import it. I've added it as a fallback in case originalNativeModules is null, though, which should cover the same case.

Copy link
Member

Choose a reason for hiding this comment

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

could you try putting const originalNativeModules = NativeModules; in module-level scope (say, after the imports) and seeing if that works? I think what's happening is that NativeModules is always lazily accessed on the react-native module object, and introducing a new variable (originalNativeModules) will eagerly save the value of NativeModules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, that makes sense. Yes, this works -- thanks for the suggestion!

@brentvatne
Copy link
Member

my only concern about this is that i don't think it will help developers who don't depend on the expo package to solve this problem -- react-native-get-random-values wouldn't be able to use this here: https://github.com/LinusU/react-native-get-random-values/blob/0a48c10a395c19affb9e24044a31e6e56afbf20a/index.js#L26-L38

that said, i don't know how we can do this without patching in a method like getIfExists

@esamelson
Copy link
Contributor Author

esamelson commented Sep 1, 2022

Yeah, I think that's a reasonable point. Per @ide's comment here this would be best utilized in packages we maintain (though other module authors could do try { const { getNativeModuleIfExists } = require('expo'); ...). Not sure there is a very elegant solution for external libraries that use expo module fallbacks -- maybe the best thing for now is to just try maintaining a list of them and see how it goes.

@esamelson esamelson requested review from ide and brentvatne and removed request for brentvatne September 6, 2022 12:00
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Sep 6, 2022
@esamelson esamelson merged commit 9731f91 into main Sep 12, 2022
@esamelson esamelson deleted the @eric/get-native-module-if-exists branch September 12, 2022 08:00
Ddv0623 pushed a commit to preciofishbone/expo that referenced this pull request Sep 26, 2022
brentvatne added a commit that referenced this pull request Nov 3, 2022
brentvatne added a commit that referenced this pull request Nov 3, 2022
brentvatne added a commit that referenced this pull request Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants