[BUG] Inconsistency between calls to npm on node.js 16.x #3871

nassau-t · 2021-10-10T13:08:55Z

Is there an existing issue for this?

I have searched the existing issues

Current Behavior

A call to npm outdated, if there are outdated packages, throws an exit code status of 1

C:\temp>node
Welcome to Node.js v16.11.0.
Type ".help" for more information.
> require('child_process').execSync('npm outdated -g --depth=0')
Uncaught Error: Command failed: npm outdated -g --depth=0
    at checkExecSyncError (node:child_process:826:11)
    at Object.execSync (node:child_process:900:15) {
  status: 1,
  signal: null,
  output: [
    null,
    <Buffer 50 61 63 6b 61 67 65 20 20 20 20 20 20 20 20 20 20 20 20 20 43 75 72 72 65 6e 74 20 20 20 57 61 6e 74 65 64 20 20 20 4c 61 74 65 73 74 20 20 4c 6f 63 ... 216 more bytes>,
    <Buffer >
  ],
  pid: 14996,
  stdout: <Buffer 50 61 63 6b 61 67 65 20 20 20 20 20 20 20 20 20 20 20 20 20 43 75 72 72 65 6e 74 20 20 20 57 61 6e 74 65 64 20 20 20 4c 61 74 65 73 74 20 20 4c 6f 63 ... 216 more bytes>,
  stderr: <Buffer >
}

C:\temp>npm -v
8.0.0

Expected Behavior

It was working like this in the the node.js 16.x branch until 16.11.

C:\temp>node
Welcome to Node.js v16.10.0.
Type ".help" for more information.
> require('child_process').execSync('npm outdated -g --depth=0')
<Buffer 50 61 63 6b 61 67 65 20 20 20 20 20 20 20 20 20 20 20 20 20 43 75 72 72 65 6e 74 20 20 20 57 61 6e 74 65 64 20 20 20 4c 61 74 65 73 74 20 20 4c 6f 63 ... 216 more bytes>

C:\temp>npm -v
7.24.0

Steps To Reproduce

In this environment...
Windows 10 pro x64. Node.js v16.11.0 (with included npm v8.0.0)
With this config...
A system with global outdated packages.
Run '...'
require('child_process').execSync('npm outdated -g --depth=0')
See error...
Throws a exit status code of 1

Environment

OS: Windows 10 Pro x64
Node: 16.11.0
npm: 8.0.0

The text was updated successfully, but these errors were encountered:

nassau-t · 2021-10-10T13:41:55Z

More details:

Call to npm outdated having outdated global packages

with node 16.11 and default npm 8.0.0

toni@MSI MINGW64 /c/temp
$ node -v
v16.11.0

toni@MSI MINGW64 /c/temp
$ result=$(npm outdated -g --depth=0); echo $?; echo "*$result*"
1
*Package             Current   Wanted   Latest  Location                         Depended by
@types/node         16.10.2  16.10.3  16.10.3  node_modules/@types/node         global
eslint               7.32.0    8.0.0    8.0.0  node_modules/eslint              global
webpack-dev-server    4.3.0    4.3.1    4.3.1  node_modules/webpack-dev-server  global*

with node 16.10 and default npm 7.24.0

toni@MSI MINGW64 ~
$ node -v
v16.10.0

toni@MSI MINGW64 ~
$ result=$(npm outdated -g --depth=0); echo $?; echo "*$result*"
0
*Package             Current   Wanted   Latest  Location                         Depended by
@types/node         16.10.2  16.10.3  16.10.3  node_modules/@types/node         global
eslint               7.32.0    8.0.0    8.0.0  node_modules/eslint              global
webpack-dev-server    4.3.0    4.3.1    4.3.1  node_modules/webpack-dev-server  global*

As you can see, exit status code has changed. When there are outdated packages, npm 7.24.0 exit with an exit status of 0, and npm 8.0.0 exit with an exit status of 1.

Call to npm outdated without having outdated global packages

with node 16.11 and default npm 8.0.0

toni@MSI MINGW64 ~
$ node -v
v16.10.0

toni@MSI MINGW64 ~
$ result=$(npm outdated -g --depth=0); echo $?; echo "*$result*"
0
**

with node 16.10 and default npm 7.24.0

toni@MSI MINGW64 ~
$ node -v
v16.11.0

toni@MSI MINGW64 ~
$ result=$(npm outdated -g --depth=0); echo $?; echo "*$result*"
0
**

