Skip to content

Commit

Permalink
Use babel runtime instead of relying on global babelHelpers and regen…
Browse files Browse the repository at this point in the history
…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/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: #198

Reviewed By: mjesun

Differential Revision: D8833903

Pulled By: rafeca

fbshipit-source-id: 7081f769f288ab358ba89ae8ee72a513bb12e225
  • Loading branch information
janicduplessis authored and facebook-github-bot committed Sep 21, 2018
1 parent 0c28693 commit 8932a9c
Show file tree
Hide file tree
Showing 7 changed files with 116 additions and 31 deletions.
1 change: 1 addition & 0 deletions packages/metro-react-native-babel-preset/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
"@babel/plugin-transform-react-jsx": "^7.0.0",
"@babel/plugin-transform-react-jsx-source": "^7.0.0",
"@babel/plugin-transform-regenerator": "^7.0.0",
"@babel/plugin-transform-runtime": "^7.0.0",
"@babel/plugin-transform-shorthand-properties": "^7.0.0",
"@babel/plugin-transform-spread": "^7.0.0",
"@babel/plugin-transform-sticky-regex": "^7.0.0",
Expand Down
12 changes: 12 additions & 0 deletions packages/metro-react-native-babel-preset/src/configs/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,14 @@ const reactDisplayName = [
const reactJsxSource = [require('@babel/plugin-transform-react-jsx-source')];
const symbolMember = [require('../transforms/transform-symbol-member')];

const babelRuntime = [
require('@babel/plugin-transform-runtime'),
{
helpers: true,
regenerator: true,
},
];

const getPreset = (src, options) => {
const isNull = src == null;
const hasClass = isNull || src.indexOf('class') !== -1;
Expand Down Expand Up @@ -135,6 +143,10 @@ const getPreset = (src, options) => {
extraPlugins.push(reactJsxSource);
}

if (!options || !options.disableBabelRuntime) {

This comment has been minimized.

Copy link
@janicduplessis

janicduplessis Sep 22, 2018

Author Contributor

@rafeca What do you think about using https://jest-bot.github.io/jest/docs/configuration.html#unmockedmodulepathpatterns-array-string instead of disabling babel runtime?

This comment has been minimized.

Copy link
@rafeca

rafeca Sep 23, 2018

Contributor

Thanks for the info! this would be helpful for us but we also have issues with some internal tooling that we have around the build processes, so for now we can't use the babel runtime at FB 😞

Anyways, babel-runtime is enabled after this commit, so the community will benefit from it

This comment has been minimized.

Copy link
@janicduplessis

janicduplessis Sep 24, 2018

Author Contributor

Makes sense, I made a PR to remove global babelHelpers and regenerator from RN facebook/react-native#21283. We'll have to figure out how to make it work at FB. Probably possible to override the getPolyfills metro config and add a copy of babelHelpers that lives in an internal directory and add a new polyfill that sets global.regeneratorRuntime.

This comment has been minimized.

Copy link
@rafeca

rafeca Sep 24, 2018

Contributor

ha! I had a similar commit internally that handled our config 😄But I'm going to import yours and merge it with mine since you took care of things in RN land that I didn't know about 🙃

extraPlugins.push(babelRuntime);
}

return {
comments: false,
compact: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,110 +11,110 @@ Array [
0,
],
Array [
4,
6,
0,
5,
0,
],
Array [
6,
8,
0,
2,
0,
"require",
],
Array [
6,
8,
2,
2,
0,
"require",
],
Array [
6,
8,
13,
2,
7,
],
Array [
6,
8,
39,
2,
0,
],
Array [
8,
10,
0,
3,
0,
"arbitrary",
],
Array [
8,
10,
2,
3,
0,
"arbitrary",
],
Array [
8,
10,
11,
3,
9,
],
Array [
8,
10,
12,
3,
10,
"code",
],
Array [
8,
10,
16,
3,
9,
],
Array [
8,
10,
17,
3,
0,
],
Array [
10,
12,
0,
4,
0,
],
Array [
10,
12,
6,
4,
6,
"b",
],
Array [
10,
12,
7,
4,
7,
],
Array [
10,
12,
10,
4,
10,
"require",
],
Array [
10,
12,
21,
4,
17,
],
Array [
10,
12,
45,
4,
0,
Expand Down Expand Up @@ -210,6 +210,31 @@ Array [
]
`;

exports[`code transformation worker: transforms an es module with regenerator 1`] = `
"__d(function (global, _$$_REQUIRE, _$$_IMPORT_DEFAULT, _$$_IMPORT_ALL, module, exports, _dependencyMap) {
var _interopRequireDefault = _$$_REQUIRE(_dependencyMap[0], \\"@babel/runtime/helpers/interopRequireDefault\\");
Object.defineProperty(exports, \\"__esModule\\", {
value: true
});
exports.test = test;
var _regenerator = _interopRequireDefault(_$$_REQUIRE(_dependencyMap[1], \\"@babel/runtime/regenerator\\"));
function test() {
return _regenerator.default.async(function test$(_context) {
while (1) {
switch (_context.prev = _context.next) {
case 0:
case \\"end\\":
return _context.stop();
}
}
}, null, this);
}
});"
`;

exports[`code transformation worker: transforms import/export syntax when experimental flag is on 1`] = `
Array [
Array [
Expand Down
45 changes: 42 additions & 3 deletions packages/metro/src/JSTransformer/worker/__tests__/worker-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,24 +142,63 @@ describe('code transformation worker:', () => {
'__d(function (global, _$$_REQUIRE, _$$_IMPORT_DEFAULT, _$$_IMPORT_ALL, module, exports, _dependencyMap) {',
" 'use strict';",
'',
' var _c = babelHelpers.interopRequireDefault(_$$_REQUIRE(_dependencyMap[0], "./c"));',
' var _interopRequireDefault = _$$_REQUIRE(_dependencyMap[0], "@babel/runtime/helpers/interopRequireDefault");',
'',
' _$$_REQUIRE(_dependencyMap[1], "./a");',
' var _c = _interopRequireDefault(_$$_REQUIRE(_dependencyMap[1], "./c"));',
'',
' _$$_REQUIRE(_dependencyMap[2], "./a");',
'',
' arbitrary(code);',
'',
' var b = _$$_REQUIRE(_dependencyMap[2], "b");',
' var b = _$$_REQUIRE(_dependencyMap[3], "b");',
'});',
].join('\n'),
);
expect(result.output[0].data.map).toMatchSnapshot();
expect(result.dependencies).toEqual([
{
data: {isAsync: false},
name: '@babel/runtime/helpers/interopRequireDefault',
},
{data: {isAsync: false}, name: './c'},
{data: {isAsync: false}, name: './a'},
{data: {isAsync: false}, name: 'b'},
]);
});

it('transforms an es module with regenerator', async () => {
const result = await transform(
'/root/local/file.js',
'local/file.js',
'export async function test() {}',
{
assetExts: [],
assetPlugins: [],
assetRegistryPath: '',
asyncRequireModulePath: 'asyncRequire',
isScript: false,
minifierPath: 'minifyModulePath',
babelTransformerPath,
transformOptions: {dev: true},
dynamicDepsInPackages: 'reject',
},
);

expect(result.output[0].type).toBe('js/module');
expect(result.output[0].data.code).toMatchSnapshot();
expect(result.output[0].data.map).toHaveLength(13);
expect(result.dependencies).toEqual([
{
data: {isAsync: false},
name: '@babel/runtime/helpers/interopRequireDefault',
},
{
data: {isAsync: false},
name: '@babel/runtime/regenerator',
},
]);
});

it('transforms import/export syntax when experimental flag is on', async () => {
const contents = ['import c from "./c";'].join('\n');

Expand Down
11 changes: 3 additions & 8 deletions packages/metro/src/lib/polyfills/__tests__/require-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,6 @@ const fs = require('fs');

const {transformSync} = require('@babel/core');

// Include the external-helpers plugin to be able to detect if they're
// needed when transforming the requirejs implementation.
const PLUGINS = ['@babel/plugin-external-helpers'];

function createModule(
moduleSystem,
moduleId,
Expand All @@ -34,7 +30,6 @@ describe('require', () => {
return transformSync(rawCode, {
ast: false,
babelrc: false,
plugins: PLUGINS.map(require),
presets: [require.resolve('metro-react-native-babel-preset')],
retainLines: true,
sourceMaps: 'inline',
Expand All @@ -56,9 +51,9 @@ describe('require', () => {
});

it('does not need any babel helper logic', () => {
// Super-simple check to validate that no babel helpers are used.
// This check will need to be updated if https://fburl.com/6z0y2kf8 changes.
expect(moduleSystemCode.includes('babelHelpers')).toBe(false);
// The react native preset uses @babel/transform-runtime so helpers will be
// imported from @babel/runtime.
expect(moduleSystemCode.includes('@babel/runtime')).toBe(false);
});

it('works with plain bundles', () => {
Expand Down
4 changes: 1 addition & 3 deletions packages/metro/src/reactNativeTransformer.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
'use strict';

const crypto = require('crypto');
const externalHelpersPlugin = require('@babel/plugin-external-helpers');
const fs = require('fs');
const inlineRequiresPlugin = require('babel-preset-fbjs/plugins/inline-requires');
const makeHMRConfig = require('metro-react-native-babel-preset/src/configs/hmr');
Expand All @@ -25,7 +24,6 @@ import type {Plugins as BabelPlugins} from 'babel-core';

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

Expand Down Expand Up @@ -118,7 +116,7 @@ function buildBabelConfig(filename, options, plugins?: BabelPlugins = []) {
let config = Object.assign({}, babelRC, extraConfig);

// Add extra plugins
const extraPlugins = [externalHelpersPlugin];
const extraPlugins = [];

if (options.inlineRequires) {
extraPlugins.push(inlineRequiresPlugin);
Expand Down
15 changes: 15 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,15 @@
dependencies:
regenerator-transform "^0.13.3"

"@babel/plugin-transform-runtime@^7.0.0":
version "7.0.0"
resolved "https://registry.yarnpkg.com/@babel/plugin-transform-runtime/-/plugin-transform-runtime-7.0.0.tgz#0f1443c07bac16dba8efa939e0c61d6922740062"
integrity sha512-yECRVxRu25Nsf6IY5v5XrXhcW9ZHomUQiq30VO8H7r3JYPcBJDTcxZmT+6v1O3QKKrDp1Wp40LinGbcd+jlp9A==
dependencies:
"@babel/helper-module-imports" "^7.0.0"
"@babel/helper-plugin-utils" "^7.0.0"
resolve "^1.8.1"

"@babel/plugin-transform-shorthand-properties@^7.0.0":
version "7.0.0"
resolved "https://registry.yarnpkg.com/@babel/plugin-transform-shorthand-properties/-/plugin-transform-shorthand-properties-7.0.0.tgz#85f8af592dcc07647541a0350e8c95c7bf419d15"
Expand Down Expand Up @@ -6753,6 +6762,12 @@ resolve@^1.3.2, resolve@^1.5.0:
dependencies:
path-parse "^1.0.5"

resolve@^1.8.1:
version "1.8.1"
resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.8.1.tgz#82f1ec19a423ac1fbd080b0bab06ba36e84a7a26"
dependencies:
path-parse "^1.0.5"

restore-cursor@^2.0.0:
version "2.0.0"
resolved "https://registry.yarnpkg.com/restore-cursor/-/restore-cursor-2.0.0.tgz#9f7ee287f82fd326d4fd162923d62129eee0dfaf"
Expand Down

0 comments on commit 8932a9c

Please sign in to comment.