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

Use yarn when running inside yarn workspace. #3997

Merged
merged 7 commits into from
Feb 10, 2018
Merged

Use yarn when running inside yarn workspace. #3997

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f1018cf
Run yarn after ejecting.
bradfordlemley Feb 8, 2018
869413f
On eject, choose to run yarn instead of npm if yarn is available.
bradfordlemley Feb 9, 2018
c792a98
Move monorepo to react-dev-utils.
bradfordlemley Feb 9, 2018
a34a610
Fix lint.
bradfordlemley Feb 9, 2018
4817651
Rename monorepo to workspaceUtils.
bradfordlemley Feb 9, 2018
c664c11
Add react-dev-utils dep for create-react-app.
bradfordlemley Feb 9, 2018
41fb5b0
getMonorepo -> findMonorepo
bradfordlemley Feb 10, 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
11 changes: 10 additions & 1 deletion packages/react-scripts/scripts/eject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ function getGitStatus() {
}
}

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

inquirer
.prompt({
type: 'confirm',
Expand Down Expand Up @@ -224,7 +233,7 @@ inquirer
}
}

if (fs.existsSync(paths.yarnLockFile)) {
if (fs.existsSync(paths.yarnLockFile) || isYarnAvailable()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the problem is due to npm corrupting a Yarn-produced tree, this can be solved by deleting node_modules first before re-running npm.

Copy link
Contributor Author

@bradfordlemley bradfordlemley Feb 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

i don't think it's corrupting, per-se -- it's just that if this is happening within a yarn workspace, npm can't handle it properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I’d like to understand what’s going on here better. Why do we run Yarn in this test even if the user created an app with npm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean echo yes | npm run eject, right? We can change that to yarn. In that case, maybe you want to detect that the eject script was run with yarn and automatically use yarn for the install? That would help enforce some consistency with yarn/npm usage, but not sure what your philosophy is on that. Let me know what you want to do in this PR, not sure this PR is the appropriate place for that auto-detecting change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm no, I just mean that if an app was created with npm, we should run npm on ejecting, and if it was created with Yarn, we should use Yarn.

We use the lockfile to tell if the app was created with Yarn. Is this not sufficient in this case? Why?

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 extract that into react-dev-utils maybe and use in both places?
Or cache this in config/paths.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.

There are a bunch of places where a decision to use yarn or npm is made, probably all should be updated:

create-react-app/packages/create-react-app/createReactApp.js

Line 188 in c342677

const useYarn = useNpm ? false : shouldUseYarn();

create-react-app/packages/react-scripts/scripts/init.js

Line 88 in c342677

const useYarn = fs.existsSync(path.join(appPath, 'yarn.lock'));

create-react-app/packages/react-scripts/scripts/eject.js

Line 227 in c342677

if (fs.existsSync(paths.yarnLockFile)) {

create-react-app/packages/react-scripts/scripts/build.js

Line 48 in c342677

const useYarn = fs.existsSync(paths.yarnLockFile);

Copy link
Contributor

@gaearon gaearon Feb 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The first one intentionally checks for existence of Yarn because we haven't created a project yet.

All the other ones are checking for presence of yarn.lock. If it's committed, then the project was created with Yarn (maybe by another member of your team) so you should really install it (even if you don't have it).

I don't mind unifying all except the first one.

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 createReactApp.js: if a user is creating an app in a yarn workspace, they really want to be using yarn, using npm will cause issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds good.

const windowsCmdFilePath = path.join(
appPath,
'node_modules',
Expand Down
1 change: 0 additions & 1 deletion tasks/e2e-monorepos.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ verifyTest

# Test eject
echo yes | npm run eject
yarn
verifyBuild
yarn start --smoke-test
verifyTest
Expand Down