So, when there are no outdated packages, both exit with status code 0.

Summary

I understand that exit error code must be 0 if it's ok, or 1-255 if there are an error.
When I call npm outdated and there are outdated packages, there is no failure or error. Why it return a 1 exit code?

The usual way to do that should be to exit with an error code of 0 (if there are no error), and then, if you want to know if there are outdated packages or not, read the return string (as showed on the examples), and if the string is a void string, there are no outdated packages.

ljharb · 2021-10-10T20:28:56Z

This changed in npm v7.24.2 - npm v7.24.1 and below exit zero in this case.

nassau-t · 2021-10-10T21:01:42Z

I'm astonished.
How it's possible that anyone can return an error (exit codes 1-255) when there aren't any error.
In my thought it is a lot more consistent to return an ok (exit code 0) and if someone want to know if there are no results, it's very easy, because the return string would be an empty string.

without outdated packages

$ result=$(npm outdated -g --depth=0); echo $?; echo "*$result*"
0
**

with outdated packages

$ result=$(npm outdated -g --depth=0); echo $?; echo "*$result*"
0
*Package             Current   Wanted   Latest  Location                         Depended by
@types/node         16.10.2  16.10.3  16.10.3  node_modules/@types/node         global
eslint               7.32.0    8.0.0    8.0.0  node_modules/eslint              global
webpack-dev-server    4.3.0    4.3.1    4.3.1  node_modules/webpack-dev-server  global*

ljharb · 2021-10-10T21:12:57Z

@nassau-t that depends on what mental model you have of this command. If it's "get me the results of an outdated check", then it should return 0, because it got the results. If it's "tell me if everything is up to date", then it absolutely should return nonzero when there's nonzero outdated deps.

nassau-t · 2021-10-10T21:30:10Z

I will prove it empirically:

C:\temp>node
Welcome to Node.js v16.11.0.
Type ".help" for more information.
> require('child_process').execSync('npm outdated -g --depth=0')
Uncaught Error: Command failed: npm outdated -g --depth=0
    at checkExecSyncError (node:child_process:826:11)
    at Object.execSync (node:child_process:900:15) {
  status: 1,
  signal: null,
  output: [
    null,
    <Buffer 50 61 63 6b 61 67 65 20 20 20 20 20 20 20 20 20 20 20 20 20 43 75 72 72 65 6e 74 20 20 20 57 61 6e 74 65 64 20 20 20 4c 61 74 65 73 74 20 20 4c 6f 63 ... 216 more bytes>,
    <Buffer >
  ],
  pid: 14996,
  stdout: <Buffer 50 61 63 6b 61 67 65 20 20 20 20 20 20 20 20 20 20 20 20 20 43 75 72 72 65 6e 74 20 20 20 57 61 6e 74 65 64 20 20 20 4c 61 74 65 73 74 20 20 4c 6f 63 ... 216 more bytes>,
  stderr: <Buffer >
}

C:\temp>npm -v
8.0.0

What do you see here? Because I see an error.

In other words, exit status code has nothing to do with what you think or what you expect. It only makes reference to if the execution is ok or there is an error in the execution, nothing more.

ljharb · 2021-10-10T21:34:47Z

Right - but “the execution” is subjective.

nassau-t · 2021-10-10T21:58:12Z

No, it's not subjective.

It's not subjective because an exit status code of 0 is execution were ok, and an exit status code of 1-255 is there were an error on the execution. Because of that, the system says there is an error. It's not me, and neither are you. It's totally objective because that is the rule.

Look at the example above, I have proved it.

ljharb · 2021-10-11T00:14:01Z

There is no rule, it’s merely a convention, and every tool is always free to choose as they see fit.

An analogue might be, when i do rm nonexistentPath, it’s actually very annoying that rm errors there. I’m not actually asking it to succeed when that path becomes unlinked, I’m asking it to make sure that path is unlinked - i don’t actually care if the path existed before or not.

nassau-t · 2021-10-11T15:44:45Z

I will repeat the empirical proof again.

C:\temp>node
Welcome to Node.js v16.11.0.
Type ".help" for more information.
> try {
...   let result = require('child_process').execSync('npm outdated -g --depth=0');
...   console.log('ok');
... } catch (err) {
...   console.log('error');
... }
error
undefined

What do you see here? Because I see an error.
And in every language (java, python...) that you execute that, it will end with an error. Simply because npm outdated has exit with error code 1, that is an error.

I repeat: exit status code has nothing to do with what you think or what you expect. It only makes reference to if the execution is ok or there is an error in the execution, nothing more.

