Repository navigation
Updated to @babel/core@7, node 10.14.2 - #393
Conversation
|
@vweevers can you please review? |
|
Could you split the commits into manual changes and generated code? |
|
The change are only in the files in the https://github.com/nodejs/readable-stream/pull/393/files#diff-0d3594230a1e7b0f30b7a66fa975d607 Should be easy enough to review. |
| "@babel/plugin-transform-spread": "^7.2.0", | ||
| "@babel/plugin-transform-template-literals": "^7.2.0", | ||
| "@babel/polyfill": "^7.0.0", | ||
| "@babel/preset-env": "^7.2.0", |
There was a problem hiding this comment.
This babel setup is fragile. For example, @babel/preset-env is a dependency, but not used in the configuration. And @babel/plugin-transform-classes is used, but not listed in dependencies. I'm guessing the setup works because @babel/plugin-transform-classes is a dependency of @babel/preset-env and hoisted by npm.
IMO we have to make a choice: either we use the preset, or we don't and install+configure all plugins ourselves.
There was a problem hiding this comment.
Correction: @babel/preset-env is used, not by the main build script, but by the update-browser-errors npm script. Still, we're missing dependencies.
There was a problem hiding this comment.
Can you recommend some different way to do this? This is definitely my best, I feel using babel lead to fragile code anyway.
I've added the missing dependencies.
There was a problem hiding this comment.
Luckily, all the plugins that we use, are included in @babel/preset-env. So we can definitely simplify the setup. With some configuration to specify target environments (node and browser versions), which could reduce the number of code transformations. If we put that configuration in a .babelrc, it will be applied to both the build script and update-browser-errors.
We could even have two builds, one for node (with less code transformations, possibly better performance?) and one for browsers.
I can open an issue or PR (but might not get around to it until ~ a week from now).
There was a problem hiding this comment.
Would you be ok if we land this, ship it in a release, and then you send a PR to clean things up?
|
PTAL CI is green 🎉. |
| "babel-polyfill": "^6.9.1", | ||
| "babel-preset-env": "^1.7.0", | ||
| "bl": "^2.0.0", | ||
| "buffer": "^5.1.0", |
There was a problem hiding this comment.
Why was buffer needed before?
There was a problem hiding this comment.
No idea, likely a leftover.
2 participants
Fixes: #392