Switch back to babel-loader #5143
Conversation
iansu
force-pushed
the
babel-loader-customization
branch
from
569a3a8
to
64e3540
Compare
|
pls remove yarn lock |
|
Oops, I left it out initially but it must have got added back when I rebased. |
iansu
force-pushed
the
babel-loader-customization
branch
from
64e3540
to
c92efca
Compare
| return { | ||
| ...config.options, | ||
| caller: { | ||
| name: 'babel-preset-react-app', |
There was a problem hiding this comment.
We should probably spread caller if present and keep existing name. Just add the craInvalidationToken.
| // file basis. | ||
| if (source.indexOf('.macro') !== -1 || source.indexOf('/macro') !== -1) { | ||
| return { | ||
| ...config.options, |
There was a problem hiding this comment.
Rest spread isn't available in lower versions of Node 8, please switch this to Object.assign.
|
|
||
| module.exports = loader.custom(() => overrides); | ||
| module.exports = function() { |
There was a problem hiding this comment.
Let's name this file something else, e.g. overrides or loader-customization.
There was a problem hiding this comment.
Maybe babel-preset-react-app/webpack-overrides
There was a problem hiding this comment.
Internally, Babel uses overrides so I like that -- this is specific to the webpack loader though.
webpack-overrides is good, but loader-overrides would work too.
|
Good catch. |
| // 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 |
There was a problem hiding this comment.
Nit: update comment to reflect the move from modifying the name to adding craInvalidationToken
| // to the caller, a valid option accepted by Babel, to compose a one-time | ||
| // cacheIdentifier for the file. We cannot tune the loader options on a per | ||
| // file basis. | ||
| if (source.indexOf('.macro') !== -1 || source.indexOf('/macro') !== -1) { |
There was a problem hiding this comment.
Nit: you can do one lookup for just macro.
Then check the character at previous index.
There was a problem hiding this comment.
To make that work we'd have to save the index into a variable. Is that tradeoff worth it?
There was a problem hiding this comment.
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!)
There was a problem hiding this comment.
I assume regex would be way slower?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I assume regex would be way slower?
It depends. Regex can actually be faster.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@Timer I would assume regex would be slower too but then I started to second guess that.
Maybe doing both checks in one go with a regex would be fastest?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // to the caller, a valid option accepted by Babel, to compose a one-time | ||
| // cacheIdentifier for the file. We cannot tune the loader options on a per | ||
| // file basis. | ||
| if (/[./]macro/.test(source)) { |
There was a problem hiding this comment.
Now is this going to be slow because I'm creating a new regex each time this function is called? Should it be declared once outside of this function?
There was a problem hiding this comment.
Yeah, externalize it and feed it through new RegExp.
| @@ -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 | |||
There was a problem hiding this comment.
This comment needs deleted.
// We need to use our own loader until `babel-loader` supports
// customization
// https://github.com/babel/babel-loader/pull/687
* Switch back to babel-loader * Preserve existing caller options. Use Object.assign instead of object spread. * Updated filename in package.json * Update comment about cache identifier * Update macro check to use a regex * Move macro check regex out of function
Closes #5122