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

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

Closed
2 tasks done
ThisIsMissEm opened this issue May 31, 2022 · 16 comments · Fixed by #5035
Closed
2 tasks done

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

ThisIsMissEm opened this issue May 31, 2022 · 16 comments · Fixed by #5035
Labels
Bug thing that needs fixing Priority 2 secondary priority issue Release 8.x work is associated with a specific npm 8 release

Comments

@ThisIsMissEm
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

This issue exists in the latest npm version

  • I am using the latest npm

Current Behavior

If I do:

npm info npm@8.7.1 && echo $?

Then the exit code is 0, but no output from npm info is printed, as 8.7.1 of npm doesn't exist. I would expect npm info to give a non-zero exit code if the package at the specified version doesn't exist. If the package doesn't exist at all, then npm info does have a non-zero exit code (with logs telling you it doesn't exist)

Expected Behavior

npm info npm@8.7.1

Would log a message saying the specified version of that package doesn't exist, and exit with a non-zero exit code, much like for non-existent packages.

Steps To Reproduce

See above, assuming npm@8.7.1 hasn't been published in the meantime.

Environment

  • npm: 8.7.1
  • Node.js: v16.13.1
  • OS Name: MacOS latest
  • System Model Name:
  • npm config: no non-default configuration.
@ThisIsMissEm ThisIsMissEm added Bug thing that needs fixing Needs Triage needs review for next steps Release 8.x work is associated with a specific npm 8 release labels May 31, 2022
@ThisIsMissEm ThisIsMissEm changed the title [BUG] npm info exits with zero exit code, even if package@version is not found [BUG] npm info exits with zero exit code if package@version is not found May 31, 2022
@ljharb
Copy link
Contributor

ljharb commented May 31, 2022

I'd indeed expect an error, just like npm show $doesNotExist errors out.

@ThisIsMissEm
Copy link
Author

npm show npm@8.7.1 has the same behaviour as npm info npm@8.7.1

@ljharb
Copy link
Contributor

ljharb commented May 31, 2022

yes, they're aliases of the same command.

@nlf nlf added Priority 2 secondary priority issue and removed Needs Triage needs review for next steps labels May 31, 2022
fritzy pushed a commit that referenced this issue Jun 22, 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

Co-authored-by: @lukaskuhn-lku
Co-authored-by: @ljharb
@simoneb
Copy link

simoneb commented Aug 25, 2022

This is a breaking change for us, we were relying on the behavior that nothing was output and a successful exit code was returned for a package/version that didn't exist

@alasdairhurst
Copy link

@simoneb conversely, returning nothing is a breaking change for us. It's not clear when the regression was introduced though but it could be reasonable to introduce a feature to opt-in to this behavior.

That said you can always redirect the output to /dev/null and add || true to get the behavior you want :)

On our side, maybe there's another way to detect if a package was not published in one line of bash...

@simoneb
Copy link

simoneb commented Aug 26, 2022

@simoneb conversely, returning nothing is a breaking change for us. It's not clear when the regression was introduced though but it could be reasonable to introduce a feature to opt-in to this behavior.

That said you can always redirect the output to /dev/null and add || true to get the behavior you want :)

On our side, maybe there's another way to detect if a package was not published in one line of bash...

how is a breaking change something that has been that way for a long time?

@alasdairhurst
Copy link

@simoneb You're not wrong in that respect. I wasn't aware how long the unexpected behavior had been around for and wrote some code in the past few months that relied on the new behavior assuming it behaved this way from the start.

@ljharb
Copy link
Contributor

ljharb commented Aug 26, 2022

This is the inevitable struggle - if you're relying on a bug, the bug fix is a breaking change for you.

@simoneb
Copy link

simoneb commented Aug 26, 2022

This is the inevitable struggle - if you're relying on a bug, the bug fix is a breaking change for you.

Who decided it was a bug? Until this issue was reported this was the behavior of that command since a long time and nothing suggested that it was a bug. In the best case it was an undocumented behavior.

@ljharb
Copy link
Contributor

ljharb commented Aug 26, 2022

@simoneb ok, i'll indulge you :-) what would be the failure case for this command if not "nothing was found"?

@simoneb
Copy link

simoneb commented Aug 26, 2022

I accept the new behavior, I'm not implying that it should be changed back to what it was and we have already adapted our affected codebase to handle both old and new behavior. but I thought it was worth reporting as this is a breaking change introduced in a minor semver release.

@ljharb
Copy link
Contributor

ljharb commented Aug 26, 2022

Right - what I'm saying tho is, that since there's no reasonable argument to be made for the previous behavior, only for the new behavior, that's why I'd consider it a bug, no matter how long it's been around.

@simoneb
Copy link

simoneb commented Aug 26, 2022

No argument advocating the previous behavior as right on my side either, just reporting the breaking change, that's all ✌️

@MarshallOfSound
Copy link

Ref: #5035 (comment)

@ljharb this just hit us over in the Electron release team as a result of some semi-automated npm updates our release infrastructure pulled in the new npm and this broke our release logic in a relatively annoying way that was quite hard to deal with.

I agree with everything you said above specifically:

since there's no reasonable argument to be made for the previous behavior, only for the new behavior, that's why I'd consider it a bug, no matter how long it's been around.

but I think there is a line to be drawn that if the old behavior was not entirely unreasonable and could be relied upon, a change to "improve" that behavior should be marked as breaking and ideally would have gone out in npm@9. I.e. I think this move is correct, I wouldn't call it a bugfix though rather a "breaking improvement". The key thing in our case was the expectation that exit code semantics are stable for a given major version of npm, I don't think that's an unfair assumption especially when the exit codes have worked that way since at least npm@2

@ljharb
Copy link
Contributor

ljharb commented Oct 27, 2022

@MarshallOfSound that's a fair counterpoint.

@lukekarrys
Copy link
Contributor

To give a little context around this from the npm side:

Error codes and terminal output are something that npm does not treat consistently currently. This is an area of active improvement and working towards consistency is difficult due to the tradeoff between being hesitant to make changes that could be deemed breaking and not wanting to do more than ~1 major release per year. I understand that this was a case where we were not hesitant enough, and it did cause breakages in the ecosystem. That's something I do take seriously, and I see it as one of the worst case scenario for minor releases like this one.

(As an aside, there's always the option to add a flag, which has its own tradeoffs I'll save for another thread.)

I appreciate both @simoneb and @MarshallOfSound adding their experience to the thread. At the very least, it will help us inform future decisions around stuff like this. To put it on the record here: exit codes are definitely part of the npm semver contract. A relatively recent addition to the docs is a page about logging (#4113). Documenting this would go great on that page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Priority 2 secondary priority issue Release 8.x work is associated with a specific npm 8 release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants