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

fix: consider equality in publish dist tag check error message #7993

Open
wants to merge 5 commits into
base: latest
Choose a base branch
from

Conversation

reggi
Copy link
Contributor

@reggi reggi commented Dec 19, 2024

Currently the message for if it's the same version is wrong:

Cannot implicitly apply the "latest" tag because published version 1.1.12 is higher than the new version 1.1.12. You must specify a tag using --tag.

@reggi reggi requested a review from a team as a code owner December 19, 2024 16:55
@ljharb
Copy link
Contributor

ljharb commented Dec 19, 2024

It seems like this entire message shouldn't be triggering if the version is equal - isn't there pre-existing logic for "you can't publish a version that's already published" that should be hit?

@reggi
Copy link
Contributor Author

reggi commented Dec 19, 2024

I was thinking of this as a layer over the registry logic, the same that the front end and back end would implement overlapping validation. I think having this error thrown in the cli here is acceptable rather than intentionally letting them slip through and having the registry handle it, helps with debugging with --dry-run too.

@ljharb
Copy link
Contributor

ljharb commented Dec 19, 2024

In that case I'd expect a totally separate error - not about "latest" but about re-publishing an already-published version.

@reggi reggi changed the title fix: consider equality in publish dist tag check fix: consider equality in publish dist tag check error message Dec 19, 2024
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.

LGTM!

hashtagchris
hashtagchris previously approved these changes Dec 19, 2024
test/lib/commands/publish.js Show resolved Hide resolved
@wraithgar
Copy link
Member

isn't there pre-existing logic for "you can't publish a version that's already published" that should be hit?

Until npm 11 the cli did not have this information on publish. It send the new packument to the registry and all errors happened there.

@hashtagchris
Copy link
Contributor

hashtagchris commented Dec 20, 2024

isn't there pre-existing logic for "you can't publish a version that's already published" that should be hit?

Until npm 11 the cli did not have this information on publish. It send the new packument to the registry and all errors happened there.

If anyone else is curious, the fetch of the existing packument was added in e2f4455 (and then updated in this PR)

lib/commands/publish.js Show resolved Hide resolved
lib/commands/publish.js Show resolved Hide resolved
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.

4 participants