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

Allow packageManager to be specified by name alone #300

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

rotu
Copy link

@rotu rotu commented Sep 4, 2023

@rotu rotu marked this pull request as ready for review September 4, 2023 07:41
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

This allows omitting the version from the package.json packageManager field as well which it shouldn't do.

@arcanis
Copy link
Contributor

arcanis commented Sep 4, 2023

And for corepack install -g (and this command only), yarn should alias to yarn@^1, not yarn@*.

@rotu
Copy link
Author

rotu commented Sep 4, 2023

This allows omitting the version from the package.json packageManager field as well which it shouldn't do.

Hmm... I thought that was enforced in resolveDescriptor, which has the allowTags option. Under what circumstances (if any) would we allow a version range but not a tag?

@rotu
Copy link
Author

rotu commented Sep 4, 2023

And for corepack install -g (and this command only), yarn should alias to yarn@^1, not yarn@*.

@arcanis I left that out of scope on purpose. I can't figure out the intended behavior so I punted.

At the very least it seems like corepack use yarn should be synonymous with corepack use yarn@* and either yarn@stable or yarn@latest.

But here's what it is right now:

  • corepack install -g yarn@stable -> yarn 3.6.3

  • corepack install -g 'yarn@*' -> yarn 4.0.0-rc.50

  • corepack install -g 'yarn@latest' -> Usage Error: Tag not found (latest)

  • npm exec --package yarn -c 'yarn --version' -> 1.22.19

  • npm exec --package 'yarn@*' -c 'yarn --version' -> 1.22.19

  • yarn dlx --package 'yarn' yarn --version -> 1.22.19

  • yarn dlx --package 'yarn@*' yarn --version -> 2.4.3


Heck, even https://www.npmjs.com/package/yarn and https://yarnpkg.com/package?q=yarn&name=yarn don't acknowledge any version of yarn exists past 2.4.3, so how should a user know what a valid version specifier is?

Yarn is a wonderful project but the critical user story of "how should I install it, and what version should I use?" is a dumpster fire. I am of the opinion that the yarn cli needs to provide its own backwards compatibility affordances, not require other tools to implement special rules.

@arcanis
Copy link
Contributor

arcanis commented Sep 4, 2023

Yeah, I think it's fine to keep it as a follow-up. I have another idea to make this process a little less magic, I'll handle that.

Heck, even https://www.npmjs.com/package/yarn and https://yarnpkg.com/package?q=yarn&name=yarn don't acknowledge any version of yarn exists past 2.4.3, so how should a user know what a valid version specifier is?

Because you're looking at the npm package, and Yarn hasn't been distributed on npm for the past couple of years. So yeah, the version you're seeing there is outdated. The proper list of versions is here: https://repo.yarnpkg.com/tags

@rotu
Copy link
Author

rotu commented Sep 4, 2023

Because you're looking at the npm package, and Yarn hasn't been distributed on npm for the past couple of years. So yeah, the version you're seeing there is outdated. The proper list of versions is here: https://repo.yarnpkg.com/tags

That's awesome documentation and makes a lot of sense!

The npm page and yarn page are the de facto storefront for a javascript package, and even if they aren't the place releases happen, they need guidance in the readmes (even if those instructions start with "Don't install this version and don't install with npm" and a link to the recommendations for how to get started with yarn).

The npm page has also been confusing because the "berry" dist-tag implies that "berry" is the name for major version 2, but the active development repo is also called "berry" and has releases for major versions 2, 3, and 4.

I have definitely been turned off (perhaps unfairly) from trying yarn in the past by these sorts of issues!

@rotu
Copy link
Author

rotu commented Sep 4, 2023

This allows omitting the version from the package.json packageManager field as well which it shouldn't do.

@merceyz Why not?

Please see my latest commit, which is how I feel it should work, somewhat like npm install --save-exact.

@rotu rotu requested a review from merceyz September 7, 2023 01:20
@aduh95
Copy link
Contributor

aduh95 commented Oct 8, 2023

@merceyz @arcanis Can we move forward with this?

@merceyz
Copy link
Member

merceyz commented Oct 8, 2023

@merceyz Why not?

Reproducibility, if a project is installable and works today it should work in a year, if a range is used that might not be the case anymore.

@merceyz @arcanis Can we move forward with this?

@aduh95 I don't think this should land in its current state and it needs some tests to ensure it works as expected.

@rotu
Copy link
Author

rotu commented Oct 8, 2023

@merceyz Why not?

Reproducibility, if a project is installable and works today it should work in a year, if a range is used that might not be the case anymore.

Note that corepack use npm@stable is accepted and has the same issue. This locks down the version in the same way.

@merceyz
Copy link
Member

merceyz commented Oct 8, 2023

That command would set the package.json#packageManager field to a specific version of npm, not a range.

@rotu
Copy link
Author

rotu commented Oct 8, 2023

That command would set the package.json#packageManager field to a specific version of npm, not a range.

