-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
feat(remix-dev): use auto-detected package manager #2562
Conversation
Hi @illright, Welcome, and thank you for contributing to Remix! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
@illright Could you please rebase your branch onto latest |
@MichaelDeBoey done |
Apparently, at some point they wanted to be spelled "pNPM", but not anymore
This would also fix #1098, right? |
Yes, it should |
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.
Love the direction on this. Great work!
@@ -183,6 +186,79 @@ describe("remix CLI", () => { | |||
{ question: /install/i, type: ["n", ENTER] }, | |||
]); | |||
}); | |||
|
|||
it.skip("recognizes when Yarn was used to run the command", async () => { | |||
let projectDir = getProjectDir("yarn-create"); |
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.
Could you move these tests to create-test.ts? The setup there is much better for these tests. They should run a lot faster as well.
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.
Done, actually that allowed me to implement the tests, which is great :)
There are two failing tests in |
The latest commit on I think the failure is related to your changes: https://github.com/remix-run/remix/runs/5989818412?check_suite_focus=true#step:6:53 I believe we don't actually want to shell out to a package manager. We have a mock in place to make sure we don't. Maybe you forgot to update the mock? I haven't looked into it yet myself. |
Ah, it was a different test failing in CI, didn't even notice. On my machine, this was passing since I actually had For some reason, I get another test failing locally, Console dump
|
I noticed that failure on CI. Looks like we need to increase the timeout on that test because it's a pretty slow one. Thanks for fixing things up! I think we can get this merged soon. |
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.
Solid 👍 Thank you!
I should have created an issue, but I'm not sure if it's an isolated case. Instead of Hence |
@alvinthen please create an issue for this, and give more details about your env in it 🙏🏼 |
🤖 Hello there, We just published version Thanks! |
This PR introduces detection of the preferred package manager by looking at the
npm_user_agent
environment variable that all popular package managers set (npm
,yarn
,pnpm
).I need some assistance with testing — I'm not sure how to mock the calls toUPD: resolved!execSync("pnpm install", ...)
, since we're spawning another Node process that calls this function. For now, I marked these tests to be skipped.Closes #1098