search

LinusU / react-native-get-random-values

A small implementation of `getRandomValues` for React Native
MIT License
350 stars 48 forks source link

🌊 Add Expo managed workflow support for SDK 39+ #13

Closed brentvatne closed 4 years ago

brentvatne commented 4 years ago

This is done by using the ExpoRandom native module when running in the Expo managed workflow, if it exists.

This does not add any overhead to apps that are not running in the Expo managed workflow. The .expo.js file is not included in the bundle.

Fixes https://github.com/expo/expo/issues/7209

ctavan commented 4 years ago

Also fixes https://github.com/uuidjs/uuid/issues/375

brentvatne commented 4 years ago

@LinusU - any feedback on this?

LinusU commented 4 years ago

Hey @brentvatne, sorry for the delay on this, things have been a bit hectic lately.

I amended some small changes:

I also tired to test this myself, but as SDK 39 isn't out yet I wasn't able to 😅 Anyhow, I get the proper error message on Expo so it's at least reading the correct file, I guess it will work when SDK 39 is out 🚀

Thank you for this and all your work on Expo! ❤️

LinusU commented 4 years ago

Released in 🚢 1.5.0 / 2020-09-03

brentvatne commented 4 years ago

thanks @LinusU!

tasn commented 4 years ago

Hey,

This PR broke apps using an ejected expo with an SDK older than 39.0.0. :( It should detect if the app is ejected and if so, not complain about the the expo version being older than 39.0.0, or in the meanwhile, just remove this check altogether?

brentvatne commented 4 years ago

@tasn - it sounds like something is wrong with your app configuration. .expo.js files should only be loaded when you're in a managed app (in expo client or in standalone app built from only js code using expo build:ios/android)

tasn commented 4 years ago

I have an ejected SDK 36 app with Typescript and I haven't messed with the metro config. How far back to .expo.js fix go for?

No worries for me btw, I just help back the update, just wanted to let you know.

brentvatne commented 4 years ago

@tasn - ah, are you using expokit? that could be the reason.. we don't support expokit anymore (as of sdk 38) so we can't guarantee that new things we do will be compatible with it. https://blog.expo.io/time-to-start-using-expos-bare-workflow-expokit-now-deprecated-d6052890c18b

tasn commented 4 years ago

I am, yeah.

I understand you don't support expokit anymore, and I've been meaning to upgrade. It's just a massive pain, so I haven't found the time yet. However, this library is not part of expo. Is there a way to maybe make it not break for expokit? Doesn't really matter to me, as I just reverted to 1.4.0, so no worries if too much of a hassle.

brentvatne commented 4 years ago

@tasn - you could install expo-random and that would solve your problem. otherwise there isn't much we can do in the context of expokit, i think your solution of just reverting to 1.4.0 is good for now and when you have time to migrate away from expokit you can upgrade this as well

LinusU commented 4 years ago

@brentvatne I'm guessing that the answer is no, but is there any tricky work around that we could do here in the package? No .expokit.js or similar? 😅

brentvatne commented 4 years ago

@LinusU - unfortunately no, expokit doesn't have a special extension, so we couldn't do this at the module resolution level. there may be some properties we could inspect at runtime to choose the appropriate behavior, but i think it's not worth adding the complexity to support it.

ctavan commented 4 years ago

BTW I just verified that this now indeed works as desired with Expo SDK 39 🎉 ! See https://github.com/ctavan/uuid-example-expo