Skip to content

Switch back to babel-loader #5143

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Sep 27, 2018
Merged

Switch back to babel-loader #5143

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c92efca
Switch back to babel-loader
iansu Sep 27, 2018
fcfecca
Preserve existing caller options. Use Object.assign instead of object…
iansu Sep 27, 2018
0b6ca8a
Updated filename in package.json
iansu Sep 27, 2018
95dc224
Update comment about cache identifier
iansu Sep 27, 2018
a9f10a1
Update macro check to use a regex
iansu Sep 27, 2018
d3bc770
Move macro check regex out of function
iansu Sep 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions packages/babel-preset-react-app/loader.js
View file
Open in desktop

This file was deleted.

32 changes: 0 additions & 32 deletions packages/babel-preset-react-app/overrides.js
View file
Open in desktop

This file was deleted.

3 changes: 1 addition & 2 deletions packages/babel-preset-react-app/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
"dev.js",
"index.js",
"loader.js",
"overrides.js",
"prod.js",
"test.js"
],
Expand All @@ -31,7 +30,7 @@
"@babel/preset-flow": "7.0.0",
"@babel/preset-react": "7.0.0",
"@babel/runtime": "7.0.0",
"babel-loader": "8.0.2",
"babel-loader": "8.0.4",
"babel-plugin-macros": "2.4.2",
"babel-plugin-transform-dynamic-import": "2.1.0",
"babel-plugin-transform-react-remove-prop-types": "0.4.18"
Expand Down
31 changes: 31 additions & 0 deletions packages/babel-preset-react-app/webpack-overrides.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
'use strict';

const crypto = require('crypto');

module.exports = function() {
return {
// This function transforms the Babel configuration on a per-file basis
config(config, { source }) {
// Babel Macros are notoriously hard to cache, so they shouldn't be
// https://github.com/babel/babel/issues/8497
// We naively detect macros using their package suffix and insert a random
// caller name, a valid option accepted by Babel, to compose a one-time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: update comment to reflect the move from modifying the name to adding craInvalidationToken

// cacheIdentifier for the file. We cannot tune the loader options on a per
// file basis.
if (source.indexOf('.macro') !== -1 || source.indexOf('/macro') !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: you can do one lookup for just macro.
Then check the character at previous index.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make that work we'd have to save the index into a variable. Is that tradeoff worth it?

Copy link
Contributor

@Timer Timer Sep 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At that point, we might as well just look for macro. If the previous character wasn't . or /, we'd probably want to search again -- nullifying any performance gain.
Though, 9/10 times we'd probably see that . or / character and get to bail out early.

FWIW, indexOf appears to be extremely fast: https://jsperf.com/test-indexof-vs-includes
This only needs to be ran once on a file that doesn't include macros.

(I'm sure it's way slower on a large file, but faster than babel parsing it!)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume regex would be way slower?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make that work we'd have to save the index into a variable. Is that tradeoff worth it?

What do you mean? There's no real cost for putting it into a variable in this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume regex would be way slower?

It depends. Regex can actually be faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just mean the cost of both seems quite low so is it worth trading one for the other? I think the way it is currently it's easy to read and understand what's going on. I think it gets a bit more complicated/less clear if we change the algorithm (see @Timer's comment above).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Timer I would assume regex would be slower too but then I started to second guess that.

https://koukia.ca/top-6-ways-to-search-for-a-string-in-javascript-and-performance-benchmarks-ce3e9b81ad31#---0-21

Maybe doing both checks in one go with a regex would be fastest?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with using a regex.

I disagree re: the cost being low. That's how you get to slow builds, one small decision at a time. If it's a hot path (which it is for cold builds) let's put in some effort to avoid doing the same work twice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh. Whatever. I'm just being grumpy. It probably doesn't matter at this scale. Things like this tend to matter in React but this is not React.

return Object.assign({}, config.options, {
caller: Object.assign({}, config.options.caller, {
craInvalidationToken: crypto.randomBytes(32).toString('hex'),
}),
});
}
return config.options;
},
};
};
2 changes: 1 addition & 1 deletion packages/react-error-overlay/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
"babel-core": "7.0.0-bridge.0",
"babel-eslint": "9.0.0",
"babel-jest": "23.6.0",
"babel-loader": "8.0.2",
"babel-loader": "8.0.4",
"babel-preset-react-app": "^4.0.0",
"chalk": "^2.3.2",
"chokidar": "^2.0.2",
Expand Down
8 changes: 4 additions & 4 deletions packages/react-scripts/config/webpack.config.dev.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,11 +218,11 @@ module.exports = {
},
},
{
// We need to use our own loader until `babel-loader` supports
// customization
// https://github.com/babel/babel-loader/pull/687
loader: require.resolve('babel-preset-react-app/loader'),
loader: require.resolve('babel-loader'),
options: {
customize: require.resolve(
'babel-preset-react-app/webpack-overrides'
),
// @remove-on-eject-begin
babelrc: false,
configFile: false,
Expand Down
5 changes: 4 additions & 1 deletion packages/react-scripts/config/webpack.config.prod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,8 +272,11 @@ module.exports = {
// We need to use our own loader until `babel-loader` supports
// customization
// https://github.com/babel/babel-loader/pull/687
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment needs deleted.

// We need to use our own loader until `babel-loader` supports
// customization
// https://github.com/babel/babel-loader/pull/687

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Removed in 232e892.

loader: require.resolve('babel-preset-react-app/loader'),
loader: require.resolve('babel-loader'),
options: {
customize: require.resolve(
'babel-preset-react-app/webpack-overrides'
),
// @remove-on-eject-begin
babelrc: false,
configFile: false,
Expand Down
2 changes: 1 addition & 1 deletion packages/react-scripts/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"babel-core": "7.0.0-bridge.0",
"babel-eslint": "9.0.0",
"babel-jest": "23.6.0",
"babel-loader": "8.0.2",
"babel-loader": "8.0.4",
"babel-plugin-named-asset-import": "^0.2.0",
"babel-preset-react-app": "^4.0.0",
"bfj": "6.1.1",
Expand Down