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

Better support monorepos by allowing users to opt into autoconfiguring "root" #8636

Closed
wants to merge 2 commits into from

Conversation

loganfsmyth
Copy link
Member

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change? N
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Definitely curious for feedback on this one.

The Problem

In Babel 7, we've limited the expected scope of config files a lot, which has caused some migration pains for users. Because the scope of .babelrc files is now limited to a single package, users are trying to transition to use babel.config.js files. The issue is that, by default, Babel expects that file to be in the working directory of the build, and that is commonly not the case for monorepo users.

Current Requirements

Currently, the workaround is that users have to manually pass the name, e.g.

// from ./packages/some-packages/
babel --config-file ../../babel.config.js src -d lib

which is painful, and similar steps would need to be taken for anything that runs in a per-package way. This means that monorepos that have repo-wide builds, like Babel's monorepo, work fine, but monorepos with per-package logic end up requiring lots of manual configuration. This is particularly painful for packages that are commonly used without any options (like @babel/register), or provide no way to easily pass options (like babel-jest).

This PR

This PR proposes to allow --root true or root: true (where before root was only a string, and defaulted to cwd), which would walk up the directory structure looking for a babel.config.js file, and set root to that directory, and would throw an error if no root was found.

What this would allow is for users to opt into more intelligent 'root' handling. Specifically, they would have to opt in because this option then requires the presence of a babel.config.js in your repo.

For cases like @babel/register and babel-jest, we could consider exposing extra entrypoints to handle this in an automated way, for example @babel/register/auto-root could be exposed as an entry point for users who want to use it in the context of a monorepo. And babel-jest can expose a babel-jest/auto-root that does module.exports = require(".").createTransformer({ root: true });, so users would opt into monorepo-global configs by using these special entry points.

Questions

One of the benefits of dropping the unbounded-searching behavior we had for .babelrc files in Babel 6 is that it meant that it was easy to accidentally pick up a configuration that you didn't mean to apply. babel.config.js files are specifically loaded from a single location to avoid issues like this. This PR requires an opt-in and throws loudly when a babel.config.js isn't found to try to avoid any concerns about this kind off thing. If we opted into searching as the default, it's also not at all clear what should happen when the file isn't found. Do we use the working directory as root? That would mean that the addition of a babel.config.js into the monorepo root would change the root value and potentially change what .babelrc files are considered valid, since root is the default value for babelrcRoots. To me that seems like it would be a pretty bad experience for users. Changing defaults now would also likely be a breaking change.

What do folks think about using alternative module entry points for this? It still requires monorepo users to do additional steps, but at least it's something that we could clearly document and recommend. It's also infinitely better than expecting people to do all of this themselves in any monorepo project that they need to migrate.

What do you think about overloading root here? I think root: false at least makes sense as "there is no root, so don't load config files", but root: true meaning to automatically search for the root isn't necessarily super discoverable. I also considered monorepo: true or something, but I wasn't convinced that it was any more clear, and it meant that the behavior of root then depends tangentially on another flag that users may or may not expect.

@loganfsmyth loganfsmyth added PR: New Feature 🚀 A type of pull request used for our changelog categories i: enhancement pkg: core labels Sep 6, 2018
@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9029/

@nicolo-ribaudo
Copy link
Member

I think that we can be more conservative: instead of having an option to enable babel.config.js searching in the file tree, we should have an option specifically for monorepos: it should just be an alias for root: ".", root: "../..".
First Babel should search the babel.config.js file using the current algorighm (root: "."). If it can't be found, Babel should try to resolve babel.config.js using root: "../..".
If you aren't using a "standard" monorepo but ou have a package deeply nested in your folders structure, you would still need to explicitly set the root option.
This would also prevent users from having a shared config files in their home directory, like some had in v6.

What do you think about overloading root here? I think root: false at least makes sense as "there is no root, so don't load config files", but root: true meaning to automatically search for the root isn't necessarily super discoverable. I also considered monorepo: true or something, but I wasn't convinced that it was any more clear, and it meant that the behavior of root then depends tangentially on another flag that users may or may not expect.