ljharb · 2021-10-11T15:46:23Z

And in one view, when there are outdated packages, there’s an error in execution, and one absolutely and only wants the thrown error in your case.

I’m not actually arguing in favor of the exit code - I’m objecting to the incorrect assertion that there’s an obviously correct answer here.

nassau-t · 2021-10-11T15:53:43Z

An error of execution could be if there weren't an internet connection, or if it was impossible to get the list of packages...

If there are outdates packages, that is not an error of execution. It was possible to get packages, to compare packages, to return a list of outdates packages... There weren't any error execution. If you want to return the string "HEY, THERE ARE OUTDATED PACKAGES!!", you can do it. But you can not return an exit code status of 1 because there hasn't been any error of execution. So you are lying. It must return 0.

ljharb · 2021-10-11T15:56:01Z

What you’re describing is not a rule - not something that anyone “must” do. It’s just a convention.

Accusing someone of lying is a violation of npm’s CoC; assume good faith.

nassau-t · 2021-10-11T15:58:29Z

Name it a rule, a convention... The fact is that all computer languages work this way.

I'm not acussing you of lying, I accusing npm outdated to be lying when it return a status exit code of 1 when there had not been any error at all.

ljharb · 2021-10-11T16:00:10Z

Ah, sorry then, i misread.

All things do not work this way - i use and create plenty of CLI tools that return a nonzero code when the conceptually requested action couldn’t be completed, even if there’s no error in execution. That’s what’s maximally useful, which is much, much more important than ideological purity, especially of something that is merely a convention.

nassau-t · 2021-10-11T16:04:26Z

C:\temp>node
Welcome to Node.js v16.11.0.
Type ".help" for more information.
> try {
...   let result = require('child_process').execSync('npm outdated -g --depth=0');
...   console.log('ok');
... } catch (err) {
...   console.log('error');
... }
error
undefined
> .exit

C:\temp>npm outdated -g --depth=0
Package             Current   Wanted   Latest  Location                         Depended by
@types/node         16.10.2  16.10.3  16.10.3  node_modules/@types/node         global
eslint               7.32.0    8.0.0    8.0.0  node_modules/eslint              global
webpack-dev-server    4.3.0    4.3.1    4.3.1  node_modules/webpack-dev-server  global

npm outdated is lying. There weren't any error because it can return the list of outdated packages, not error connection, not error reading packages... But it have returned 1 in the execution.

I don't have it in my profile (because I think that arguments must prevail) but I'm also a software engineer. I say this because I am not here to make a mess, but because I firmly believe in what I am defending.

nassau-t · 2021-10-11T16:10:52Z

I think that it is very important in a system that all commands works in the same way. It's like unix design. You can put pieces together. But if any command make it's own rules or conventions, it's a mess. Because every single piece work in a different way.

npm outdated can return a 0 or 1, it's not a big problem. The problem is that any language that executes npm outdated are expecting a exit code status of 0 if all works well. So if it return a 1-255 exit code status, they will interpret that there has been an error (network, disk...). So, you can put pieces together, but you will have to handle exceptions in every tool that don't work as expected.

ljharb · 2021-10-11T16:19:48Z

I don’t agree with that expectation. npm ls exits nonzero when the dep graph is invalid - and MUST so it fails CI - but there was no error in execution.

If you have that expectation generically, your expectations are what’s broken.

nassau-t · 2021-10-11T17:03:12Z

I don't use npm ls, but I think that if npm ls can't do their job because there are a condition that don't allow to make it's work, it must return a non zero status code. It's ok with the rules or conventions (0 -> ok, 1-255 -> error).

And I repeat, that are not my expectations, are the expectations of any language, as you can see again in the example. Nodejs throws an error. It's not me that says there is an error. It's nodejs who says me that there is an error because it runs the catch block.

C:\temp>node
Welcome to Node.js v16.11.0.
Type ".help" for more information.
> try {
...   let result = require('child_process').execSync('npm outdated -g --depth=0');
...   console.log('ok');
... } catch (err) {
...   console.log('error');
... }
error
undefined
> .exit

And I repeat, "there ara outdated packages" is only and answer to the question "are there outdated packages?". An answer. Npm outdated can't say: as there are outdated packages I will return an error during the execution, because it's lying. There wasn't any error during execution.

So npm 7.24.0 was working well (on npm outdated), and 8.0.0 isn't.

ljharb · 2021-10-11T18:04:40Z

