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

Add support for yarn and lerna monorepos. #3741

Merged
merged 29 commits into from
Feb 1, 2018
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
bf2d814
Support for multiple source paths via package.json srcPaths entry.
Nov 9, 2017
d9f5cf7
Clean up, use file matching (similar to original) in webpack configs …
bradfordlemley Jan 10, 2018
12e76ad
Remove package lock files.
bradfordlemley Jan 11, 2018
bd1124d
Fix for eject.
bradfordlemley Jan 11, 2018
eae24e9
Filter tests to run only tests in monorepo components included by the…
bradfordlemley Jan 12, 2018
f4b3a0d
Fix conditions for when to use template.
bradfordlemley Jan 12, 2018
739b59b
Fix eject.
bradfordlemley Jan 12, 2018
7227e5d
Remove code that is not needed w/ Jest 22.
bradfordlemley Jan 13, 2018
a93573d
Include all cra-comp tests in monorepo instead of trying to include o…
bradfordlemley Jan 13, 2018
8102907
Pin find-pkg version.
bradfordlemley Jan 14, 2018
9a1b92c
Hopefully fix jest test file matching on windows by removing first sl…
bradfordlemley Jan 14, 2018
f4f2882
E2E tests for monorepo.
bradfordlemley Jan 17, 2018
d8e0319
Run monorepo tests in CI.
bradfordlemley Jan 18, 2018
f96d04c
Fix and test post-eject build.
bradfordlemley Jan 18, 2018
565c1d7
Fix e2e test.
bradfordlemley Jan 18, 2018
75ae0c5
Fix test suite names in appveyor.
bradfordlemley Jan 18, 2018
fbc6bde
Include individual package dirs as srcPaths instead of top-level mono…
bradfordlemley Jan 18, 2018
21f0b00
Fix running tests after eject.
bradfordlemley Jan 18, 2018
9feda8b
Clean up test workspace, add some verifcations.
bradfordlemley Jan 19, 2018
8ab33c7
Cleanup.
bradfordlemley Jan 19, 2018
d52e904
Try to fix hang when running test on appveyor.
bradfordlemley Jan 20, 2018
79ac815
Don't write babel or lint config to package.json when ejecting.
bradfordlemley Jan 21, 2018
3e81144
Incorporate review comments.
bradfordlemley Jan 22, 2018
ec6f5a8
Fixes for windows.
bradfordlemley Jan 22, 2018
978674f
Fix for kitchensink mocha tests compiling.
bradfordlemley Jan 22, 2018
fc9b890
Add lerna monorepo test.
bradfordlemley Jan 23, 2018
4d0bc68
Fix lerna bootstrap on windows.
bradfordlemley Jan 23, 2018
9e7490c
Incorporate more review comments:
bradfordlemley Jan 25, 2018
50b4666
Add monorepo info to user guide.
bradfordlemley Jan 30, 2018
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
2 changes: 2 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@ script:
- 'if [ $TEST_SUITE = "installs" ]; then tasks/e2e-installs.sh; fi'
- 'if [ $TEST_SUITE = "kitchensink" ]; then tasks/e2e-kitchensink.sh; fi'
- 'if [ $TEST_SUITE = "old-node" ]; then tasks/e2e-old-node.sh; fi'
- 'if [ $TEST_SUITE = "monorepos" ]; then tasks/e2e-monorepos.sh; fi'
env:
matrix:
- TEST_SUITE=simple
- TEST_SUITE=installs
- TEST_SUITE=kitchensink
- TEST_SUITE=monorepos
matrix:
include:
- node_js: 0.10
Expand Down
5 changes: 4 additions & 1 deletion appveyor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@ environment:
test_suite: "installs"
- nodejs_version: 8
test_suite: "kitchensink"
- nodejs_version: 8
test_suite: "monorepos"
- nodejs_version: 6
test_suite: "simple"
- nodejs_version: 6
test_suite: "installs"
- nodejs_version: 6
test_suite: "kitchensink"

