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

Resolve babel.config.js 'babelrcRoots' values relative to the config file. #8910

Merged
merged 1 commit into from Nov 5, 2018

Conversation

loganfsmyth
Copy link
Member

Q                       A
Fixed Issues? Fixes #8909
Patch: Bug Fix? Y
Major: Breaking Change? Y, if you were relying on the old incorrect behavior :(
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

We were accidentally resolving these paths relative to the working directory, even though they were supposed to be relative to the config file itself.

I think this qualifies as a bug fix, but it does technically break people if they were relying on the babel.config.js value being resolved relative to the working directory. That said, our examples make it relatively clear that it's expecting to be relative to the config file, so hopefully that isn't a real-world issue.

@loganfsmyth loganfsmyth added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Oct 20, 2018
@babel-bot
Copy link
Collaborator

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

@@ -168,6 +169,7 @@ export function buildRootChain(
babelrc = validatedFile.options.babelrc;
}
if (babelrcRoots === undefined) {
babelrcRootsDirectory = validatedFile.dirname;
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain me how this PR fixes the issue?
If I understand it correctly, this line is the only change: if babelrcRoots undefined, we use validatedFile.dirname instead of context.cwd, which is the default value of babelrcRootsDirectory (which is the same as the old behavior).
In the issue you said that it occurs when babelrcRoots is set to something, so I don't understand how this PR fixes it.

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 issue is that users may be doing configFile: "some/nested/path/config.js" for instance, or in cases like

project/
  babel.config.js
  packages/
    module/
      // run Babel with cwd here with `rootMode: "upward"`

and in both cases, previously we'd use cwd for the paths, rather than the location of the config file, so if your babelrcRoots value was

babelrcRoots: [
  ".",
]

the expectation is that the . is relative to the config file, but currently we are calculating it relative to the working directory.

Copy link
Member

Choose a reason for hiding this comment

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

babelrcRoots: [
   ".",
]

the expectation is that the . is relative to the config file, but currently we are calculating it relative to the working directory.

But doesn't this PR only changes the resolution behavior if babelrcRoots === undefined? If it is an array we still resolve relative to context.cwd, because we never go inside that if.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, the if (babelrcRoots === undefined) { check here is if it was undefined in the babel.transform arguments, not if it was undefined in the config file.

With the way things are set up now, we entirely ignore babelrcRoots in the config file if it is passed in the programmatic arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Oh ok 👍

@loganfsmyth loganfsmyth merged commit c6d2f45 into babel:master Nov 5, 2018
@loganfsmyth loganfsmyth deleted the babelrcroots-cwd branch November 5, 2018 16:51
@sibelius
Copy link

sibelius commented Nov 7, 2018

does this fixes a possible issue with jest multi runner on lerna when trying to compile all packages?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: core PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
6 participants