Skip to content

Conversation

@EvanBacon
Copy link
Contributor

@EvanBacon EvanBacon commented Apr 23, 2019

What kind of change does this PR introduce?

What is the current behavior?

fixes #1806

What is the new behavior?

You can now use a wide variety of Expo modules.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

@EvanBacon
Copy link
Contributor Author

Can someone recommend a good way to test this?

@CompuIves
Copy link
Member

Wooow, this is awesome!! Thanks a lot @EvanBacon! We deployed the PR to here: http://pr1807.cs.lbogdan.tk/s/new. There you can also test if you can add expo modules. I'd be glad to help get this through!

@EvanBacon
Copy link
Contributor Author

@CompuIves thanks for the deploy! The aliases seem to work as expected. It looks like there is a bug where .web extensions aren't being used in node modules.

// This works: `expo-asset/build/AssetRegistry.web.js`
import * as AssetRegistry from 'expo-asset/build/AssetRegistry.web'

// This doesn't, it resolves to: `expo-asset/build/AssetRegistry.js`
// which also exists but is being used for the native functionality
// essentially returning an empty object as it's looking for a React Native `NativeModule`. 
import * as AssetRegistry from 'expo-asset/build/AssetRegistry'

CompuIves added a commit that referenced this pull request Apr 24, 2019
Fixes the issue with #1807
@CompuIves
Copy link
Member

Ahh, I see! I pushed a fix to master, if you merge it should work (on staging at least).

@EvanBacon
Copy link
Contributor Author

This works really well! Still some bugs that I can work out in the individual modules. One more thing that would be nice is the environment variable __DEV__ that Metro provides. Not sure if this will conflict with anything on your end though, we do this in the expo/webpack-config here to better support libraries that aren't fully optimized for web.

SaraVieira pushed a commit that referenced this pull request Apr 30, 2019
Fixes the issue with #1807
@EvanBacon
Copy link
Contributor Author

Given that __DEV__ is not an actual blocker, I think it would make sense to merge this PR as is unless @CompuIves has any other concerns.

@CompuIves
Copy link
Member

CompuIves commented Apr 30, 2019

That's great!! I'll merge this in tomorrow morning!

Really cool to have Expo support soon in CodeSandbox, I've been meaning for a long time to look into it. Thank you so much for contributing this @EvanBacon!

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.

Expo support

2 participants