- nodejs_version: 6
test_suite: "monorepos"
cache:
- node_modules -> appveyor.cleanup-cache.txt
- packages\react-scripts\node_modules -> appveyor.cleanup-cache.txt
Expand Down
5 changes: 4 additions & 1 deletion packages/react-scripts/config/jest/babelTransform.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
// @remove-file-on-eject
// @remove-on-eject-begin
/**
* Copyright (c) 2014-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
// @remove-on-eject-end
'use strict';

const babelJest = require('babel-jest');

module.exports = babelJest.createTransformer({
presets: [require.resolve('babel-preset-react-app')],
// @remove-on-eject-begin
babelrc: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still create babelrc on eject? If we do I think it might be confusing that removing babel-preset-react-app from it won't actually remove it (because it's also inlined here and in webpack config).

Maybe we don't need to create babelrc on eject then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, babel config still gets written to package.json on eject. Yes, it could be confusing, and probably should not be there. Will remove it. Same thing for eslint config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FTR, removing babel config from package.json impacted mocha tests that run in e2e-kitchensink. Babelrc was already being created in the e2e script itself, but only for tests before eject. I opted to fix this by adding .babelrc to the fixture and removing the part of e2e script that was creating it.

// @remove-on-eject-end
});
64 changes: 53 additions & 11 deletions packages/react-scripts/config/paths.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
const path = require('path');
const fs = require('fs');
const url = require('url');
const findPkg = require('find-pkg');
const globby = require('globby');

// Make sure any symlinks in the project folder are resolved:
// https://github.com/facebookincubator/create-react-app/issues/637
Expand Down Expand Up @@ -63,6 +65,8 @@ module.exports = {
servedPath: getServedPath(resolveApp('package.json')),
};

let checkForMonorepo = true;

// @remove-on-eject-begin
const resolveOwn = relativePath => path.resolve(__dirname, '..', relativePath);

Expand All @@ -86,17 +90,13 @@ module.exports = {
ownNodeModules: resolveOwn('node_modules'), // This is empty on npm 3
};

const ownPackageJson = require('../package.json');
const reactScriptsPath = resolveApp(`node_modules/${ownPackageJson.name}`);
const reactScriptsLinked =
fs.existsSync(reactScriptsPath) &&
fs.lstatSync(reactScriptsPath).isSymbolicLink();

// config before publish: we're in ./packages/react-scripts/config/
if (
!reactScriptsLinked &&
__dirname.indexOf(path.join('packages', 'react-scripts', 'config')) !== -1
) {
// detect if template should be used, ie. when cwd is react-scripts itself
const useTemplate =
appDirectory === fs.realpathSync(path.join(__dirname, '..'));

checkForMonorepo = !useTemplate;

if (useTemplate) {
module.exports = {
dotenv: resolveOwn('template/.env'),
appPath: resolveApp('.'),
Expand All @@ -117,3 +117,45 @@ if (
};
}
// @remove-on-eject-end

module.exports.srcPaths = [module.exports.appSrc];

const findPkgs = (rootPath, globPatterns) => {
const globOpts = {
cwd: rootPath,
strict: true,
absolute: true,
};
return globPatterns
.reduce(
(pkgs, pattern) =>
pkgs.concat(globby.sync(path.join(pattern, 'package.json'), globOpts)),
[]
)
.map(f => path.dirname(path.normalize(f)));
};

const getMonorepoPkgPaths = () => {
const monoPkgPath = findPkg.sync(path.resolve(appDirectory, '..'));
if (monoPkgPath) {
// Yarn workspace
let pkgPatterns = require(monoPkgPath).workspaces;
Copy link
Contributor

Choose a reason for hiding this comment

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

yarn workspaces can coexist with lerna. That scenario needs to be handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For lerna+YW (useWorkspaces), lerna uses the same config as YW itself (package.workspaces), so I think that case is handled correctly here. For the case where lerna is not configured to useWorkspaces, but there is a packages.workspaces, this logic would choose the package.workspaces config, but I don't think that's really a valid use case. In summary, I think this logic is ok as-is, I'll add a comment to clear it up, give me more details if I'm missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

perfect, I just want to make sure this part. There could potentially be no use cases. But have to be careful and you can do this kind of things.

if (!pkgPatterns) {
// lerna
const lernaJson = path.resolve(path.dirname(monoPkgPath), 'lerna.json');
pkgPatterns = fs.existsSync(lernaJson) && require(lernaJson).packages;
}
const pkgPaths = findPkgs(path.dirname(monoPkgPath), pkgPatterns);
// check if app is part of monorepo
if (pkgPaths.indexOf(appDirectory) !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add this filter above findPkgs method?
This seems like we are iterating again.

return pkgPaths.filter(f => fs.realpathSync(f) !== appDirectory);
}
}
return [];
};

if (checkForMonorepo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we merge useTemplate and checkForMonoRepo together

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 useTemplate code is removed when ejecting. I'm sure there are other ways to do this logic, but not sure there is a clearly cleaner way ... if you have a way you think is clearly cleaner, send it in, I'll take it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

move the usetemplate assignment to the checkForMonoRepo
and use checkForMonoRepo for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

change the variable name as needed.

// if app is in a monorepo (lerna or yarn workspace), treat other packages in
// the monorepo as if they are app source
Array.prototype.push.apply(module.exports.srcPaths, getMonorepoPkgPaths());
}
12 changes: 7 additions & 5 deletions packages/react-scripts/config/webpack.config.dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,18 +145,19 @@ module.exports = {
options: {
formatter: eslintFormatter,
eslintPath: require.resolve('eslint'),
// @remove-on-eject-begin
baseConfig: {
extends: [require.resolve('eslint-config-react-app')],
},
// @remove-on-eject-begin
ignore: false,
useEslintrc: false,
// @remove-on-eject-end
},
loader: require.resolve('eslint-loader'),
},
],
include: paths.appSrc,
include: paths.srcPaths,
exclude: [/[/\\\\]node_modules[/\\\\]/],
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain why this has become necessary? Is this because non-CRA packages have their source code at the same level of nesting as their own node_modules and we want to skip those?

Copy link
Contributor Author

@bradfordlemley bradfordlemley Jan 21, 2018

Choose a reason for hiding this comment

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

Yep, since srcPaths includes comp1 root (or or top-level monorepo root, depending on decision above), both comp1/file.js and comp1/node_modules/somedep/somefile.js get included in the include filter, so we need to filter out node_modules. (Originally, only app/src was included, so there was no need to filter out app/node_modules.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Am not sure this is getting exclufed. I can still find node_modules inside the comp1 that is inside app3. Doesnt it completely exclude nodemodules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These webpack excludes just exclude them from getting transpiled and linted.

},
{
// "oneOf" will traverse all following loaders until one will
Expand All @@ -178,7 +179,8 @@ module.exports = {
// The preset includes JSX, Flow, and some ESnext features.
{
test: /\.(js|jsx|mjs)$/,
include: paths.appSrc,
include: paths.srcPaths,
exclude: [/[/\\\\]node_modules[/\\\\]/],
use: [
// This loader parallelizes code compilation, it is optional but
// improves compile time on larger projects
Expand All @@ -188,8 +190,8 @@ module.exports = {
options: {
// @remove-on-eject-begin
babelrc: false,
presets: [require.resolve('babel-preset-react-app')],
// @remove-on-eject-end
presets: [require.resolve('babel-preset-react-app')],
// This is a feature of `babel-loader` for webpack (not Babel itself).
// It enables caching results in ./node_modules/.cache/babel-loader/
// directory for faster rebuilds.
Expand Down Expand Up @@ -275,8 +277,8 @@ module.exports = {
options: {
// @remove-on-eject-begin
babelrc: false,
presets: [require.resolve('babel-preset-react-app')],
// @remove-on-eject-end
presets: [require.resolve('babel-preset-react-app')],
cacheDirectory: true,
},
},
Expand Down
12 changes: 7 additions & 5 deletions packages/react-scripts/config/webpack.config.prod.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,20 +152,21 @@ module.exports = {
options: {
formatter: eslintFormatter,
eslintPath: require.resolve('eslint'),
// @remove-on-eject-begin
// TODO: consider separate config for production,
// e.g. to enable no-console and no-debugger only in production.
baseConfig: {
extends: [require.resolve('eslint-config-react-app')],
},
// @remove-on-eject-begin
ignore: false,
useEslintrc: false,
// @remove-on-eject-end
},
loader: require.resolve('eslint-loader'),
},
],
include: paths.appSrc,
include: paths.srcPaths,
exclude: [/[/\\\\]node_modules[/\\\\]/],
},
{
// "oneOf" will traverse all following loaders until one will
Expand All @@ -186,7 +187,8 @@ module.exports = {
// The preset includes JSX, Flow, and some ESnext features.
{
test: /\.(js|jsx|mjs)$/,
include: paths.appSrc,
include: paths.srcPaths,
exclude: [/[/\\\\]node_modules[/\\\\]/],
use: [
// This loader parallelizes code compilation, it is optional but
// improves compile time on larger projects
Expand All @@ -196,8 +198,8 @@ module.exports = {
options: {
// @remove-on-eject-begin
babelrc: false,
presets: [require.resolve('babel-preset-react-app')],
// @remove-on-eject-end
presets: [require.resolve('babel-preset-react-app')],
compact: true,
highlightCode: true,
},
Expand Down Expand Up @@ -317,8 +319,8 @@ module.exports = {
options: {
// @remove-on-eject-begin
babelrc: false,
presets: [require.resolve('babel-preset-react-app')],
// @remove-on-eject-end
presets: [require.resolve('babel-preset-react-app')],
cacheDirectory: true,
},
},
Expand Down
3 changes: 3 additions & 0 deletions packages/react-scripts/fixtures/kitchensink/.babelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"presets": ["react-app"]
}
70 changes: 70 additions & 0 deletions packages/react-scripts/fixtures/monorepos/lerna-ws/bootstrap.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
'use strict';

const { execSync } = require('child_process');
const { resolve } = require('path');
const { existsSync } = require('fs');
const { platform } = require('os');
const crossSpawn = require('cross-spawn');

// function shouldUseYarn() {
// try {
Copy link
Contributor

Choose a reason for hiding this comment

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

What’s up with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is to force npm as the client (as also specified in lerna.json) so that lerna bootstrap --hoist works.
Sorry, forgot to clean that up. But, it's good you caught it because it raises a good question about the recommended approaches.

This test case is hoisting react which allows the shared components to pick it up without actually specifying it as a dependency -- which is handy, but probably bad and will be broken by #1752. I originally did that to avoid components having to specify react and match version with the app for fear that mismatched versions would cause issues. (Note: YW automatically hoists, so this is partially matching that behavior.)

But maybe this should be approached some other way. Ideas? Is it ideal for shared components to specify react as a peerDependency and match the app react version?

// execSync('yarnpkg --version', { stdio: 'ignore' });
// return true;
// } catch (e) {
// return false;
// }
// }

function shouldUseNpmConcurrently() {
try {
const versionString = execSync('npm --version');
const m = /^(\d+)[.]/.exec(versionString);
// NPM >= 5 support concurrent installs
return Number(m[1]) >= 5;
} catch (e) {
return false;
}
}

// const yarn = shouldUseYarn();
const yarn = false;
const windows = platform() === 'win32';
const lerna = resolve(
__dirname,
'node_modules',
'.bin',
windows ? 'lerna.cmd' : 'lerna'
);

if (!existsSync(lerna)) {
if (yarn) {
console.log('Cannot find lerna. Please run `yarn --check-files`.');
} else {
console.log(
'Cannot find lerna. Please remove `node_modules` and run `npm install`.'
);
}
process.exit(1);
}

let args = ['bootstrap'].concat(process.argv.slice(2));
if (yarn) {
// Yarn does not support concurrency
crossSpawn.sync(
lerna,
args.concat(['--npm-client=yarn', '--concurrency=1']),
{
stdio: 'inherit',
}
);
} else {
if (
// The Windows filesystem does not handle concurrency well
windows ||
// Only newer npm versions support concurrency
!shouldUseNpmConcurrently()
) {
args.push('--concurrency=1');
}
crossSpawn.sync(lerna, args, { stdio: 'inherit' });
}
7 changes: 7 additions & 0 deletions packages/react-scripts/fixtures/monorepos/lerna-ws/lerna.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"lerna": "2.6.0",
"packages": [
"packages/*"
],
"version": "0.0.0"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"devDependencies": {
"cross-spawn": "6.0.3",
"lerna": "2.6.0"
},
"scripts": {
"postinstall": "node bootstrap.js --hoist \"@(react|react-dom)\""
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import React from 'react';

const Comp1 = () => <div>Comp1</div>;

export default Comp1;
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import React from 'react';
import ReactDOM from 'react-dom';
import Comp1 from '.';

it('renders Comp1 without crashing', () => {
const div = document.createElement('div');
ReactDOM.render(<Comp1 />, div);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"name": "comp1",
"version": "1.0.0",
"main": "index.js",
"license": "MIT"
}
11 changes: 11 additions & 0 deletions packages/react-scripts/fixtures/monorepos/packages/comp2/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import React from 'react';

import Comp1 from 'comp1';

const Comp2 = () => (
<div>
Comp2, nested Comp1: <Comp1 />
</div>
);

export default Comp2;
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import React from 'react';
import ReactDOM from 'react-dom';
import Comp2 from '.';

it('renders Comp2 without crashing', () => {
const div = document.createElement('div');
ReactDOM.render(<Comp2 />, div);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "comp2",
"dependencies": {
"comp1": "^1.0.0"
},
"version": "1.0.0",
"main": "index.js",
"license": "MIT"
}
Loading