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 automatically resolving 'root' with rootMode: "upward". #8660

Merged
merged 1 commit into from Sep 15, 2018

Conversation

loganfsmyth
Copy link
Member

@loganfsmyth loganfsmyth commented Sep 10, 2018

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

This is an alternative to #8636 that is a bit simpler. It introduces a separate rootMode: "root" | "upward" | "upward-optional" option. From the docs PR:

  • "root" - Passes the "root" value through as unchanged.
  • "upward" - Walks upward from the "root" directory, looking
    for a directory containing a babel.config.js
    file, and throws an error if a babel.config.js
    is not found.
  • "upward-optional" - Walk upward from the "root" directory,
    looking for a directory containing a babel.config.js
    file, and falls back to "root" if a babel.config.js
    is not found.

Otherwise, this is very similar to #8636 and has all of the same requirements of it being an opt-in feature.

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

babel-bot commented Sep 10, 2018

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

@babel-bot
Copy link
Collaborator

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

throw Object.assign(
(new Error(
`Babel was run with autoRoot:true but a root babel.config.js could not ` +
`be found when searching from "${rootDir}"`,
Copy link
Member

Choose a reason for hiding this comment

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

To help the user troubleshoot its setup, we could log every dir?

[...] when searching in /babel/a, /babel, /, ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I tweaked the text to mention the upward search, do you think that's enough? I worry that including all the directories would make the error harder to read.

@xtuc
Copy link
Member

xtuc commented Sep 10, 2018

Note for members: need documentation

@@ -24,6 +24,30 @@ const BABELRC_FILENAME = ".babelrc";
const BABELRC_JS_FILENAME = ".babelrc.js";
const BABELIGNORE_FILENAME = ".babelignore";

export function findClosestConfig(rootDir: string): string {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of Closes, which is not very accurate, could we name this: findConfigUpwards, findParentConfig, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Those are good suggestions, thanks. I'll go with findConfigUpwards.

@loganfsmyth loganfsmyth changed the title Better support monorepos by allowing users to opt into automatically resolving 'root' with autoRoot: true. Better support monorepos by allowing users to opt into automatically resolving 'root' with rootMode: "upward". Sep 11, 2018

throw Object.assign(
(new Error(
`Babel was run with rootMode:"${rootMode}" but a root could not ` +
Copy link
Member

Choose a reason for hiding this comment

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

Nit: No need to interpolate, you can just write rootMode: "upward".

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I moved the code around and forgot to update this snippet.

@loganfsmyth
Copy link
Member Author

Docs posted in babel/website#1822

packages/babel-core/src/config/partial.js Show resolved Hide resolved
);
}
default:
throw new Error(`Assertion failure - unknown rootMode value`);
Copy link
Member

Choose a reason for hiding this comment

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

We could also link to the website to learn more.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't generally do that with assertions. This is mostly here to satisfy Flow. The user should never see this and if we got here, it means Flow didn't catch a type error somewhere, since it would mean we got a RootMode type that didn't match the type definition.

Copy link
Member

Choose a reason for hiding this comment

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

Got you, there's a config validation before that anyway (assertRootMode).

Just FYI I used to use https://github.com/xtuc/mamacro#examples for only assertion, they are quite close to C ones.

packages/babel-cli/src/babel/options.js Show resolved Hide resolved

const nextDir = path.dirname(dirname);
if (dirname === nextDir) break;
dirname = nextDir;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could set a max limit, we can be almost sure to not find any config after 30 iterations. That will avoid tranversing the whole system in some special setups.

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 do this type of loop in several places during config resolution. I'm not sure it's worth it to over-optimize up front, especially since you have to opt into the upward-optional mode for it to have been wasted time.

Copy link
Member

Choose a reason for hiding this comment

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

Ok that make sense, i'm just worried about some undebuggable issues that it could cause.

Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

Can we change the PR description just for consistency? Awesome work!

@loganfsmyth
Copy link
Member Author

@hzoo Good call, done!

@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

6 participants