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: don't try to deprecate nonexistant versions #6050

Merged
merged 1 commit into from Jan 13, 2023

Conversation

wraithgar
Copy link
Member

If you pass npm a version that doesn't exist, it still tries to PUT the
packument but with no changes. This is unneccessary and currently
results in a 422 error from the npm registry

If you pass npm a version that doesn't exist, it still tries to PUT the
packument but with no changes.  This is unneccessary and currently
results in a 422 error from the npm registry
@wraithgar wraithgar requested a review from a team as a code owner January 12, 2023 21:09
Copy link
Collaborator

@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! what will the error output be when no versions exist?

@nlf
Copy link
Contributor

nlf commented Jan 12, 2023

i had the same question as @ljharb, it doesn't feel like the best interaction to just silently do nothing if there's no matching version either. it would be better to throw an error telling the user there were no matching published versions, or at least log a warning

@npm-cli-bot
Copy link
Collaborator

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 43.000 ±6.73 15.065 ±0.10 13.887 ±0.12 16.322 ±0.90 2.625 ±0.01 2.608 ±0.01 2.113 ±0.04 9.915 ±0.03 2.137 ±0.03 3.433 ±0.39
#6050 36.359 ±0.97 14.937 ±0.05 13.863 ±0.01 16.371 ±0.90 2.595 ±0.01 2.638 ±0.10 2.023 ±0.01 9.698 ±0.04 2.031 ±0.02 3.251 ±0.11
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 28.305 ±0.96 11.918 ±0.02 10.876 ±0.00 11.702 ±0.06 2.406 ±0.01 2.390 ±0.01 2.176 ±0.05 7.466 ±0.06 2.077 ±0.00 2.857 ±0.02
#6050 27.187 ±3.00 11.704 ±0.01 10.769 ±0.03 11.878 ±0.33 2.386 ±0.00 2.407 ±0.06 2.107 ±0.01 7.359 ±0.04 1.947 ±0.04 2.814 ±0.06

@wraithgar
Copy link
Member Author

There is currently no error output (other than the registry bug) so my approach was to make it act as closely to how it works now without introducing new errors that could be considered breaking changes. We can certainly start exploding here, but as it stands now you can deprecate nonexistent versions with no warnings from npm.

@wraithgar
Copy link
Member Author

npm has operated like this since at least v6 (I didn't check code before then)

@lukekarrys lukekarrys merged commit 8be672b into latest Jan 13, 2023
@lukekarrys lukekarrys deleted the gar/deprecate-nonexistant branch January 13, 2023 21:51
@github-actions github-actions bot mentioned this pull request Jan 13, 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.

None yet

5 participants