That's my understanding too - I feel like I don't understand your point. I'm picturing the user specifying a value loosely with corepack use. Then corepack install will set up the project locked to that particular package manager.

if a range is used that might not be the case anymore.

What's the use case you're envisioning here which, after this PR, would create a non-reproducible build?

@rotu
Copy link
Author

rotu commented Oct 9, 2023

@merceyz updated the tests. I think this clarifies how I expect this to work, basically like npm install --save-exact, where the user can be fuzzy but the result will be specific.

EDIT: I'm having second thoughts about this. Maybe the right thing to do is to allow a specifier. corepack use will automatically reify the version, but if the user wants to put in a package specifier (a la #95), why shouldn't we trust them? It might be more important to the user to have bug fixes than to have reproducibility, especially for nascent package managers like bun.

A reproducible build will always rely on developer choices. Like committing the lockfile or pnp files, using a particular version of the node runtime, using deterministic build steps, and setting package manager options like install-strategy. To that end, I think it makes sense to respect the user's choice of how they want to express their packageManager specifier, rather than forcing a version (which may or may not result in a reproducible build anyway; e.g. #308).

`corepack use` makes it easy to lock down a specific version.
Copy link
Contributor

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

Why shouldn't we trust them? It might be more important to the user to have bug fixes than to have reproducibility, especially for nascent package managers like bun.

The current model is to favour good practices, in particular reproducibility. It's a major point of Corepack, otherwise you can just npm install whatever package manager you want and that's it, no need for Corepack.

I agree with @merceyz on this - adding support for loose specifiers is fine on the CLI commands (because they only matter at the time the commands are run), but allowing them in the packageManager field is a different story that should be evaluated in its own PR (and I'll push back on it for the reasons described above).

@rotu
Copy link
Author

rotu commented Oct 9, 2023

Why shouldn't we trust them? It might be more important to the user to have bug fixes than to have reproducibility, especially for nascent package managers like bun.

The current model is to favour good practices, in particular reproducibility. It's a major point of Corepack, otherwise you can just npm install whatever package manager you want and that's it, no need for Corepack.

Good practice is, when reproducibility is key, to track the full version with hash, which is exactly what corepack use does. If the package manager actually follows the semantic versioning spec, then good practice is to stick with compatible releases, including tracking prerelease lines as they mature.

Even without this restriction, corepack is necessary because:

  1. Typically node_modules/.bin is not in $PATH so even if you install your preferred package manager for a project, it's unwieldy to call.
  2. The yarn package manager does not make itself available in a package repository. That is, npm install yarn@latest and npm install yarn@berry install versions which are very out of date.

I agree with @merceyz on this - adding support for loose specifiers is fine on the CLI commands (because they only matter at the time the commands are run), but allowing them in the packageManager field is a different story that should be evaluated in its own PR (and I'll push back on it for the reasons described above).

That's going to wind up with those CLI commands in build scripts and commit hooks. The whole idea behind a human-editable package.json is to track the developer's intent, not enforce reproducibility (that's what the lockfile is for). #95 has 28 👍 versus 2 👎, which seems to clearly demonstrate that we should accept loose versions here.

@arcanis and @merceyz, would you be happy with instead reifying the version on corepack install as I previously had?

@arcanis
Copy link
Contributor

arcanis commented Oct 9, 2023

@arcanis and @merceyz, would you be happy with instead reifying the version on corepack install as I previously had?

Not sure what you mean by that - as far as I can tell from the tests you link, this still lets you write a range in the packageManager field, which is what I don't think we should merge as part of this PR.

Essentially, I believe this PR is conflating two different things (ranges in the CLI, and ranges in the project definition), and I'm against using the merits of the first request (which has consensus) to land the second (which hasn't).

@rotu
Copy link
Author

rotu commented Oct 9, 2023

@arcanis and @merceyz, would you be happy with instead reifying the version on corepack install as I previously had?

Not sure what you mean by that - as far as I can tell from the tests you link, this still lets you write a range in the packageManager field, which is what I don't think we should merge as part of this PR.

Yes. In that revision of the PR, this lets you write a range in the packageManager field and then, the first time you corepack install, it rewrites the packageManager field to the specific version. This is a much more comfortable dev experience than crashing out with a "Usage Error" (even on corepack use and corepack up!).

Essentially, I believe this PR is conflating two different things (ranges in the CLI, and ranges in the project definition), and I'm against using the merits of the first request (which has consensus) to land the second (which hasn't).

Yes, in fixing the first, I realized how arbitrary and unwieldy the second one is and what it would take to fix. The feedback on #18 and #95 seems clear consensus that having something package.json which looks like a package specifier but doesn't work like one is bad DX, and that users should be able to edit package.json by hand.

So @arcanis and @merceyz, what do you think the developer's EXPECTATION is if they have written an imprecise version (or none at all) into package.json?

@rotu rotu changed the title Don't require version range Allow packageManager to be specified by name alone Oct 9, 2023
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 this pull request may close these issues.

Corepack use should allow omitting version Support for semver ranges?
4 participants