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

fix: expo should now work with apha #262

Merged
merged 1 commit into from
Sep 12, 2021
Merged

Conversation

dannyhw
Copy link
Member

@dannyhw dannyhw commented Sep 11, 2021

Issue: #247

What I did

Adjusted the docs so that the steps should now produce a working expo project.

I also added a link to a script that also includes a temporary fix for react-native-web.

How to test

Follow the steps for expo setup and run the script linked in the docs and test the expo app

@dannyhw dannyhw added the 6.5 label Sep 11, 2021
@dannyhw dannyhw self-assigned this Sep 11, 2021
@lauriharpf
Copy link
Contributor

Did some testing, hope I understood correctly:

Personally would prefer to avoid adding the webpack.config.js changes at https://gist.github.com/dannyhw/92b3ff0d6ccaead9df2820a507154b87#file-setup_expo_rn_sb-sh-L47 to transpile @storybook and thus prefer the approach in #251 . There are a couple of reasons for this:

  1. dangerouslyAddModulePathsToTranspile sounds like something best avoided if possible 🙂
  2. If we can get away with users of Storybook React Native not needing to modify their webpack.config.js, it would be great. Requiring to do so adds complexity to the setup. Also, the more customizations we need in webpack.config.js, the higher the risk that using Storybook React Native starts to get intertwined with the build customizations the users make (e.g. some combinations don't work).
  3. I'm not sure if transpiling solves use of new APIs, such as the example at fix: Fix #120 (running "yarn web" on Expo fails). #251 (comment) . Perhaps there's a polyfill that gets added, but haven't tested that.

Don't know if I missed something, but it looks like the tutorial steps in this branch aren't fully in sync with the script at https://gist.github.com/dannyhw/92b3ff0d6ccaead9df2820a507154b87 (the webpack.config.js customization is only in the script, not the tutorial?).

One thing I strongly agree with is adding a setup script for Expo, that is an excellent idea 👍 💪 . We could perhaps include all such setup scripts in this repo itself to make it easier for people to make PRs to adjust them. When we have scripts, we could strip out the echo " etc. *nix commands from the tutorial and only include what should be in the file. If someone wants to automate, they can run the scripts fully or partially - the tutorial could be targeted more at those who want to go step-by-step and learn by doing.

@dannyhw
Copy link
Member Author

dannyhw commented Sep 12, 2021

Did some testing, hope I understood correctly:

Personally would prefer to avoid adding the webpack.config.js changes at https://gist.github.com/dannyhw/92b3ff0d6ccaead9df2820a507154b87#file-setup_expo_rn_sb-sh-L47 to transpile @storybook and thus prefer the approach in #251 . There are a couple of reasons for this:

  1. dangerouslyAddModulePathsToTranspile sounds like something best avoided if possible 🙂
  2. If we can get away with users of Storybook React Native not needing to modify their webpack.config.js, it would be great. Requiring to do so adds complexity to the setup. Also, the more customizations we need in webpack.config.js, the higher the risk that using Storybook React Native starts to get intertwined with the build customizations the users make (e.g. some combinations don't work).
  3. I'm not sure if transpiling solves use of new APIs, such as the example at fix: Fix #120 (running "yarn web" on Expo fails). #251 (comment) . Perhaps there's a polyfill that gets added, but haven't tested that.

Don't know if I missed something, but it looks like the tutorial steps in this branch aren't fully in sync with the script at https://gist.github.com/dannyhw/92b3ff0d6ccaead9df2820a507154b87 (the webpack.config.js customization is only in the script, not the tutorial?).

One thing I strongly agree with is adding a setup script for Expo, that is an excellent idea 👍 💪 . We could perhaps include all such setup scripts in this repo itself to make it easier for people to make PRs to adjust them. When we have scripts, we could strip out the echo " etc. *nix commands from the tutorial and only include what should be in the file. If someone wants to automate, they can run the scripts fully or partially - the tutorial could be targeted more at those who want to go step-by-step and learn by doing.

Yeah I actually didn't include it in the tutorial because I only added it in the script for testing purposes, I want to remove it once we have the fix from #120 because I agree with you here that we should work by default where possible instead of including more hacks.

@dannyhw dannyhw merged commit 84c61f8 into next-6.0 Sep 12, 2021
@dannyhw dannyhw deleted the fix/expo-alpha-guide branch September 12, 2021 19:17
dannyhw added a commit to raychanks/react-native that referenced this pull request Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants