-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Set babel cwd to project rootDir for each project in multiproject mode. #7794
Conversation
Yeah, it is. But I think it's the correct change. The worry is for people having Not sure if we can do this before 25 either way, though. |
I'd consider it a bug and hope it gets into v24. I just migrated a project and was bitten by this. |
It worked for you in 23 using babel 7? |
It was Jest 23 and Babel 6, but I think the general problem is the same. I could run Jest as a "top-level" command in a workspace (e.g. here |
This is great and it would fix workarounds we do e.g. for React Native example. Feel free to apply this patch: diff --git a/examples/react-native/.babelrc.js b/examples/react-native/babel.config.js
similarity index 100%
rename from examples/react-native/.babelrc.js
rename to examples/react-native/babel.config.js
diff --git a/examples/react-native/jest.config.js b/examples/react-native/jest.config.js
index c31d72bce..8eb675e9b 100644
--- a/examples/react-native/jest.config.js
+++ b/examples/react-native/jest.config.js
@@ -1,6 +1,3 @@
module.exports = {
preset: 'react-native',
- transform: {
- '^.+\\.js$': require.resolve('react-native/jest/preprocessor.js'),
- },
}; |
@@ -52,7 +52,7 @@ const createTransformer = (options: any): Transformer => { | |||
configString: string, | |||
{config, instrument, rootDir}: {config: ProjectConfig} & CacheKeyOptions, | |||
): string { | |||
const babelOptions = loadBabelConfig(config.cwd, filename); | |||
const babelOptions = loadBabelConfig(config.rootDir, filename); |
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.
IMO making rootDir
a cwd
conflates the two concepts a bit. Can we specify root
or babelrcRoots
or something to Babel to achieve the same thing?
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.
babelrcRoots
is wrong. root
is probably correct (and what I suggested in #7438 (comment) - root: config.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.
Feedback being
I don't think I'd set
root
because it's really more dependent on user project layout, so it's something I'd probably expect the user to set. If that's getting easier, that's excellent too though.
But if we do require to user to set rootDir
, then I think we're good to go
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.
The question is whether to use babel's cwd
or root
.
I think cwd
is the correct option to use to achieve exactly the same behavior as running individually.
Other than being used as the default for root
, it is also used as the base for relative filenames. I'm not sure if there's anything else (resolving plugins?), but I think we're likely to run into subtle issues if we use root
instead of cwd
.
The feedback quoted in comment above was from @loganfsmyth, and it sounds like he is suggesting to use cwd
.
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.
Realistically, what I'd probably do is have the initial call to createTransformer()
call process.cwd()
in https://github.com/facebook/jest/blob/b16789230fd45056a7f2fa199bae06c7a1780deb/packages/babel-jest/src/index.js#L30 as
cwd: process.cwd(),
...options
That way user's doing things like:
module.exports = require("babel-jest").createTransformer({
plugins: ['@babel/preset-react'],
});
end up with the preset consistently resolved relative to the working directory at the time the transformer was created. The core goal of cwd
is to define the root directory to use when resolve relative paths passed into Babel's options. I'd then expect Jest to pass filename
as an absolute path, so that the cwd
doesn't matter.
I can see the argument for setting root
based on rootDir
and if that's the way Jest wants to go I don't think it's unreasonable, as long as it is documented as a breaking change for users upgrading to new versions. Ideally users should still be able to do
module.exports = require("babel-jest").createTransformer({
root: __dirname,
});
to override Jest's default root
value and set an explicit one in Babel's options.
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.
Realistically, what I'd probably do is have the initial call to
createTransformer()
callprocess.cwd()
in
We want this to work for MPR (https://jestjs.io/docs/en/configuration#projects-array-string-projectconfig) which might be in different repos (e.g. relay and react, see https://jestjs.io/blog/2017/05/06/jest-20-delightful-testing-multi-project-runner#multi-project-runner-configuration-overhaul)
So using process.cwd()
won't necessarily work (in that example it probably would as it's ran from a directory above)
Also, we want to be able to run Jest in any directory, and have it walk up until it finds a config
Ideally users should still be able to do
Yes, they'd still be able to override it if they want to.
One setup which unexpectedly (IMO) works is the e2e/multi-project-babel/prj-3 test:
IMO the following setup should work, but it doesn't:
One option to handle these would be to use jest I think Comments? Is |
@bradfordlemley Yeah, I think examples like that were what made me hesitant to recommend Jest set Babel's |
In my mind, that's what @cpojer @thymikee @rubennorte thoughts on what we should do here? |
Yeah, adding a new config item ( Also, since changing babel's So, maybe the best way forward is to document the issue with a work-around (e.g., make your own Transformer to set babel options?) and then fix the issue in 25 (along with cleaning up I don't think it's worth pursing this PR any further for 24 unless someone makes the call that it's ok to break certain project setups. |
what is the correct workaround until this lands? |
@sibelius We have docs for monorepos with an example for Jest here: https://babeljs.io/docs/en/config-files#jest |
@loganfsmyth does all my packages jest.config.js have to use this babel jest wrapper? module.exports = require('babel-jest').createTransformer({
rootMode: 'upward',
}); I have a monorepo like this https://github.com/entria/entria-fullstack/blob/master/packages/server/jest.config.js or do I need to specify this only in root jest.config.js? |
commit 0b5c1d0d2d8568e7e7e5cdab46690f9ccf4eaad1 Merge: 4d2ebc4 49a5eca Author: Simen Bekkhus <[email protected]> Date: Tue Apr 19 15:03:01 2022 +0200 Merge branch 'main' into multiprj-babel commit 4d2ebc4 Author: Bradford Lemley <[email protected]> Date: Mon Feb 4 10:29:31 2019 -0700 Add tests for a few other babel config methods commit 4070442 Author: Bradford Lemley <[email protected]> Date: Mon Feb 4 08:14:39 2019 -0700 Apply patch from jestjs#7794 (comment). commit 51d008c Author: Bradford Lemley <[email protected]> Date: Sun Feb 3 11:03:28 2019 -0700 Configure babel cwd to be project rootDir commit 0dc6df1 Author: Bradford Lemley <[email protected]> Date: Sun Feb 3 15:08:05 2019 -0700 Add test demonstrating babel behavior difference individually vs. multiprj
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Currently, babel transform behaves differently for projects when run from multi-project vs. running individually.
This is because babel's
cwd
was being set to the top-level project rootDir, instead of to the individual project's rootDirs.Closes #7359.
This could be considered a breaking change.
Test plan
Add test to demonstrate that projects with their own .babelrc.js configurations run successfully individually and when run with higher-level multi-project configuration.
(First commit adds test only to show current failure, further commits are fixes.)