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 the package manager version specified by the project, but still allow other package manager invocations #195

Closed
kenrick95 opened this issue Oct 11, 2022 · 14 comments · Fixed by #197

Comments

@kenrick95
Copy link
Contributor

At time of writing, at 0.14.0 there is COREPACK_ENABLE_STRICT=0 , which disables this check completely and will always use the fallback version of package managers. However, there should be a middle ground, to use the package manager version specified by the project, but does not throw error when other package manager is invoked.

Use case example:

  • globally, I prepared & activated [email protected] and [email protected]
  • there is a project, managed using [email protected] (specified in the "packageManager" field in package.json). However there are some external dependencies with pre/post-install scripts that calls "yarn build".

With the default configuration of Corepack, I cannot bootstrap the project at all, i.e. I cannot successfully run pnpm install. This is because invocation of yarn inside that repo will fail due to "Usage error", i.e. #157)

However, with COREPACK_ENABLE_STRICT=0, I still cannot bootstrap the project as expected, because now pnpm install will use the fallback/global pnpm (7.13.4) instead of the version specified by the repo (6.33.0)

Proposal:

When I set COREPACK_ENABLE_STRICT=0

  • running pnpm in that project should still use the version specified by the repo (6.33.0) instead of the fallback/global version.
  • running yarn in that project should use the global/fallback version

What do you think?

@styfle
Copy link
Member

styfle commented Oct 11, 2022

Agreed, that is how I expected it to work.

This seems like a bug.

@aduh95
Copy link
Contributor

aduh95 commented Oct 11, 2022

The original name for the env variable was something like COREPACK_IGNORE_PACKAGE_JSON, as I figured it would be convenient to give a way for users to skip the package.json parsing entirely (e.g. if you have an enormous package.json file, you might prefer to specify the version yourself instead of having Corepack reparse your JSON file every time).

That being said, I agree that your use case seems valid, and Corepack should default to make it work, or at least provide a way to make it work.

Slightly off-topic, but in PHP land, you can use @build instead of composer build (see https://getcomposer.org/doc/articles/scripts.md#referencing-scripts), I really wish JS package managers had adopted a similar pattern as it would circumvent this problem entirely.

@arcanis
Copy link
Contributor

arcanis commented Oct 11, 2022

Yep, I also thought that COREPACK_ENABLE_STRICT would only affect the package manager check, not the version itself 🤔

There's probably still time to change it since we're experimental, especially since the current behaviour isn't obvious.

I really wish JS package managers had adopted a similar pattern as it would circumvent this problem entirely.

In Yarn you can use run build (instead of yarn run build), the original intent being that other packages managers could implement run as well and thus avoid users having to hardcode the package managers' name just for running scripts.

Unfortunately afaik only Yarn implements it (we didn't advertise it much, to be fair).

@mrmckeb
Copy link

mrmckeb commented Oct 14, 2022

We've hit this issue too. In our case, a package we rely on has an install script that looks like this:

{
  "install": "restore-from-cache || npm run rebuild",
}

We want to use Corepack to help engineers and tooling use the right package manager and version to interact with the repo, but checking invocations by packages feels too strict. Ideally, packages would be able to use "run" or "corepack run" instead (#57), but that could take a long time to filter through the ecosystem.

We don't want to have our engineers set COREPACK_ENABLE_STRICT to 0 as we feel that negates the benefits of Corepack.

@aduh95
Copy link
Contributor

aduh95 commented Oct 14, 2022

Open an issue on their repo asking them to consider using $npm_execpath run build instead of npm run build? I'm not saying that it's not worth fixing the issue on Corepack side, but that seems it should be easy enough for them to fix that.

@styfle
Copy link
Member

styfle commented Oct 14, 2022

$npm_execpath doesn't work on Windows #57 (comment)

@ljharb
Copy link
Member

ljharb commented Oct 14, 2022

Isn't the "right" package manager for a package with scripts referencing npm, npm?

@arcanis
Copy link
Contributor

arcanis commented Oct 14, 2022

The right package manager is always the one managing the whole project. Hardcoding one is always a bad idea, because it runs afoul of the settings define for the project (for example yarn run foo wouldn't read the .npmrc, and the other way around) in the best case, and literally doesn't work in the worst case (npm run foo would crash on PnP installs).

@ljharb
Copy link
Member

ljharb commented Oct 14, 2022

The script can already ignore any settings set by the outer project by passing NPM_CONFIG_ env vars to another npm run invocation. It seems like the bad idea is using a package manager that the author didn't intend on any project at any level of the dep graph. I see tho that taking any of these ideas to their logical extension leads to an inevitable conclusion that wouldn't be productive to discuss, so I'll drop the topic.

@nathanhammond
Copy link

There isn't a "correct" solution here if we want to maintain the packageManager matching check. There's just a long list of options with varying degrees of pragmatic behavior.

Options:

  1. Drop the corepack check entirely.
  2. Drop the corepack check when invoked by another process.
  3. Push the solution to consumers via COREPACK_ENABLE_STRICT=0 <whatever> install
  4. Push the solution to package authors via a solution that approximates $npm_execpath or npm run foo || yarn run foo || pnpm run foo

Of these, Option 2 seems like it may be a good pragmatic balance. Anything that requires behavior change is likely to prevent adoption.

@aduh95
Copy link
Contributor

aduh95 commented Oct 26, 2022

Of these, Option 2 seems like it may be a good pragmatic balance. Anything that requires behavior change is likely to prevent adoption.

@nathanhammond would you like to send a PR? That seems like a reasonable approach to me.

@merceyz
Copy link
Member

merceyz commented Oct 26, 2022

That seems like a reasonable approach to me.

If I'm reading the suggestion correctly then I disagree, disabling the check defeats one of the "goals" of Corepack.

If the local project is configured for a different package manager, Corepack will request you to run the command again using the right package manager - thus avoiding corruptions of your install artifacts.
https://github.com/nodejs/corepack/tree/5eadc50192e205c60bfb1cad91854e9014a747b8#when-building-packages

@nathanhammond
Copy link

@merceyz I'm not attached to anything and I specifically note:

There isn't a "correct" solution here. There's just a long list of options with varying degrees of pragmatic behavior.

The reality is that corepack isn't ecosystem compatible, as demonstrated by the package I submitted an issue to (linked in this thread).

You're proposing we remain ecosystem incompatible which is a fine choice, but ensures that adoption of corepack will be limited.

In general: we collectively wrote the guidelines you cite, and we can always change them to improve the experience of using corepack.

@nathanhammond
Copy link

@aduh95 this is well outside the scope of work I should be focusing on right now (you may have seen my team's big announcement yesterday) so I won't have any opportunity to get to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants