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(view): error on missing version #5035

Merged
merged 15 commits into from Jun 22, 2022
Merged

fix(view): error on missing version #5035

merged 15 commits into from Jun 22, 2022

Conversation

wraithgar
Copy link
Member

@wraithgar wraithgar commented Jun 16, 2022

This fixes an error in npm show. When calling npm show with a specific
version of a package that does not exist, it does not show anything and
gives a zero exit code. This has been changed: now it gives a 404 Error
similar to if the package does not exist. Can be tested with npm show
express@5.0.0 (local: node bin/npm-cli.js info express@5.0.0)

Fixes #4964

Credit: @lukaskuhn-lku

@wraithgar wraithgar requested a review from a team as a code owner June 16, 2022 16:23
@wraithgar
Copy link
Member Author

This is a continuation of #5011, I fixed the tests.

@npm-cli-bot
Copy link
Collaborator

npm-cli-bot commented Jun 16, 2022

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 39.943 ±0.08 21.314 ±0.29 19.670 ±0.00 22.354 ±0.73 3.307 ±0.01 3.318 ±0.08 2.720 ±0.03 13.198 ±0.10 2.640 ±0.05 3.792 ±0.04
#5035 42.364 ±0.04 21.445 ±0.38 18.401 ±0.15 21.432 ±0.31 3.240 ±0.03 3.266 ±0.07 2.587 ±0.01 12.577 ±0.17 2.574 ±0.01 3.770 ±0.10
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 30.172 ±0.95 15.573 ±0.11 13.788 ±0.18 14.835 ±0.26 3.027 ±0.01 3.136 ±0.07 2.644 ±0.03 9.374 ±0.09 2.497 ±0.04 3.425 ±0.01
#5035 29.501 ±0.24 15.553 ±0.06 13.798 ±0.04 15.021 ±0.39 2.999 ±0.05 2.971 ±0.05 2.653 ±0.03 9.268 ±0.03 2.522 ±0.04 3.383 ±0.02

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.

What about a dist-tag that doesn’t exist?

lib/commands/view.js Show resolved Hide resolved
@wraithgar
Copy link
Member Author

What about a dist-tag that doesn’t exist?

This PR also fixes that situation

~/D/n/cli (gar/5011|✔) $ node . view lodash@asdf
npm ERR! code E404
npm ERR! 404 No match found for version asdf
npm ERR! 404 
npm ERR! 404  'lodash@asdf' is not in this registry.

jsjoeio added a commit to coder/code-server that referenced this pull request Aug 23, 2022
npm changed the way the `npm view` command handles missing versions.
Before it exited with a non-error. Now it errors.

Ref: npm/cli#5035

This modifies the script logic to handle those new changes.
jsjoeio added a commit to coder/code-server that referenced this pull request Aug 23, 2022
* chore: clean up logging in npm script

* fix: catch error if npm version missing

npm changed the way the `npm view` command handles missing versions.
Before it exited with a non-error. Now it errors.

Ref: npm/cli#5035

This modifies the script logic to handle those new changes.
@MarshallOfSound
Copy link

MarshallOfSound commented Oct 27, 2022

@wraithgar Probably not worth a new issue but I wanted to note that this caused a catastrophic relatively bad release infrastructure failure for the Electron project.

We have since recovered and worked around the issue and ensured the npm is correctly pinned in our deployment environment but this was IMO a drastic breaking change in behaviour in a patch bump of the NPM cli.

Specifically we were checking if a version had been published to npm via the CLI using:

npm show electron@1.2.3 --json

And assuming that "no output" meant the version had not been published and "valid output" could be parsed into the registry metadata. In both cases since the dawn of npm the command exited cleanly, this change caused a non-zero exit code in the unpublished scenario which then propagated as an error through our release scripts causing an infinite backoff-and-retry (as we assumed a non-zero exit code meant npm was having issues).

Not sure if this is even gonna scratch on the radar, but I'd consider a change in exit code semantics for a CLI tool a breaking change and would be reserved for the next major version of npm.

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.

[BUG] npm info exits with zero exit code if package@version is not found
7 participants