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

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

Merged

Conversation

brentvatne
Copy link
Contributor

@brentvatne brentvatne commented Aug 19, 2020

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 expo/expo#7209

@ctavan
Copy link

ctavan commented Aug 20, 2020

Also fixes uuidjs/uuid#375

@brentvatne
Copy link
Contributor Author

@LinusU - any feedback on this?

@LinusU LinusU force-pushed the @brent/expo-managed-workflow-native-module branch from 8e25a1a to 85f4839 Compare September 3, 2020 09:13
@LinusU
Copy link
Owner

LinusU commented Sep 3, 2020

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

I amended some small changes:

  • fixed typ in readme
  • switched back to require (I'm not comfortable chaining in non-major version, even though it shouldn't be a problem for anyone)
  • code linting

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 LinusU merged commit 86b1ac9 into LinusU:master Sep 3, 2020
@LinusU
Copy link
Owner

LinusU commented Sep 3, 2020

Released in 🚢 1.5.0 / 2020-09-03

@brentvatne
Copy link
Contributor Author

thanks @LinusU!

ctavan added a commit to uuidjs/uuid that referenced this pull request Sep 9, 2020
Thanks to
LinusU/react-native-get-random-values#13 Expo
finally has support for a sync CSPRNG.

Fixes #375
@tasn
Copy link

tasn commented Sep 11, 2020

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
Copy link
Contributor Author

brentvatne commented Sep 11, 2020

@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
Copy link

tasn commented Sep 12, 2020

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
Copy link
Contributor Author

@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
Copy link

tasn commented Sep 14, 2020

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
Copy link
Contributor Author

@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
Copy link
Owner

LinusU commented Sep 15, 2020

@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
Copy link
Contributor Author

@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
Copy link

ctavan commented Oct 4, 2020

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

ctavan added a commit to uuidjs/uuid that referenced this pull request Oct 4, 2020
Thanks to
LinusU/react-native-get-random-values#13 Expo
finally has support for a sync CSPRNG.

Fixes #375
ctavan added a commit to uuidjs/uuid that referenced this pull request Oct 4, 2020
ctavan added a commit to uuidjs/uuid that referenced this pull request Oct 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compatibility with uuid 7.x, nanoid, and others
4 participants