npm ls's job is to check the graph's validity, and fail if it's invalid. npm outdated's job is, arguably, to check if everything's up to date, and fail if it's not.

lukekarrys · 2021-10-11T19:08:57Z

npm outdated having an exit code of 1 when there are outdated packages was the behavior of npm 4-6. This behavior was unintentionally removed from npm v7 (#1750) and then this regression was fixed in npm 7.24.2 (#3799).

Since this behavior has been requested both ways before (#2556 #3844), I think it would be best if it went through the RFC process so we can discuss it further.

@nassau-t Would you be willing to open an RFC with your reasoning from this issue and we can have further discussion about this?

For now, I am going to close this issue since it will be more productive to have the discussion over there.

nassau-t · 2021-10-11T20:11:38Z

Created RFC npm/rfcs#473

npm/cli#3871

nassau-t added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Oct 10, 2021

nassau-t changed the title ~~[BUG] Inconsistency between npm on node.js 16.x~~ [BUG] Inconsistency between calls to npm on node.js 16.x Oct 10, 2021

nassau-t mentioned this issue Oct 10, 2021

(unintended?) npm@8 breaking change: return status different for npm outdated nodejs/node#40382

Closed

lukekarrys closed this as completed Oct 11, 2021

lukekarrys mentioned this issue Oct 11, 2021

[BUG] npm outdated exit code should not be set with json output #3844

Closed

1 task

dkrnl added a commit to Intecmedia/Intecmedia.Webpack that referenced this issue Oct 23, 2021

npm outdated: fix silent exit code

51e5516

npm/cli#3871

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG] Inconsistency between calls to npm on node.js 16.x #3871

[BUG] Inconsistency between calls to npm on node.js 16.x #3871

nassau-t commented Oct 10, 2021 •

edited

Loading

nassau-t commented Oct 10, 2021 •

edited

Loading

ljharb commented Oct 10, 2021

nassau-t commented Oct 10, 2021

ljharb commented Oct 10, 2021

nassau-t commented Oct 10, 2021 •

edited

Loading

ljharb commented Oct 10, 2021

nassau-t commented Oct 10, 2021

ljharb commented Oct 11, 2021

nassau-t commented Oct 11, 2021

ljharb commented Oct 11, 2021

nassau-t commented Oct 11, 2021

ljharb commented Oct 11, 2021

nassau-t commented Oct 11, 2021

ljharb commented Oct 11, 2021

nassau-t commented Oct 11, 2021

nassau-t commented Oct 11, 2021 •

edited

Loading

ljharb commented Oct 11, 2021

nassau-t commented Oct 11, 2021 •

edited

Loading

ljharb commented Oct 11, 2021

lukekarrys commented Oct 11, 2021 •

edited

Loading

nassau-t commented Oct 11, 2021

[BUG] Inconsistency between calls to npm on node.js 16.x #3871

[BUG] Inconsistency between calls to npm on node.js 16.x #3871

Comments

nassau-t commented Oct 10, 2021 • edited Loading

Is there an existing issue for this?

Current Behavior

Expected Behavior

Steps To Reproduce

Environment

nassau-t commented Oct 10, 2021 • edited Loading

ljharb commented Oct 10, 2021

nassau-t commented Oct 10, 2021

ljharb commented Oct 10, 2021

nassau-t commented Oct 10, 2021 • edited Loading

ljharb commented Oct 10, 2021

nassau-t commented Oct 10, 2021

ljharb commented Oct 11, 2021

nassau-t commented Oct 11, 2021

ljharb commented Oct 11, 2021

nassau-t commented Oct 11, 2021

ljharb commented Oct 11, 2021

nassau-t commented Oct 11, 2021

ljharb commented Oct 11, 2021

nassau-t commented Oct 11, 2021

nassau-t commented Oct 11, 2021 • edited Loading

ljharb commented Oct 11, 2021

nassau-t commented Oct 11, 2021 • edited Loading

ljharb commented Oct 11, 2021

lukekarrys commented Oct 11, 2021 • edited Loading

nassau-t commented Oct 11, 2021

nassau-t commented Oct 10, 2021 •

edited

Loading

nassau-t commented Oct 10, 2021 •

edited

Loading

nassau-t commented Oct 10, 2021 •

edited

Loading

nassau-t commented Oct 11, 2021 •

edited

Loading

nassau-t commented Oct 11, 2021 •

edited

Loading

lukekarrys commented Oct 11, 2021 •

edited

Loading