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

feat(remix-dev): use auto-detected package manager #2562

Merged
merged 10 commits into from
Apr 12, 2022
Merged

feat(remix-dev): use auto-detected package manager #2562

merged 10 commits into from
Apr 12, 2022

Conversation

illright
Copy link
Contributor

@illright illright commented Mar 31, 2022

  • Docs
  • Tests

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 to execSync("pnpm install", ...), since we're spawning another Node process that calls this function. For now, I marked these tests to be skipped. UPD: resolved!

Closes #1098

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Mar 31, 2022

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 CLA Signed label will be added to the pull request.

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

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Mar 31, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@MichaelDeBoey MichaelDeBoey changed the title Implement package manager detection feat(remix-dev): use auto-detected package manager Mar 31, 2022
@MichaelDeBoey
Copy link
Member

@illright Could you please rebase your branch onto latest dev & resolve conflicts?

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label Apr 6, 2022
@illright
Copy link
Contributor Author

illright commented Apr 7, 2022

@MichaelDeBoey done

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Apr 7, 2022
Apparently, at some point they wanted to be spelled "pNPM", but not anymore
@machour machour added the feat:dx Issues related to the developer experience label Apr 7, 2022
@machour
Copy link
Collaborator

machour commented Apr 7, 2022

This would also fix #1098, right?

@illright
Copy link
Contributor Author

illright commented Apr 7, 2022

Yes, it should

Copy link
Member

@kentcdodds kentcdodds left a 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");
Copy link
Member

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.

Copy link
Contributor Author

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 :)

@kentcdodds kentcdodds added the needs-response We need a response from the original author about this issue/PR label Apr 7, 2022
@illright
Copy link
Contributor Author

There are two failing tests in cli-test, I'm not sure why they're failing. It doesn't seem to be related to my changes + they fail even on dev

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Apr 12, 2022
@illright illright marked this pull request as ready for review April 12, 2022 11:55
@kentcdodds
Copy link
Member

The latest commit on dev passed CI: https://github.com/remix-run/remix/runs/5993726267?check_suite_focus=true

I think the failure is related to your changes: https://github.com/remix-run/remix/runs/5989818412?check_suite_focus=true#step:6:53

image

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.

@illright
Copy link
Contributor Author

Ah, it was a different test failing in CI, didn't even notice. On my machine, this was passing since I actually had pnpm installed :)

For some reason, I get another test failing locally, packages/remix-dev/__tests__/cli-test.ts. This happens both on this branch and in dev.

Console dump
A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them.
Summary of all failing tests
 FAIL  packages/remix-dev/__tests__/cli-test.ts (23.704 s)
  ● remix CLI › create prompts › allows you to go through the prompts

    thrown: Object {
      "status": "timeout",
      "stderr": "- Creating your app…
    ",
? Where would you like to create your app? /tmp/remix-tests-757s57373s/my-app
? What type of app do you want to create? Just the basics
? Where do you want to deploy? Choose Remix if you're unsure; it's easy to chang
? Do you want me to run `npm install`? No
    ",
    }

      160 |
      161 |   describe("create prompts", () => {
    > 162 |     it("allows you to go through the prompts", async () => {
          |     ^
      163 |       let projectDir = path.join(TEMP_DIR, "my-app");
      164 |
      165 |       let proc = childProcess.spawn(

      at __tests__/cli-test.ts:162:5
      at __tests__/cli-test.ts:161:3
      at Object.<anonymous> (__tests__/cli-test.ts:60:1)

  ● remix CLI › create prompts › allows you to go through the prompts and convert to JS

    thrown: Object {
      "status": "timeout",
      "stderr": "- Creating your app…
    ",
? Where would you like to create your app? /tmp/remix-tests-757s57373s/my-js-app
    
? What type of app do you want to create? Just the basics
? Where do you want to deploy? Choose Remix if you're unsure; it's easy to chang
? Do you want me to run `npm install`? No
    ",
    }

      185 |     });
      186 |
    > 187 |     it("allows you to go through the prompts and convert to JS", async () => {
          |     ^
      188 |       let projectDir = path.join(TEMP_DIR, "my-js-app");
      189 |
      190 |       let proc = childProcess.spawn(

      at __tests__/cli-test.ts:187:5
      at __tests__/cli-test.ts:161:3
      at Object.<anonymous> (__tests__/cli-test.ts:60:1)


Test Suites: 1 failed, 1 skipped, 19 passed, 20 of 21 total
Tests:       2 failed, 4 skipped, 300 passed, 306 total
Snapshots:   65 passed, 65 total
Time:        25.097 s, estimated 31 s
Ran all test suites in 8 projects.

@kentcdodds
Copy link
Member

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.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Solid 👍 Thank you!

@alvinthen
Copy link
Contributor

I should have created an issue, but I'm not sure if it's an isolated case. Instead of npm_user_agent, on my env, it was npm_config_user_agent

Hence yarn create remix is still prompting npm install

@machour
Copy link
Collaborator

machour commented Apr 22, 2022

@alvinthen please create an issue for this, and give more details about your env in it 🙏🏼

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-cbd996f-20220510 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed feat:dx Issues related to the developer experience package:dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Detect if user is running yarn and use yarn install instead of npm install
5 participants