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
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9097/ |
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9063/ |
c4a4119
to
aeea6a4
Compare
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}"`, |
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.
To help the user troubleshoot its setup, we could log every dir?
[...] when searching in
/babel/a
,/babel
,/
, ...
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 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.
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 { |
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.
Instead of Closes
, which is not very accurate, could we name this: findConfigUpwards
, findParentConfig
, etc.
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.
Those are good suggestions, thanks. I'll go with findConfigUpwards
.
aeea6a4
to
827b30c
Compare
autoRoot: true
.rootMode: "upward"
.
|
||
throw Object.assign( | ||
(new Error( | ||
`Babel was run with rootMode:"${rootMode}" but a root could not ` + |
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.
Nit: No need to interpolate, you can just write rootMode: "upward"
.
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! I moved the code around and forgot to update this snippet.
827b30c
to
859bde0
Compare
Docs posted in babel/website#1822 |
); | ||
} | ||
default: | ||
throw new Error(`Assertion failure - unknown rootMode value`); |
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 could also link to the website to learn more.
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 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.
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.
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.
|
||
const nextDir = path.dirname(dirname); | ||
if (dirname === nextDir) break; | ||
dirname = nextDir; |
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.
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.
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 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.
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.
Ok that make sense, i'm just worried about some undebuggable issues that it could cause.
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.
Can we change the PR description just for consistency? Awesome work!
@hzoo Good call, done! |
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, lookingfor 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 ababel.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.