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!: no implicit latest tag on publish when latest > version #7939

Merged
merged 20 commits into from
Dec 6, 2024

Conversation

reggi
Copy link
Contributor

@reggi reggi commented Nov 26, 2024

BREAKING CHANGE: Upon publishing, in order to apply a default "latest" dist tag, the command now retrieves all prior versions of the package. It will require that the version you're trying to publish is above the latest semver version in the registry, not including pre-release tags.

Implements npm RFC7.

Related to prerelease dist-tag: #7910
Apart of npm 11 roadmap: npm/statusboard#898

@reggi reggi requested a review from a team as a code owner November 26, 2024 04:23
@reggi reggi force-pushed the reggi/prevent-lower-semver-without-explicit-tag branch 2 times, most recently from 3491a30 to 1747c7a Compare November 26, 2024 04:34
@reggi
Copy link
Contributor Author

reggi commented Nov 26, 2024

I took liberty to standardize all the const npmFetch = require('npm-registry-fetch') imports, wasn't crazy about fetch collision, and wanted all to use the same variable. Can revert.

@reggi reggi force-pushed the reggi/prevent-lower-semver-without-explicit-tag branch from 1747c7a to c8351fc Compare November 26, 2024 15:59
@wraithgar
Copy link
Member

I took liberty to standardize

This is how things get better. Iteratively, and as we are already working in these files.

Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I am so excited that this could make https://npmjs.com/safe-publish-latest obsolete!

lib/commands/publish.js Outdated Show resolved Hide resolved
@ljharb
Copy link
Contributor

ljharb commented Nov 26, 2024

hmm, one difference i notice here between https://github.com/ljharb/safe-publish-latest/blob/main/getLatestError.js and this PR is that it's only comparing the "latest" version, not all versions.

Specifically, if I have v1.4.0 and v1.5.0 published but have v1.3.0 marked as latest, and I publish v1.4.1, i wouldn't necessarily want that to be auto-marked latest - in other words, the only time i want implicit latest is when i have the normal scenario of "the latest tag is also the latest version".

@reggi
Copy link
Contributor Author

reggi commented Nov 27, 2024

@ljharb we spoke about this a bit before implementation, I broke it down like this

The two options are:

  1. latest: 10, but 11 exists you deploy 10.5 without tag and it works, lets call this dist-tag version < publish version get "implicit latest" (this PR)
  2. latest: 10, but 11 exists you deploy 10.5 without tag and it fails because the latest published version (not tag) to exist is 11, lets call this latest published version < publish version gets "implicit latest"

We went with 1, the idea being that this is only about "should we use default (implicit) tag latest tag or not", and that only looking at the existing "latest" tag value makes the most sense here.

@ljharb
Copy link
Contributor

ljharb commented Nov 27, 2024

That's certainly a viable decision, and I'm glad yall discussed it (altho it'd have been nice to have it discussed publicly, like npm changes historically have been) - but unfortunately I very much have encountered scenario 2 many times, not just on my own packages, which is why I wrote safe-publish-latest that way, and have been using it successfully for 8 years on many hundreds of packages.

Obviously this can ship, and we could wait for another major to ship scenario 2, but it seems like the wiser option is to always prefer being maximally strict right out the gate, since tightening is semver-major but loosening is not?

@reggi
Copy link
Contributor Author

reggi commented Dec 2, 2024

@ljharb reimplemented using semver latest instead of latest dist tag. I can't think of a downside for using semver latest as suggested. Overall I think it makes sense and will feels more intuitive.

Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

awesome, LGTM and thank you!

lib/commands/publish.js Outdated Show resolved Hide resolved
lib/commands/publish.js Outdated Show resolved Hide resolved
lib/commands/publish.js Outdated Show resolved Hide resolved
lib/commands/publish.js Outdated Show resolved Hide resolved
@reggi reggi marked this pull request as ready for review December 2, 2024 18:48
lib/commands/publish.js Outdated Show resolved Hide resolved
lib/commands/dist-tag.js Outdated Show resolved Hide resolved
@reggi reggi force-pushed the reggi/prevent-lower-semver-without-explicit-tag branch from 37cb797 to 777184d Compare December 4, 2024 20:49
lib/commands/publish.js Outdated Show resolved Hide resolved
lib/commands/publish.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

Those publish tests look so much cleaner now.

@wraithgar wraithgar merged commit f3ac7b7 into latest Dec 6, 2024
71 checks passed
@wraithgar wraithgar deleted the reggi/prevent-lower-semver-without-explicit-tag branch December 6, 2024 16:39
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.

3 participants