Skip to content
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

Merged
merged 6 commits into from
Feb 9, 2018

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Jan 15, 2018

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:

  • package.json engines.node field
  • package.json engines.browsers field
  • package.json browserslist field
  • browserslist or .browserslistrc files
  • .babelrc or .babelrc.js files with babel-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:

  • Fix tests
  • Add more tests
  • Test performance
  • Test in real world apps

cc. @gaearon @thejameskyle

(asset.package && asset.package.babel) ||
(await asset.getConfig(['.babelrc', '.babelrc.js']));
if (babelrc) {
config = Object.assign({}, babelrc, {
Copy link
Member Author

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?

Copy link
Member

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

Copy link

Choose a reason for hiding this comment

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

Agreed

Copy link
Member Author

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?

Copy link
Member

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


// 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/')) {
Copy link
Member Author

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

Copy link
Contributor

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

Copy link

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?

Copy link
Member Author

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.

Copy link

Choose a reason for hiding this comment

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

Thanks for explaining!

// 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);
Copy link
Member Author

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

@yavorsky
Copy link

yavorsky commented Jan 15, 2018

BTW, We already have a PR for preset-env that adds engines from the package.json support: babel/babel-preset-env#114

It presumes engines.node, but we could extend it to consider engines.browsers field also.

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.
Then, you can just reuse logic provided in preset-env, which already can search browserslist in package.json and browserslist config files.

@parcel-bundler parcel-bundler deleted a comment from aaqibkhaan Jan 15, 2018
let sourcePlugins = new Set(sourceEnv.map(p => p[0]));
targetEnv = targetEnv.filter(plugin => {
return !sourcePlugins.has(plugin[0]);
});
Copy link

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.

Copy link
Member Author

@devongovett devongovett Jan 15, 2018

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link

@gaearon gaearon Jan 15, 2018

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.

Copy link

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.

Copy link
Contributor

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".

Copy link

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.

Copy link
Contributor

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.

Copy link

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?

@jaydenseric
Copy link

@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 package.json engines in .babelrc.js.

@codecov-io
Copy link

codecov-io commented Jan 29, 2018

Codecov Report

Merging #559 into master will decrease coverage by 0.53%.
The diff coverage is 92.42%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/Asset.js 97.72% <100%> (+0.05%) ⬆️
src/assets/JSAsset.js 93.58% <100%> (ø) ⬆️
src/transforms/babel.js 91.56% <90.66%> (+1.09%) ⬆️
src/utils/getTargetEngines.js 94.11% <94.11%> (ø)
src/SourceMap.js 80.59% <0%> (-11.95%) ⬇️
src/assets/CSSAsset.js 82.35% <0%> (-7.36%) ⬇️
src/visitors/fs.js 88.63% <0%> (+1.13%) ⬆️
src/Logger.js 96.96% <0%> (+1.51%) ⬆️
src/WorkerFarm.js 93.22% <0%> (+1.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3fcfa0...337e1b1. Read the comment docs.

@devongovett
Copy link
Member Author

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 es2015 preset, modules will only be actually be compiled to the app's defined browser target. I think this is fairly safe to do, but let me know if there is a case where this wouldn't work.

The locally installed version of babel-core specified by the module is used to do the compilation. This should allow for future babel updates while maintaining compatibility with existing modules that don't update.

@devongovett devongovett changed the title [WIP] Automatically transpile dependencies with babel-preset-env Automatically transpile dependencies with babel-preset-env Jan 29, 2018
@devongovett
Copy link
Member Author

Check out the tests to see some examples of how it works.

return targets;
}

function getMinSemver(version) {

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.

Copy link
Member Author

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.

@devongovett
Copy link
Member Author

Ok removed bundlerOptions for now while we debate future options (e.g. source field etc.) in #13. Going to merge this as is. This disables .babelrc for node_modules, and enables automatic babel-preset-env compilation.

@piscisaureus
Copy link

My project uses typescript, which already knows how to target various ecmascript versions.
There is no need to have babel go over the javascript that this produced by the typescript compiler.
Is there a way to turn this feature off?

piscisaureus added a commit to propelml/propel that referenced this pull request Mar 4, 2018
piscisaureus added a commit to propelml/propel that referenced this pull request Mar 4, 2018
piscisaureus added a commit to propelml/propel that referenced this pull request Mar 4, 2018
satazor added a commit to moxystudio/react-with-moxy that referenced this pull request May 5, 2018
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.
@RPDeshaies
Copy link

Following @piscisaureus ' comment, is there a way to use the Typescript compiler to bundle node modules instead since it's already there ?

@fathyb fathyb mentioned this pull request Sep 29, 2018
3 tasks
DeMoorJasper pushed a commit to parcel-bundler/website that referenced this pull request Nov 14, 2018
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.