-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Support running create-react-app inside a monorepo. #3967
Conversation
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 |
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.
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.
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.
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?
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.
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.
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.
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?)
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.
Let's discuss in a new issue. 👍
tasks/e2e-monorepos.sh
Outdated
pushd packages | ||
npx create-react-app newapp | ||
cd newapp | ||
yarn |
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.
Do we want to run yarn automatically when creating the app (ie., inside init.js) to avoid running it here?
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.
Not sure what you mean. We use yarn to create an app if it’s installed.
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.
Yeah, you're right, I think this might be due to a yarn bug not creating the react-scripts .bin link when we run "yarn add react-scripts" for the new app. Needs some more debugging.
@bradfordlemley What's remaining here? |
As it stands, it's necessary to run Without running This is due to a yarn bug, not creating node_modules/.bin link to the hoisted react-scripts. I haven't tracked down a matching existing yarn issue yet, but I suspect there is one, and if not, I will create one. In the mean time, would you be ok with a workaround in init.js? Not sure what the workaround will be yet, maybe just to run |
… for react-scripts.
@gaearon The yarn workspace bug described above exists in yarn 1.3.2, but appears to be fixed in yarn nightly (1.4.1-20180211.2236). I wasn't able to track down the exact issue/fix. I've added a workaround in init.js, let me know what you think. |
@@ -172,10 +178,18 @@ module.exports = function( | |||
fs.unlinkSync(templateDependenciesPath); | |||
} | |||
|
|||
// yarn ws bug not creating app/node_modules/.bin link to hoisted react-scripts |
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.
Can this be now removed considering yarn
is at @1.6.0 and the fix landed in 1.5.1 in Feb (I think?)?
Of course there's the case that someone would run the script with a old version of yarn
but 🤷♂️
This reverts commit e188e1e.
@gaearon @bradfordlemley any updates on this? |
Please see #1333 @lifeiscontent |
Several create-react-app scripts assume that the create-react-app packages will be in app/node_modules.
But in a monorepo, e.g. yarn workspace, the create-react-app packages can be hoisted to the top-level node_modules.
This PR fixes those scripts to allow them to find packages at the top-level node_modules.
Fixes #3031.