Use babel runtime instead of relying on global babelHelpers and regenerator #198

janicduplessis · 2018-07-12T23:37:19Z

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).

janicduplessis · 2018-07-12T23:38:50Z

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-io · 2018-07-12T23:39:28Z

Codecov Report

Merging #198 into master will decrease coverage by <.01%.
The diff coverage is 100%.

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

Impacted Files	Coverage Δ
...etro-react-native-babel-preset/src/configs/main.js	`100% <ø> (ø)`	⬆️
packages/metro/src/reactNativeTransformer.js	`70.9% <100%> (-0.52%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e5833b...8ed955a. Read the comment docs.

janicduplessis · 2018-07-12T23:40:36Z

packages/metro/package.json

@@ -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",


I was checking deps and those were not used in the metro package.

janicduplessis · 2018-07-13T02:57:01Z

packages/metro/src/reactNativeTransformer.js

@@ -28,7 +27,6 @@ type ModuleES6 = {__esModule?: boolean, default?: {}};

 const cacheKeyParts = [
  fs.readFileSync(__filename),
-  require('babel-plugin-external-helpers/package.json').version,


Should we still keep the babel version as part of that cache key?

rafeca · 2018-07-13T13:10:43Z

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

facebook-github-bot

@rafeca has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

janicduplessis · 2018-07-13T14:36:42Z

@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).

newyankeecodeshop · 2018-07-18T16:51:08Z

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?)

janicduplessis · 2018-07-25T14:24:22Z

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).

janicduplessis · 2018-07-25T14:24:47Z

@rafeca Did you have a chance to look at this?

newyankeecodeshop · 2018-08-28T20:58:16Z

Any movement on this? It looks like a great addition to decouple babel dependencies from React Native itself.

ItsNoHax · 2018-08-30T13:20:59Z

Will this fix the whole regeneratorRuntime missing issue?

loganfsmyth · 2018-09-03T19:04:57Z

@janicduplessis Babel maintainer here. This would be great overall, since I think external-helpers isn't that useful now that @babel/runtime is trimmed down.

I'll also say that the 7.0.0 external-helpers transform also accepts the same whitelist option that babel.buildExternalHelpers does, https://github.com/babel/babel/blob/master/packages/babel-plugin-external-helpers/src/index.js#L10, so if there are issues with getting this landed in the short term, a quick fix would be to take the list of helpers that RN builds and pass it to the transform. That way it will fall back to injecting the helpers into each individual file, instead of inserting references to helpers that don't exist.

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.

simonwang6666 · 2018-09-05T07:14:27Z

Any movement of this?

rafeca · 2018-09-08T20:53:12Z

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 😃

…erator

janicduplessis · 2018-09-10T04:16:39Z

@rafeca Cool, I just rebased on master to fix conflicts.

facebook-github-bot

rafeca has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

simonwang6666 · 2018-09-12T08:49:52Z

@rafeca Will this cherry-picked in next release?

janicduplessis · 2018-09-12T16:33:19Z

@wangshangming This has not been merged to master yet, when it does we'll know when it will make it in a release.

rafeca · 2018-09-14T11:44:22Z

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:

The current [babelHelpers](https://github.com/facebook/react-native/blob/master/Libraries/polyfills/babelHelpers.js) that we use are from babel-7.0.0-beta.47. Between that beta and latest stable differ slightly in behaviour, making some of our tests fail.
Because @babel/runtime adds a bunch of requires instead of calling the static methods, tests that use jest.autoMock() stop working since they also mock the runtime. Unfortunately we still have lots of old tests with that so I'm dealing with them.

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 require calls adds a small overhead, and this together with inline requires could cause perf regressions).

I'll keep you informed

LinusU · 2018-09-17T09:55:35Z

[...] adding new require calls [...]

This might not be relevant at all, but just so you know, you can pass useESModules: true to @babel/plugin-transform-runtime in order to use import instead of require. That could potentially be optimized better...

…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

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 12, 2018

janicduplessis commented Jul 12, 2018

View reviewed changes

janicduplessis commented Jul 13, 2018

View reviewed changes

facebook-github-bot reviewed Jul 13, 2018

View reviewed changes

janicduplessis mentioned this pull request Jul 18, 2018

[GENERAL][BUGFIX] Add asyncToGenerator to babel polyfills facebook/react-native#20253

Closed

janicduplessis force-pushed the babel-runtime branch 2 times, most recently from d717970 to 3ae32a5 Compare August 3, 2018 04:36

newyankeecodeshop mentioned this pull request Sep 7, 2018

0.57.0 Discussion react-native-community/releases#34

Closed

Use babel runtime instead of relying on global babelHelpers and regen…

452efbb

…erator

janicduplessis force-pushed the babel-runtime branch from 3ae32a5 to 452efbb Compare September 10, 2018 04:14

Remove external-helpers-plugin

8ed955a

facebook-github-bot reviewed Sep 11, 2018

View reviewed changes

LinusU mentioned this pull request Sep 14, 2018

Bundle failure on decorators facebook/react-native#19955

Closed

3 tasks

LinusU mentioned this pull request Sep 18, 2018

[0.56][RELEASE] Bundler cuts off some used babel helpers in release build (decorators issue) facebook/react-native#20150

Closed

3 tasks

sunnylqm mentioned this pull request Sep 20, 2018

react native 0.57 cannot resolve module "@babel/plugin-external-helpers" facebook/react-native#21214

Closed

facebook-github-bot closed this in 8932a9c Sep 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use babel runtime instead of relying on global babelHelpers and regenerator #198

Use babel runtime instead of relying on global babelHelpers and regenerator #198

janicduplessis commented Jul 12, 2018

janicduplessis commented Jul 12, 2018 •

edited

Loading

codecov-io commented Jul 12, 2018 •

edited

Loading

janicduplessis Jul 12, 2018

janicduplessis Jul 13, 2018

rafeca commented Jul 13, 2018

facebook-github-bot left a comment

janicduplessis commented Jul 13, 2018 •

edited

Loading

newyankeecodeshop commented Jul 18, 2018

janicduplessis commented Jul 25, 2018

janicduplessis commented Jul 25, 2018

newyankeecodeshop commented Aug 28, 2018

ItsNoHax commented Aug 30, 2018

loganfsmyth commented Sep 3, 2018 •

edited

Loading

simonwang6666 commented Sep 5, 2018

rafeca commented Sep 8, 2018

janicduplessis commented Sep 10, 2018

facebook-github-bot left a comment

simonwang6666 commented Sep 12, 2018

janicduplessis commented Sep 12, 2018

rafeca commented Sep 14, 2018

LinusU commented Sep 17, 2018

Use babel runtime instead of relying on global babelHelpers and regenerator #198

Use babel runtime instead of relying on global babelHelpers and regenerator #198

Conversation

janicduplessis commented Jul 12, 2018

janicduplessis commented Jul 12, 2018 • edited Loading

codecov-io commented Jul 12, 2018 • edited Loading

Codecov Report

janicduplessis Jul 12, 2018

Choose a reason for hiding this comment

janicduplessis Jul 13, 2018

Choose a reason for hiding this comment

rafeca commented Jul 13, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

janicduplessis commented Jul 13, 2018 • edited Loading

newyankeecodeshop commented Jul 18, 2018

janicduplessis commented Jul 25, 2018

janicduplessis commented Jul 25, 2018

newyankeecodeshop commented Aug 28, 2018

ItsNoHax commented Aug 30, 2018

loganfsmyth commented Sep 3, 2018 • edited Loading

simonwang6666 commented Sep 5, 2018

rafeca commented Sep 8, 2018

janicduplessis commented Sep 10, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

simonwang6666 commented Sep 12, 2018

janicduplessis commented Sep 12, 2018

rafeca commented Sep 14, 2018

LinusU commented Sep 17, 2018

janicduplessis commented Jul 12, 2018 •

edited

Loading

codecov-io commented Jul 12, 2018 •

edited

Loading

janicduplessis commented Jul 13, 2018 •

edited

Loading

loganfsmyth commented Sep 3, 2018 •

edited

Loading