-
Notifications
You must be signed in to change notification settings - Fork 613
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
Use babel runtime instead of relying on global babelHelpers and regenerator #198
Conversation
Not sure how we should go about including the @babel/runtime dependency, it could be added here or in RN, I think RN makes more sense since that is where the babelHelpers / regeneratorRuntime are defined atm. If this makes sense I will make a PR on RN that adds @babel/runtime and removes babelHelpers and the regenerator global. |
Codecov Report
@@ Coverage Diff @@
## master #198 +/- ##
==========================================
- Coverage 84.24% 84.24% -0.01%
==========================================
Files 134 134
Lines 4406 4405 -1
Branches 691 691
==========================================
- Hits 3712 3711 -1
Misses 614 614
Partials 80 80
Continue to review full report at Codecov.
|
packages/metro/package.json
Outdated
@@ -15,7 +15,6 @@ | |||
"dependencies": { | |||
"@babel/core": "7.0.0-beta.48", | |||
"@babel/generator": "7.0.0-beta.48", | |||
"@babel/helper-remap-async-to-generator": "7.0.0-beta.48", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was checking deps and those were not used in the metro package.
@@ -28,7 +27,6 @@ type ModuleES6 = {__esModule?: boolean, default?: {}}; | |||
|
|||
const cacheKeyParts = [ | |||
fs.readFileSync(__filename), | |||
require('babel-plugin-external-helpers/package.json').version, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we still keep the babel version as part of that cache key?
Thanks for doing this! This looks like the right approach, much more robust 👍 I think it's better to have this in metro tbh. I guess that you're going to remove https://github.com/facebook/react-native/blob/master/rn-get-polyfills.js#L21 once this change gets propagated to RN? I'm going to import this PR to our internal systems to check that it does not break anything or it does not cause any build size regression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafeca has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@rafeca Yes we’ll be able to remove that as well as the global regenerator we add in InitializeCore (https://github.com/facebook/react-native/blob/master/Libraries/Core/InitializeCore.js#L90). |
This looks great! Will this make it possible for RN apps which target a modern JavaScriptCore (e.g. iOS 10.3+) to use a custom babel config that does not even need the regenerator runtime? (because generator functions are natively supported?) |
It will help since RN won’t always bundle regeneratorRuntime, it will only be added if babel needs it (this means it won’t add it when targeting modern JSC with a custom babel config). |
@rafeca Did you have a chance to look at this? |
d717970
to
3ae32a5
Compare
Any movement on this? It looks like a great addition to decouple babel dependencies from React Native itself. |
Will this fix the whole regeneratorRuntime missing issue? |
@janicduplessis Babel maintainer here. This would be great overall, since I think I'll also say that the 7.0.0 I'd expect this to become more and more of a problem since we'll likely add new helpers over 7.x's lifetime and we assume that, if you inject a reference, you're saying that you actually have that helper available. |
Any movement of this? |
Hey folks, when I tried to import this PR it caused a few errors in our internal builds, I'm going to try again next week (probably tweaking it a bit) and see if I can squeeze it in 😃 |
3ae32a5
to
452efbb
Compare
@rafeca Cool, I just rebased on master to fix conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rafeca has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@rafeca Will this cherry-picked in next release? |
@wangshangming This has not been merged to master yet, when it does we'll know when it will make it in a release. |
I'm still dealing with some internal issues in order to be able to merge this PR, but making slow but steady progress. To give some context, these are the two main issues found:
Lastly, once I get everything working I'll do some performance measurement to check that the startup times of RN apps does not regress (adding new I'll keep you informed |
This might not be relevant at all, but just so you know, you can pass |
…erator (#198) Summary: **Summary** The RN transformer currently relies on the enviroment providing babelHelpers and regeneratorRuntime as globals by using 'babel-external-helpers'. This wasn't really a problem before since helpers were stable and we could maintain our copy easily but it seems like there are more now with babel 7 and it makes sense to include only those used by the app. This is exactly what babel/transform-runtime does. It will alias all helpers and calls to regeneratorRuntime to files in the babel/runtime package. This will solve issues like this #20150 caused by missing babelHelpers. This solution also avoids bloating babelHelpers to fix OSS issues like the one linked before. **Test plan** - Updated tests so they all pass. - Tested that it actually works by applying the changes locally in an RN app. - Added a test for async functions, to make sure regenerator is aliased properly and doesn't depend on the global. - Made sure require-test.js still fails if the require implementation contains babel helpers (by adding an empty class in the file). Pull Request resolved: facebook/metro#198 Reviewed By: mjesun Differential Revision: D8833903 Pulled By: rafeca fbshipit-source-id: 7081f769f288ab358ba89ae8ee72a513bb12e225
…erator (#198) Summary: **Summary** The RN transformer currently relies on the enviroment providing babelHelpers and regeneratorRuntime as globals by using 'babel-external-helpers'. This wasn't really a problem before since helpers were stable and we could maintain our copy easily but it seems like there are more now with babel 7 and it makes sense to include only those used by the app. This is exactly what babel/transform-runtime does. It will alias all helpers and calls to regeneratorRuntime to files in the babel/runtime package. This will solve issues like this #20150 caused by missing babelHelpers. This solution also avoids bloating babelHelpers to fix OSS issues like the one linked before. **Test plan** - Updated tests so they all pass. - Tested that it actually works by applying the changes locally in an RN app. - Added a test for async functions, to make sure regenerator is aliased properly and doesn't depend on the global. - Made sure require-test.js still fails if the require implementation contains babel helpers (by adding an empty class in the file). Pull Request resolved: facebook/metro#198 Reviewed By: mjesun Differential Revision: D8833903 Pulled By: rafeca fbshipit-source-id: 7081f769f288ab358ba89ae8ee72a513bb12e225
…erator (facebook#198) Summary: **Summary** The RN transformer currently relies on the enviroment providing babelHelpers and regeneratorRuntime as globals by using 'babel-external-helpers'. This wasn't really a problem before since helpers were stable and we could maintain our copy easily but it seems like there are more now with babel 7 and it makes sense to include only those used by the app. This is exactly what babel/transform-runtime does. It will alias all helpers and calls to regeneratorRuntime to files in the babel/runtime package. This will solve issues like this facebook/react-native#20150 caused by missing babelHelpers. This solution also avoids bloating babelHelpers to fix OSS issues like the one linked before. **Test plan** - Updated tests so they all pass. - Tested that it actually works by applying the changes locally in an RN app. - Added a test for async functions, to make sure regenerator is aliased properly and doesn't depend on the global. - Made sure require-test.js still fails if the require implementation contains babel helpers (by adding an empty class in the file). Pull Request resolved: facebook#198 Reviewed By: mjesun Differential Revision: D8833903 Pulled By: rafeca fbshipit-source-id: 7081f769f288ab358ba89ae8ee72a513bb12e225
…erator (#198) Summary: **Summary** The RN transformer currently relies on the enviroment providing babelHelpers and regeneratorRuntime as globals by using 'babel-external-helpers'. This wasn't really a problem before since helpers were stable and we could maintain our copy easily but it seems like there are more now with babel 7 and it makes sense to include only those used by the app. This is exactly what babel/transform-runtime does. It will alias all helpers and calls to regeneratorRuntime to files in the babel/runtime package. This will solve issues like this facebook#20150 caused by missing babelHelpers. This solution also avoids bloating babelHelpers to fix OSS issues like the one linked before. **Test plan** - Updated tests so they all pass. - Tested that it actually works by applying the changes locally in an RN app. - Added a test for async functions, to make sure regenerator is aliased properly and doesn't depend on the global. - Made sure require-test.js still fails if the require implementation contains babel helpers (by adding an empty class in the file). Pull Request resolved: facebook/metro#198 Reviewed By: mjesun Differential Revision: D8833903 Pulled By: rafeca fbshipit-source-id: 7081f769f288ab358ba89ae8ee72a513bb12e225
Summary
The RN transformer currently relies on the enviroment providing babelHelpers and regeneratorRuntime as globals by using 'babel-external-helpers'. This wasn't really a problem before since helpers were stable and we could maintain our copy easily but it seems like there are more now with babel 7 and it makes sense to include only those used by the app.
This is exactly what @babel/transform-runtime does. It will alias all helpers and calls to regeneratorRuntime to files in the @babel/runtime package.
This will solve issues like this facebook/react-native#20150 caused by missing babelHelpers. This solution also avoids bloating babelHelpers to fix OSS issues like the one linked before.
Test plan