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

Use env var to detect yarn or npm as the package manager #11322

Merged
merged 5 commits into from
Sep 1, 2021

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Aug 18, 2021

This removes the --use-npm flag and adds the --use-yarn flag. It also removes the previous behavior of checking for the existence of yarn as the signal to use it as the package manager. The new behavior will be to use npm by default and to use yarn with the --use-yarn flag.

The goal of this PR is to remove the check on whether yarn is present in the user's path as the signal to use yarn.

This PR is implemented on top of #11304, since it changed some of the same files and was easier to get tests passing on top of that. I will rework this based on the outcome of that PR. For now, here's the differing commits in this PR: lukekarrys/create-react-app@lk/dev-npm-7...lk/use-npm

@arcanis
Copy link
Contributor

arcanis commented Aug 18, 2021

The new behavior will be to use npm by default and to use yarn with the --use-yarn flag.

I'm curious why?

@merceyz
Copy link
Contributor

merceyz commented Aug 21, 2021

It would be better to use whichever package manager called it by checking process.env.npm_config_user_agent and allow using flags to override the environment check.

@bryik
Copy link

bryik commented Aug 23, 2021

It would be better to use whichever package manager called it by checking process.env.npm_config_user_agent and allow using flags to override the environment check.

Are you suggesting CRA should use yarn if the user runs yarn create react-app my-app and npm if the user runs npx create-react-app my-app? If so, I agree. I think this would be more intuitive than the current behaviour of defaulting to yarn if yarn is installed.

@lukekarrys lukekarrys changed the title Use npm as default package manager Use env var to detect yarn or npm as the package manager Aug 23, 2021
@lukekarrys
Copy link
Contributor Author

It would be better to use whichever package manager called it by checking process.env.npm_config_user_agent and allow using flags to override the environment check.

I agree that's a much better solution, and I reworked this PR to implement something similar to that.

I opted against using anything from npm_config since that could be overridden by the user. I think I found a solution using npm_execpath, and I'm open to any other feedback on if this will work.

@merceyz
Copy link
Contributor

merceyz commented Aug 25, 2021

npm_execpath can be pretty much anything with Yarn 1, the safest check is process.env.npm_config_user_agent

Are you suggesting CRA should use yarn if the user runs yarn create react-app my-app and npm if the user runs npx create-react-app my-app?

@bryik Yes

@lukekarrys
Copy link
Contributor Author

the safest check is process.env.npm_config_user_agent

This can also be anything as it's user configurable with npm config set user-agent.

npm_execpath can be pretty much anything with Yarn 1

I agree that npm_execpath is not ideal as Yarn 1 will only set it if it hasn't already been set. But I still think this is more likely to be correct than the user configurable npm_config_user_agent when running yarn create create-react-app.

@merceyz Do you have some examples of common use cases where npm_execpath wouldn't be correct when running yarn create?

@merceyz
Copy link
Contributor

merceyz commented Aug 26, 2021

This can also be anything as it's user configurable with npm config set user-agent.

If the user changes the user-agent config to look like Yarn while using npm that's on them, regardless it's still the most likely to be correct way to see if the user is using Yarn; especially considering all Yarn versions after 2.0.0-rc.4 (released 2019-07-05) doesn't let the user change it through a config.

@merceyz Do you have some examples of common use cases where npm_execpath wouldn't be correct when running yarn create?

Since it's set to the main module filename pretty much always.

$ docker run -it --rm node:16.7.0 yarn exec env | grep npm_execpath
npm_execpath=/opt/yarn-v1.22.5/bin/yarn.js

$ docker run -it --rm node:16.7.0 bash -c 'yarn set version latest && yarn exec env' | grep npm
_execpath
npm_execpath=/.yarn/releases/yarn-1.22.11.cjs

$ docker run -it --rm node:16.7.0 bash -c 'yarn set version nightly && yarn exec env' | grep np
m_execpath
npm_execpath=/.yarn/releases/yarn-nightly.cjs

@lukekarrys
Copy link
Contributor Author

Thanks for those examples and further explanation. I agree now that detecting execpath looks like more trouble than user_agent. I also checked out Neutrino and Next which use user_agent, so I think that makes it abundantly clear. I'll update this PR.

This also updates the package manager test to check
for yarn based on user_config env var
@iansu
Copy link
Contributor

iansu commented Sep 1, 2021

@arcanis We've actually made some changes to this since the initial PR. Our goal is to make running create-react-app deterministic. With the old behaviour two people running npx create-react-app my-app could end up with different installs if one of them happened to have Yarn installed.

The new behaviour in this PR looks at how create-react-app was invoked and uses the corresponding package manager. npx create-react-app my-app will use npm and yarn dlx create-react-app my-app (or yarn create react-app my-app) will use Yarn. This will also potentially allow us to add support for other package managers by following the same pattern.

@arcanis
Copy link
Contributor

arcanis commented Sep 1, 2021

Yep, I agree that some kind of detection based on how it gets invoked makes sense (related: nodejs/corepack#25). As a quick note en passant (since this PR is implemented on top of the other), #11304 happens to accidentally remove the PnP test:

https://github.com/facebook/create-react-app/pull/11304/files#diff-5562440d61a0381fa97715f19f34abaa18d5ab75a57a64c3d3a5ffa63163b93cR276-R285

@iansu
Copy link
Contributor

iansu commented Sep 1, 2021

@arcanis This test has been failing on our main branch for a while too. We're just trying to get our tests into a workable state right now so we've disabled a number of failing tests. We're planning to go back and fix them as soon as possible.

@iansu
Copy link
Contributor

iansu commented Sep 1, 2021

Looks like we've got one failing test with Yarn and Node 14. We're going to move ahead with this PR since we're still in the alpha stage and address this test failure in a follow up PR. cc @lukekarrys

@iansu iansu merged commit 04482a6 into facebook:main Sep 1, 2021
@iansu iansu added this to the 5.0 milestone Sep 2, 2021
NazariiTomei added a commit to NazariiTomei/create-vue that referenced this pull request Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants