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

Support running create-react-app inside a monorepo. #3967

Closed
wants to merge 8 commits into from
Closed

Support running create-react-app inside a monorepo. #3967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a479ce9
Use `resolve-from` to respect `node_modules` hierarchy in `createReac…
Nov 9, 2017
e2d2dec
Resolve `ownPath` in `init.js` using `resolve-from`
Nov 10, 2017
3dab136
Test running create-react-app inside monorepo.
bradfordlemley Feb 3, 2018
9670be6
Merge branch 'next' into mono-cra
bradfordlemley Feb 10, 2018
69d822c
Use common strategy to decide whether to use yarn or npm.
bradfordlemley Feb 10, 2018
40bb89c
Workaround yarn workspace bug not creating app/node_modules/.bin link…
bradfordlemley Feb 13, 2018
e188e1e
Remove yarn < 1.5 workaround.
bradfordlemley May 22, 2018
9d9b057
Revert "Remove yarn < 1.5 workaround."
bradfordlemley May 24, 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
22 changes: 9 additions & 13 deletions packages/create-react-app/createReactApp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ const url = require('url');
const hyperquest = require('hyperquest');
const envinfo = require('envinfo');
const os = require('os');
const resolveFrom = require('resolve-from');
const findMonorepo = require('react-dev-utils/workspaceUtils').findMonorepo;
const packageJson = require('./package.json');

Expand Down Expand Up @@ -323,15 +324,12 @@ function run(
);
})
.then(packageName => {
checkNodeVersion(packageName);
checkNodeVersion(root, packageName);
setCaretRangeForRuntimeDeps(packageName);

const scriptsPath = path.resolve(
process.cwd(),
'node_modules',
packageName,
'scripts',
'init.js'
const scriptsPath = resolveFrom(
root,
path.join(packageName, 'scripts', 'init.js')
);
const init = require(scriptsPath);
init(root, appName, verbose, originalDirectory, template);
Expand Down Expand Up @@ -511,12 +509,10 @@ function checkNpmVersion() {
};
}

function checkNodeVersion(packageName) {
const packageJsonPath = path.resolve(
process.cwd(),
'node_modules',
packageName,
'package.json'
function checkNodeVersion(root, packageName) {
const packageJsonPath = resolveFrom(
root,
path.join(packageName, 'package.json')
);
const packageJson = require(packageJsonPath);
if (!packageJson.engines || !packageJson.engines.node) {
Expand Down
1 change: 1 addition & 0 deletions packages/create-react-app/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"fs-extra": "^5.0.0",
"hyperquest": "^2.1.2",
"react-dev-utils": "^5.0.0",
"resolve-from": "^4.0.0",
"semver": "^5.0.3",
"tar-pack": "^3.4.0",
"tmp": "0.0.33",
Expand Down
5 changes: 5 additions & 0 deletions packages/react-scripts/fixtures/monorepos/cra-app/.template.dependencies.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"dependencies": {
"comp2": "^1.0.0"
}
}
24 changes: 24 additions & 0 deletions packages/react-scripts/fixtures/monorepos/cra-app/gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# See https://help.github.com/ignore-files/ for more about ignoring files.

# generated test output
testoutput.json

# dependencies
/node_modules

# testing
/coverage

# production
/build

# misc
.DS_Store
.env.local
.env.development.local
.env.test.local
.env.production.local

npm-debug.log*
yarn-debug.log*
yarn-error.log*
0 ...epos/packages/cra-app1/public/favicon.ico → ...ures/monorepos/cra-app/public/favicon.ico
View file
Open in desktop
File renamed without changes.
0 ...repos/packages/cra-app1/public/index.html → ...tures/monorepos/cra-app/public/index.html
View file
Open in desktop
File renamed without changes.
0 ...os/packages/cra-app1/public/manifest.json → ...es/monorepos/cra-app/public/manifest.json
View file
Open in desktop
File renamed without changes.
0 ...s/monorepos/packages/cra-app1/src/App.css → ...ts/fixtures/monorepos/cra-app/src/App.css
View file
Open in desktop
File renamed without changes.
0 ...es/monorepos/packages/cra-app1/src/App.js → ...pts/fixtures/monorepos/cra-app/src/App.js
View file
Open in desktop
File renamed without changes.
0 ...norepos/packages/cra-app1/src/App.test.js → ...ixtures/monorepos/cra-app/src/App.test.js
View file
Open in desktop
File renamed without changes.
0 ...monorepos/packages/cra-app1/src/index.css → .../fixtures/monorepos/cra-app/src/index.css
View file
Open in desktop
File renamed without changes.
0 .../monorepos/packages/cra-app1/src/index.js → ...s/fixtures/monorepos/cra-app/src/index.js
View file
Open in desktop
File renamed without changes.
0 .../monorepos/packages/cra-app1/src/logo.svg → ...s/fixtures/monorepos/cra-app/src/logo.svg
View file
Open in desktop
File renamed without changes
21 changes: 18 additions & 3 deletions packages/react-scripts/scripts/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ const execSync = require('child_process').execSync;
const spawn = require('react-dev-utils/crossSpawn');
const { defaultBrowsers } = require('react-dev-utils/browsersHelper');
const os = require('os');
const resolveFrom = require('resolve-from');
const paths = require('../config/paths');
const { findMonorepo } = require('react-dev-utils/workspaceUtils');

function isInGitRepository() {
try {
Expand Down Expand Up @@ -83,9 +86,12 @@ module.exports = function(
) {
const ownPackageName = require(path.join(__dirname, '..', 'package.json'))
.name;
const ownPath = path.join(appPath, 'node_modules', ownPackageName);
const ownPath = path.join(
resolveFrom(appPath, path.join(ownPackageName, 'package.json')),
'..'
);
const appPackage = require(path.join(appPath, 'package.json'));
const useYarn = fs.existsSync(path.join(appPath, 'yarn.lock'));
const useYarn = paths.useYarn;

// Copy over some of the devDependencies
appPackage.dependencies = appPackage.dependencies || {};
Expand Down Expand Up @@ -172,10 +178,19 @@ module.exports = function(
fs.unlinkSync(templateDependenciesPath);
}

// yarn ws not creating app/node_modules/.bin link to hoisted react-scripts
// -- workaround: install react-scripts again to create link
// -- bug in yarn 1.3.2, fix verified in nightly 1.4.1-20180211.2236 and 1.5.1
// TODO: remove this workaround when CRA enforces min yarn version
const rerunYarn = useYarn && findMonorepo(appPath).isAppIncluded;
if (rerunYarn) {
console.log('Detected app in yarn workspace, running install again');
}

// Install react and react-dom for backward compatibility with old CRA cli
// which doesn't install react and react-dom along with react-scripts
// or template is presetend (via --internal-testing-template)
if (!isReactInstalled(appPackage) || template) {
if (!isReactInstalled(appPackage) || template || rerunYarn) {
console.log(`Installing react and react-dom using ${command}...`);
console.log();

Expand Down
35 changes: 28 additions & 7 deletions tasks/e2e-monorepos.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,25 +110,46 @@ pushd "$temp_app_path"
cp -r "$root_path/packages/react-scripts/fixtures/monorepos/yarn-ws" .
cd "yarn-ws"
cp -r "$root_path/packages/react-scripts/fixtures/monorepos/packages" .
yarn

# Test cra-app1
cd packages/cra-app1
pushd packages/cra-app1
cp -r "$root_path/packages/react-scripts/fixtures/monorepos/cra-app/"* .
yarn
verifyBuild
yarn start --smoke-test
verifyTest
popd

# ******************************************************************************
# Test clean create-react-app inside workspace
# ******************************************************************************
pushd packages
npx create-react-app newapp
cd newapp
yarn build
yarn start --smoke-test
CI=true yarn test --watch=no
popd

# ******************************************************************************
# Test create-react-app w/ shared comps inside workspace
# ******************************************************************************
pushd packages
npx create-react-app --internal-testing-template="$root_path"/packages/react-scripts/fixtures/monorepos/cra-app cra-app2
cd cra-app2
verifyBuild
yarn start --smoke-test
verifyTest

# Test eject
# TODO: veriy tests can be run from other apps after eject
# -- will currently fail due to picking up ejected scripts/test.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, test.js in ejected app gets picked up (and fails because it's not a jest test) when running tests from other cra apps in the monorepo. One way to solve this would be to detect other cra apps in the monorepo (pkgs with dependency on react-scripts?) and either not treat them as cra sources, or use approot/src as their source path instead of approot. Another way could be by explicit cra source inclusion/exclusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain in more detail? I don't follow. Whose test is this? Why is it not a jest test? What directory/package layout does this happen with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mono/
  cra-app1/
    scripts/
      test.js  <-- ejected test.js script, not a test!
    src/
      App.js
      App.test.js <-- a test
  cra-app2/
    src/
      App.js

When running "yarn test" in cra-app2, jest tries to run cra-app1/scripts/test.js as a test, same as cra-app1/src/App.test.js. This is because we have given jest cra-app1 as a root and the jest testMatcher pattern '**/?(*.)(spec|test).{js,jsx,mjs}' matches test.js.

Just to be more clear -- this issue isn't related to the current PR, it just came up in testing this PR. The issue comes up when you have multiple cra-apps in a monorepo, and one or more of them has been ejected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at that structure, my earlier suggestion of detecting based on react-scripts as a dependency won't work. Since that cra-app1 has been ejected, it doesn't have react-scripts as a dependency and wouldn't be detected as a cra app. So, it falls into the category of being just some other thing in the monorepo that has its own build/test/etc. I think we need some inclusion/exclusion config, instead of assuming everything in the monorepo is cra source. (for the sake of posterity, this discussion probably belongs elsewhere -- should I create a new issue for the discussion?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's discuss in a new issue. 👍

echo yes | npm run eject
verifyBuild
yarn start --smoke-test
verifyTest
popd

# ******************************************************************************
# Test create-react-app inside workspace
# ******************************************************************************
# npx create-react-app --internal-testing-template="$root_path"/packages/react-scripts/fixtures/yarn-ws/ws/cra-app1 cra-app2
# -- above needs https://github.com/facebookincubator/create-react-app/pull/3435 to user create-react-app
popd

# Cleanup
Expand Down