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

Bundle command does not add InitializeCore to runBeforeMainModule #197

Closed
Titozzz opened this issue Jul 11, 2018 · 9 comments
Closed

Bundle command does not add InitializeCore to runBeforeMainModule #197

Titozzz opened this issue Jul 11, 2018 · 9 comments

Comments

@Titozzz
Copy link

Titozzz commented Jul 11, 2018

Bug report
I was tracking a react native bug, but since my fix is inside metro code, I figured I repost here:
facebook/react-native#19827 (comment)

What is the current behavior?
When using react-native bundle (the command that runs when you build a release), InitializeCore.js is not the first file that get required, but instead it is required later, sometime during the generation of the bridge method, randomly (which is why if you are lucky or not using Promise // setTimeout early you'll be fine)
iOS does not need all those polyfills which is why the issue is only detected on android in release mode.

My insight:
Metro does not add InitializeCore.js to the entry point when building the js bundle. I've logged here. The issue being that it takes options.runBeforeMainModule that defaults to [].
Maybe we could merge it with _this._opts.getModulesRunBeforeMainModule() which defaults to ['/node_modules/react-native/Libraries/Core/InitializeCore.js'].

I've tried to replace it with

runBeforeMainModule: [
            ...options.runBeforeMainModule,
            ..._this._opts.getModulesRunBeforeMainModule(),
          ],
and android was working in release 🎉

I'd like to get more insight from other people and feel free to ask any other question, thanks

To reproduce just create a new app with RN 0.56 and console.log(Promise) at the top of your index.js, you'll get Promise is undefined when android release.

RN version: 0.56.0 and related metro version.

@rodrigobdz
Copy link
Contributor

The problem is also present in iOS. Your fix solves it.

- and android was working in release 🎉
+ and iOS and Android were working in release 🎉

@Titozzz
Copy link
Author

Titozzz commented Jul 12, 2018

Yeah I knew it was not bundled for either, but I did not thought iOS needed the polyfills and was crashing, so good catch @rodrigobdz 🎉

@rafeca
Copy link
Contributor

rafeca commented Jul 12, 2018

Thanks for the report! @Titozzz your suggestion is correct, I'm going to prepare a fix for this

@danielgindi
Copy link

I think this might also cause this: facebook/react-native#20171

@Titozzz
Copy link
Author

Titozzz commented Jul 13, 2018

@danielgindi Does my fix also addresses that issue?

@danielgindi
Copy link

@Titozzz Yes! I got to try it today and it works!

@rafeca
Copy link
Contributor

rafeca commented Jul 13, 2018

Hey folks, 64a5b1d has just landed with a fix for this. I'm going to cherry-pick this change to the 0.38.x branch and publish [email protected]

Sorry for the inconvenience

rafeca added a commit that referenced this issue Jul 13, 2018
…production bundles

Summary: This fixes #197

Differential Revision: D8818833

fbshipit-source-id: d80a824bb4fd90ef32104c651aa3c1ecc79ebad9
@rodrigobdz
Copy link
Contributor

rodrigobdz commented Jul 13, 2018

@rafeca It's great news that it is now fixed but the question here is: how can we prevent this from happening? No additional tests were added in 64a5b1d.

@rafeca
Copy link
Contributor

rafeca commented Jul 13, 2018

@rodrigobdz we're currently working on a new configuration system for metro which will standardize the options object across the codebase and simplify the API boundary between react native and metro. As part of this initiative we will add some end to end tests to Metro that should be able to capture these issues.

To give some context, internally at FB we have quite custom logic for building the production bundles, so we don't use the logic that integrates react native with metro. We have our own end to end tests but they are testing the integration between Metro and our internal infra, so it's currently hard for us to detect these breakages (this'll change soon once we have the new API though).

rafeca added a commit that referenced this issue Jul 13, 2018
…production bundles

Summary: This fixes #197

Differential Revision: D8818833

fbshipit-source-id: d80a824bb4fd90ef32104c651aa3c1ecc79ebad9
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

No branches or pull requests

4 participants