Extract config file resolution from OptionsManager by jamestalmage · Pull Request #3564 · babel/babel

jamestalmage · 2016-07-04T04:40:17Z

Currently OptionsManager encompasses a number of tasks:

Climbing up the directory tree for the correct .babelrc / .babelignore
Following the extends option to find base configurations which are being extended.
Combining all applicable config files into a single set of options for use in the transform.

This PR separates 1 and 2 from 3.

build-config-chain.js now does all the file system searching, and builds a list of configs that should be merged.

OptionsManager#mergeOptions no longer calls itself recursively.

The entire operation is essentially reduced to:

buildChain({filename}).forEach(config => optionManager.mergeOptions(config));

Great care was taken to ensure that build-config-chain.js builds the list of options in the same order as they would have been merged in OptionsManager previously.

build-config-chain.js - has a very minimal dependency list, and building the chain does not require loading up any of the rest of babel-core, or any plugins. I believe this could be used to greatly reduce performance hits caused by require-ing lots of modules that may never be used. Additional refactoring to babel-register could prevent loading up babel-core unless it is actually needed.

avajs/ava#945 improves performance of AVA test suites some 30%-50% by precompiling in the main process, and avoiding babel-register for child processes. However, it does not work well for dynamic requires.

My eventual goal is to reduce the penalty for babel-register to near-zero when it isn't needed (because AVA successfully precompiled everything in the main process), but still have it in place in case of dynamic requires.

I think this has practical use outside of the AVA use cases described above. For one, I think it just provides a better separation of concerns. Secondly, I think it would be fairly easy to inspect the chain and determine if more aggressive caching was possible.

`build-config-chain.js` creates an array of options which will be passed to `OptionsManager#mergeOptions`. The advantage of separating it out is that `build-config-chain` has a very minimal dependency list. The eventual intent is to allow the require hook to lazy load only when required. In other words, if no required files ever match the patterns `ignore` / `only` patterns, the bulk of babel-core, and the associated plugins, will never be loaded.

codecov-io · 2016-07-04T04:46:40Z

Current coverage is 87.96%

Merging #3564 into master will increase coverage by 0.05%

@@             master      #3564   diff @@
==========================================
  Files           194        195     +1   
  Lines          9640       9790   +150   
  Methods        1101       1118    +17   
  Messages          0          0          
  Branches       2204       2252    +48   
==========================================
+ Hits           8475       8612   +137   
- Misses         1165       1178    +13   
  Partials          0          0

Powered by Codecov. Last updated by c561312...62ad67e

jamestalmage · 2016-07-07T20:10:30Z

Any feedback on this from the Babel team?

hzoo · 2016-07-07T20:13:44Z

So you're not just left here - Sorry, haven't been able to look at this (haven't really looked options in much detail anyway)

loganfsmyth · 2016-07-19T02:52:43Z

I'm totally for this as long as we're sure the config ordering will still be identical. The amount of state tracked inside the OptionsManager has been a little crazy to follow.

So I understand your long-term goal, you'd like to be confident in the options used for a file without having to load babel-core itself? Are you planning another step once this lands to expose an API to do that?

I'm all for all of this, I wish Babel had better logic for caching builds safely, but at least this is a step in a better direction.

jamestalmage · 2016-07-19T14:50:58Z

So I understand your long-term goal, you'd like to be confident in the options used for a file without having to load babel-core itself? Are you planning another step once this lands to expose an API to do that?

Yes and Yes. I am happy to keep plugging away at that goal. I would prefer to do it in smaller PR's so the core team can comment on the progression and help steer the implementation details, but I can just continue to pile on to this PR if you want proven benefits before merging.

jamestalmage · 2016-07-19T14:57:59Z

I'm totally for this as long as we're sure the config ordering will still be identical.

I spent a lot of time ensuring that, and I really do think I got it right. An added benefit of this change is that config ordering is now testable/tested.

loganfsmyth · 2016-07-20T01:24:47Z

Let's do this

hzoo · 2016-07-20T01:27:40Z

Thanks for all the work + tests @jamestalmage! 🎉

loganfsmyth · 2016-07-20T01:30:57Z

I think the benefits for overall readability and API clarity are worth it in their own right, but also happy to iterate on public APIs for config access too. Thanks for the help!

jamestalmage · 2016-07-20T04:48:58Z

Awesome! Thanks!

montogeek · 2017-03-14T15:53:31Z

"extended" plugin refers to which plugin?

loganfsmyth · 2017-03-14T17:24:45Z

@montogeek Not sure I understand? Babel lets you use extends in your config to do

{
  extends: "../otherDir/.babelrc",
}

to pull in config from a different config file.

montogeek · 2017-03-14T17:30:02Z

@loganfsmyth Thanks for the reply, I missed this option, I asked because found some errors when migrating tests to Jest

jamestalmage added 2 commits July 2, 2016 03:04

add tests for build-config-chain

62ad67e

jamestalmage mentioned this pull request Jul 4, 2016

Transpile in the main process avajs/ava#945

Closed

jamestalmage added 2 commits July 7, 2016 16:36

add tests for env options

8e84196

git commit add test for package.json config

0a9cbe6

loganfsmyth merged commit 210c3f7 into babel:master Jul 20, 2016

hzoo added this to the Next Patch milestone Jul 20, 2016

hzoo added the PR: Polish 💅 A type of pull request used for our changelog categories label Jul 20, 2016

jamestalmage deleted the lazy-require-hook branch July 20, 2016 04:37

danez modified the milestone: Next Patch Jul 20, 2016

gaearon mentioned this pull request Sep 23, 2016

env is being ignored in preset #4539

Closed

loganfsmyth mentioned this pull request Feb 24, 2017

RFC: Move caching from babel-(loader|register) to babel-core and plugin framework #5372

Open

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019

Uh oh!

Conversation

jamestalmage commented Jul 4, 2016

Uh oh!

codecov-io commented Jul 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 87.96%

Uh oh!

jamestalmage commented Jul 7, 2016

Uh oh!

hzoo commented Jul 7, 2016

Uh oh!

loganfsmyth commented Jul 19, 2016

Uh oh!

jamestalmage commented Jul 19, 2016

Uh oh!

jamestalmage commented Jul 19, 2016

Uh oh!

loganfsmyth commented Jul 20, 2016

Uh oh!

hzoo commented Jul 20, 2016

Uh oh!

loganfsmyth commented Jul 20, 2016

Uh oh!

jamestalmage commented Jul 20, 2016

Uh oh!

montogeek commented Mar 14, 2017

Uh oh!

loganfsmyth commented Mar 14, 2017

Uh oh!

montogeek commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Jul 4, 2016 •

edited

Loading

montogeek commented Mar 14, 2017 •

edited

Loading