To me root: true looks more like "this is the root config" rather then "automatically find the config". I think that we should call it autoRoot: true or (only if this option is actually for monorepos, not for a general way of searching the babel.config.js file) monorepo: true

@loganfsmyth
Copy link
Member Author

it should just be an alias for root: ".", root: "../..". If you aren't using a "standard" monorepo but ou have a package deeply nested in your folders structure, you would still need to explicitly set the root option.

We could limit it like this, I'm just not sure I seeing a compelling reason to do so.

To me root: true looks more like "this is the root config" rather then "automatically find the config". I think that we should call it autoRoot: true or (only if this option is actually for monorepos, not for a general way of searching the babel.config.js file) monorepo: true

That was my concern with root: true too. I was mostly concerned that the combination of 2 separate options would make it easier to follow. Say a user does specifies a value for root and also passes autoRoot: true, I guess we'd want to error. That gets more problematic when used with other tools, because you might have a wrapper tool that does { root: ".", ...opts }, and if the user passes { autoRoot: true } they might not realize that the wrapper is already setting root. We just had a similar issue with the sourceMap vs sourceMaps option in babel/babel-loader#658.

That said, maybe that's not the end of the world since setting root probably won't be very common anyway.

@FL0RIANMEYER
Copy link

FL0RIANMEYER commented Sep 9, 2018

Can someone explain the lookup behavior?

For me it seems, that the first config in the path between module and cwd or the first Package.json file is picked.
image

Wouldn't it be better to search for the absolute root and merge found files? Nearby configurations override more distant configs.

image

Config in Projects:

module.exports = {
  'presets': ['@babel/env'],
  'retainLines': true,
};

Config in ProjectA:

module.exports = {
  'presets': ['@babel/env'],
  'retainLines': true,
  'plugins': [
      '@babel/plugin-proposal-class-properties',
  ],
};

Config in PackageA:

module.exports = {
  'presets': ['@babel/env', '@babel/react'],
  'plugins': [
      '@babel/plugin-proposal-object-rest-spread',
  ],
};

Merged Config for ModuleA:

module.exports = {
  'presets': ['@babel/env', '@babel/react'],
  'retainLines': true,
  'plugins': [
     '@babel/plugin-proposal-class-properties',
     '@babel/plugin-proposal-object-rest-spread',
  ],
};

With opt-in setting like rootConfig to break lookup.

module.exports = {
  'rootConfig': true,
};

And loader Config like mergeConfig with default true to disallow merging and break at first config.

{
    test: /\.js$/,
    loader: 'babel-loader',
    exclude: /node_modules/,
    options: {
        mergeConfig: false,
    },
},

@loganfsmyth
Copy link
Member Author

@FL0RIANMEYER

I'd recommend reading over http://babeljs.io/docs/en/config-files if you haven't. It goes into some of the details about how config files work.

Can someone explain the lookup behavior?

Babel has a "root" option, which defaults to the value of cwd, which in turn defaults to process.cwd(). This root value is the directory that is considered the "root" of your project, and is the place where Babel loads babel.config.js files from, among other things.

Wouldn't it be better to search for the absolute root and merge found files? Nearby configurations override more distant configs.

Generally walking upward and merging is something that I've not found particularly intuitive. If you want behavior like this, you can use .babelrc files that each "extends" a config file from the parent directory, e.g. { extends: "../.babelrc" }, but I think merging as the default is more confusing that having it be opt-in.

Merging for babel.config.js also risks being very confusing because the intention is that Babel only has a single config file. Otherwise, it's unclear what happens if someone does configFile: "babel.config.js". Should that also search upward?

Copy link
Member

@xtuc xtuc left a comment

Choose a reason for hiding this comment

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

I would prefer this path for the monorepo support.

@loganfsmyth
Copy link
Member Author

@xtuc Would you prefer this or #8660 ? I've heavily leaning toward that one now that I've updated it to rootMode instead of autoRoot.

@loganfsmyth
Copy link
Member Author

Alright, closing in favor of #8660.

@loganfsmyth loganfsmyth deleted the config-root-search branch September 12, 2018 02:36
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
i: enhancement outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: core PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants