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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Set babel cwd to project rootDir for each project in multiproject mode. #7794

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0dc6df1
Add test demonstrating babel behavior difference individually vs. mul…
bradfordlemley Feb 3, 2019
51d008c
Configure babel cwd to be project rootDir
bradfordlemley Feb 3, 2019
4070442
Apply patch from https://github.com/facebook/jest/pull/7794#issuecomm…
bradfordlemley Feb 4, 2019
4d2ebc4
Add tests for a few other babel config methods
bradfordlemley Feb 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion babel.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.

module.exports = {
babelrcRoots: ['examples/*'],
plugins: [
['@babel/plugin-transform-modules-commonjs', {allowTopLevelThis: true}],
'@babel/plugin-transform-strict-mode',
Expand Down
19 changes: 19 additions & 0 deletions e2e/__tests__/multiProjectRunner.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,3 +497,22 @@ describe("doesn't bleed module file extensions resolution with multiple workers"
expect(stderr).toMatch('PASS project2/__tests__/project2.test.js');
});
});

describe('Babel config in individual project works in multi-project', () => {
it('Transpiles when running bar individually', () => {
const result = runJest('multi-project-babel/bar');
expect(result.stderr).toMatch('PASS ./bar.test.js');
expect(result.status).toBe(0);
});
it('Transpiles when running foo individually', () => {
const result = runJest('multi-project-babel/foo');
expect(result.stderr).toMatch('PASS ./foo.test.js');
expect(result.status).toBe(0);
});
it('Transpiles when running from multiproject', () => {
const result = runJest('multi-project-babel');
expect(result.stderr).toMatch('PASS bar/bar.test.js');
expect(result.stderr).toMatch('PASS foo/foo.test.js');
expect(result.status).toBe(0);
});
});
6 changes: 6 additions & 0 deletions e2e/global-setup/projects.jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,19 @@ module.exports = {
globalSetup: '<rootDir>/setup.js',
rootDir: path.resolve(__dirname, './project-1'),
testMatch: ['<rootDir>/**/*.test.js'],
transform: {
'\\.js$': require.resolve('./transform'),
},
transformIgnorePatterns: ['/node_modules/', '/packages/'],
},
{
displayName: 'project-2',
globalSetup: '<rootDir>/setup.js',
rootDir: path.resolve(__dirname, './project-2'),
testMatch: ['<rootDir>/**/*.test.js'],
transform: {
'\\.js$': require.resolve('./transform'),
},
transformIgnorePatterns: ['/node_modules/', '/packages/'],
},
],
Expand Down
8 changes: 8 additions & 0 deletions e2e/global-setup/transform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
const babelJest = require('babel-jest');

/* use higher-level babel.config.js */

module.exports = babelJest.createTransformer({
rootMode: 'upward',
});
4 changes: 4 additions & 0 deletions e2e/multi-project-babel/bar/.babelrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
module.exports = {
presets: ['@babel/preset-flow'],
};
2 changes: 2 additions & 0 deletions e2e/multi-project-babel/bar/bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
module.exports = (text: string) => text;
6 changes: 6 additions & 0 deletions e2e/multi-project-babel/bar/bar.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
const bar = require('./bar');

it('Bar transpiles', () => {
expect(bar('test')).toBe('test');
});
1 change: 1 addition & 0 deletions e2e/multi-project-babel/bar/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
4 changes: 4 additions & 0 deletions e2e/multi-project-babel/foo/.babelrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
module.exports = {
presets: ['@babel/preset-flow'],
};
2 changes: 2 additions & 0 deletions e2e/multi-project-babel/foo/foo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
module.exports = (text: string) => text;
6 changes: 6 additions & 0 deletions e2e/multi-project-babel/foo/foo.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
const foo = require('./foo');

it('Foo transpiles', () => {
expect(foo('test')).toBe('test');
});
1 change: 1 addition & 0 deletions e2e/multi-project-babel/foo/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
12 changes: 12 additions & 0 deletions e2e/multi-project-babel/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"jest": {
"projects": [
{
"rootDir": "<rootDir>/foo"
},
{
"rootDir": "<rootDir>/bar"
}
]
}
}
6 changes: 4 additions & 2 deletions packages/babel-jest/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

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?

Copy link
Member

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)

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link

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.

Copy link
Member

@SimenB SimenB Feb 5, 2019 •
edited
Loading

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

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.

const configPath = [
babelOptions.config || '',
babelOptions.babelrc || '',
Expand Down Expand Up @@ -85,7 +85,9 @@ const createTransformer = (options: any): Transformer => {
config: ProjectConfig,
transformOptions?: TransformOptions,
): string | TransformedSource {
const babelOptions = {...loadBabelConfig(config.cwd, filename).options};
const babelOptions = {
...loadBabelConfig(config.rootDir, filename).options,
};

if (transformOptions && transformOptions.instrument) {
babelOptions.auxiliaryCommentBefore = ' istanbul ignore next ';
Expand Down
16 changes: 16 additions & 0 deletions packages/jest-runtime/src/__tests__/test_root/.babelrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
module.exports = {
plugins: [
['@babel/plugin-transform-modules-commonjs', {allowTopLevelThis: true}],
'@babel/plugin-transform-strict-mode',
],
presets: [
[
'@babel/preset-env',
{
shippedProposals: true,
targets: {node: 6},
},
],
'@babel/preset-flow',
],
};