-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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.
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
Automatically transpile dependencies with babel-preset-env #559
Conversation
src/transforms/babel.js
Outdated
(asset.package && asset.package.babel) || | ||
(await asset.getConfig(['.babelrc', '.babelrc.js'])); | ||
if (babelrc) { | ||
config = Object.assign({}, babelrc, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we merge the babelrc with the generated env config or just override if you specify a babelrc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that we should allow people to specify Babel transforms for node_modules
, I think it should be exclusively babel-preset-env
and nothing else.
Otherwise we're going to end up with all these things in the ecosystem which tell you that you need to be compiling their package in a certain way. Package authors will either say:
- You need to add this Babel plugin in order to use this package
- You cannot use these Babel plugins in this way or it will break this package
If we want to change this restriction in the future, it's easier to loosen up than to make more strict. But I think this is the right way to go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thejameskyle see check above. This only happens in the top-level app, not node_modules. In that case you could have a babelrc that we need to respect, as well as a a browserslist file which we use to enable the env transforms needed. In that case do we merge, or only use the babelrc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of adding things to Babel configs without people actually specifying them, it's something that really annoys me with Jest
src/transforms/babel.js
Outdated
|
||
// Do a diff of the returned plugins. We only need to process the remaining plugins to get to the app target. | ||
// If this is the app module, the source and target will be the same, so just compile everything. | ||
if (asset.name.includes('/node_modules/')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a nicer way to detect files that are in the app rather than a module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there might not be any, we've discussed this recently at babel and it seems there is no better solution to see if a file is node_module or not than to look at its path, and we went with this approach for this kind of detection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In CRA we're currently just processing anything that's not in application src
folder with -env
. Is that wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine if you know something about the file structure (that there is a src folder), but Parcel needs to work for all types of projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining!
src/transforms/babel.js
Outdated
// Load supported source engines (node and browser versions) for source module and target app, | ||
// and generate a babel-preset-env config for each. | ||
let sourceEngines = await getTargetEngines(asset); | ||
let targetEngines = await getTargetEngines(asset, asset.options.mainFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should cache this somehow so we don't recompute the target engines for every single file
BTW, We already have a PR for preset-env that adds It presumes Unfortunately, we've focused on the other things, but I can finish it in the near future and merge if it will be useful for your intention. |
src/transforms/babel.js
Outdated
let sourcePlugins = new Set(sourceEnv.map(p => p[0])); | ||
targetEnv = targetEnv.filter(plugin => { | ||
return !sourcePlugins.has(plugin[0]); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's any possible way to make a Babel preset handle this automatically. For example we'd like our ejected users to just have babel-preset-react-app/dependencies
for non-app code, and have that automatically do the diffing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah totally, I was thinking that too. The only piece of information it needs is the entry point to the bundler (asset.options.mainFile
above in parcel's case) in order to resolve the target app's browser list. If this information were somehow provided to babel plugins via bundlers it would be possible to do in a babel preset (or even in env itself). Maybe just resolving based on the process.cwd
would be good enough though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other thing we need in Parcel's case is to track the config files we load (e.g. package.json
, .browserslistrc
, etc.) so they can be added to the file watcher and cache correctly. That way when you change your browser support file, we can recompile files automatically. Would be cool if Babel presets could emit dependencies as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I we are open for this stuff at babel, just need to specify the requirements, how this would work etc. Dunno if anyone want to focus on that at the moment as we'd like to release stable babel@7 - we could always use an outside contribution for this though 😉
The problem is that babel at the moment has no notion of project, it operates on single files within a directory structure. And it comes with support for nested config files, overrides and stuff, which could complicate things.
But I guess what is really interesting here is a single "targets" field/config for a project, usually kept at the root of the project which is a first package.json NOT within a node_modules, it should be quite easy to locate it without much hassle and without introducing much complexity to the codebase.
Let's open an issue on babel boards to discuss this further, we'd want to have a short summary of the problem and proposed solution for it as a discussion starter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I guess what is really interesting here is a single "targets" field/config for a project, usually kept at the root of the project which is a first package.json NOT within a node_modules, it should be quite easy to locate it without much hassle and without introducing much complexity to the codebase.
Just to clarify, I don't think the targets are relevant here. Env preset already can (and does) determine target browsers desired by the app (through browserslist).
What seems missing from Babel's side is inferring a library's supported engines
and then using those as "minimal guaranteed" environment heuristic.
For example if my app specifies IE 11
as a target via browserslist
, we know ES6 needs to be compiled down. But if React says via engines
that it runs on Node >= 0.12
we know that it can't possibly include ES6 code. Therefore we don't have to run React through ES6 transforms. For this to work, we need presets to know they're dealing with React package (and what engines
it has in package.json
).
Does this make sense? Sorry if you said the same and I misunderstood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yet, if that dependency was transpiled prepublish, the likelihood is even higher - which is why the community best practice remains, despite a few outlier packages and the react-native ecosystem, to publish a "main" that's CJS ES5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gaearon
I have in mind pretty much exactly what you have said, regarding the history, recent practices, proposed approaches. Except for this whole "main"/"some_other_field" point.
I dont exactly disagree with you on that, I only have seen this from slightly different angle. I just often see people considering "main" as some kind of "stable" es5 version.
You envision this being an enhancement for consumers, letting package authors to chose what they want to support etc. I thought of this (not that it is any vision of mine, and I dont agree with your approach, just had different goal in mind while we've started discussing this) more of an "enhancement" provided by package authors (with different field in the package.json) for consumers. So I have considered more opt-in situation here, whereas your solution with main
makes it more obligatory to use tooling supporting node_modules
transpilation (babel-preset-env).
My original thought hasnt really contradicted yours that much. With your approach this makes it kinda "ok, lets ship whatever you want, whatever you find appropriate, we can take care of the rest". With my original approach it was more like "let's ship whatever you want as main, we cant strictly forbid that, we'll take care of the rest, BUT if you want to support older envs right out of the box please use main
for completely transpiled version of your code and some_other_field
as an enhancement for users who'd like to benefit from less transpilation".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it clicks, thanks so much for explaining! Yeah I can see how this could be valuable although indeed the shifting baseline is a bit confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just expect most libraries supporting at least all current LTS versions which in a sense might mean "overtranspiling" main
for a part of the user base that would like to target newer envs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a conclusion or a next chapter to this convo somewhere? I ran into the issue this morning and had a very similar idea. I wonder if we can borrow insight from browserify's approach of libraries explicitly defining their transforms in package.json and including them as devDeps so consumers don't need to worry about it. I imagine you're still agreeing on a baseline target to some degree, but this could let downstream compilers opt out of certain transforms. Anyway, wanted to get thoughts on that and see if this convo has developed into a best practise somewhere. Is it recommended to transpile your node_modules directory with preset-env now, both in dev and prod?
@yavorsky I had been waiting for that PR to merge for ages, looks like it did not get migrated to the mono-repo. It would be awesome if you could get the ball rolling again. I have been manually importing the node version from |
Codecov Report
@@ Coverage Diff @@
## master #559 +/- ##
==========================================
- Coverage 94.35% 93.81% -0.54%
==========================================
Files 64 65 +1
Lines 2036 2151 +115
==========================================
+ Hits 1921 2018 +97
- Misses 115 133 +18
Continue to review full report at Codecov.
|
Updated this PR to implement the strategy described in #13 (comment). It continues to work the same way as above, but in addition allows modules to specify that they want to use a specific babel config. This allows e.g. npm linked modules to be compiled correctly without a manual build step. To summarize, you can use the following config in package.json to enable babel on node_modules: {
"bundlerOptions": {
"babel": true
}
} In addition, the babel config is merged with the babel-preset-env generated config that takes into account the app's browser target. Unnecessary presets and plugins are removed, so even if a module specified the The locally installed version of |
Check out the tests to see some examples of how it works. |
return targets; | ||
} | ||
|
||
function getMinSemver(version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related: npm/node-semver#208. This is actually a pretty tricky thing to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I think what I have here might be good enough for what we're using it for though, which is only parsing the package.engines.node
field. It's unlikely to contain any super complex semver versions in there.
Compares the source and target browser support matrix, and automatically runs only the necessary babel plugins.
127d339
to
337e1b1
Compare
Ok removed |
My project uses typescript, which already knows how to target various ecmascript versions. |
The future is to run babel through every code, including node_modules. You may read more here: parcel-bundler/parcel#559 (comment) The `babel-loader` has a very good cache for sometime now, making compiling everything really fast. `create-react-app` will be doing the same in their next major version.
Following @piscisaureus ' comment, is there a way to use the Typescript compiler to bundle node modules instead since it's already there ? |
* Update description of node_modules handling Updates the Transforms page to be consistent with parcel-bundler/parcel#559 and parcel-bundler/parcel#1101. * Improve wording Configuration files outside of node_modules will never ever apply to things inside of node_modules
Potential fix for #13. This disables
.babelrc
inside node_modules, but provides a way to automatically compile dependencies to the target browsers of the app. It compares the browser support matrix for the source module and the target app, and automatically runs only the necessary babel plugins. This allows modules to be published to npm as ES2015 or higher, and automatically get transpiled only to the level that the app desires rather than all the way down to ES5.Modules can specify the browser and node versions that they were written to support via
package.json
,.browserslistrc
file, etc. Apps also specify their target browser/node version support the same way. Parcel calls babel-preset-env to generate a list of plugins for each, and does a diff to only include the necessary plugins to get from the source language version to the target.Full target location list:
engines.node
fieldengines.browsers
fieldbrowserslist
fieldbrowserslist
or.browserslistrc
files.babelrc
or.babelrc.js
files withbabel-preset-env
preset config (questionable)Currently the list of locations Parcel searches is the same for both modules and the target app. I'm not sure if this is correct or whether only
engines
should be used for modules. Will searching in browserslist or babelrc files cause issues if they are published to npm? Theoretically modules will already be pre-compiled to the level of the published babelrc/browserslist so it seems relatively safe to use that as the source support matrix along with engines etc. Slightly worried about babelrc format changing in the future though.Would be interested in feedback here!
To do:
cc. @gaearon @thejameskyle