[RRFC] npm outdated (npm >= 7.24.2) exit with incorrect status code

nassau-t commented 3 years ago

Motivation ("The Why")

npm v8.0.0 (included in node v16.11.0) has an exit status code of 1 when there are outdated packages.

By convention, exit status code must be 0 if there is no execution error. And can be in the range 1-255 if there is an execution error (network, disk, internal error...)

But it has been replaced with a particular behaviour that has no sense with the above convention. Actual behaviour is: exit status code 0 -> there aren't outdated packages exit status code 1 -> there are outdated packages.

So this behaviour creates a problem if npm outdated is executed from a any program, because the program throws an error exception.

Example

From a node.js program, I get the error of the catch block, when from cmd npm outdated show no error, only the list of outdated packages.

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

How

Current Behaviour

So it seems that npm outdated has a status exit code of 0 if there aren't outdated packages. And a status exit code of 1 if there are outdated packages.

Desired Behaviour

npm outdated should have a status exit code of 0 if there haven't been an error (network, disk, internal error...). 1.1. If stdout return an empty string, there aren't outdated packages. 1.2. If stdout have a non empty string. there are outdated packages.
npm outdated should return a status exit code of 1 if there have been an error (network, disk, internal error...).

Examples of 1.1. and 1.2., you know that npm outdated has ended ok, and there is very simple to know if there are outdated packages or not.

Example of 1.1.: Execute ok, there are no outdated packages.

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

Example of 1.2.: Execute ok, there are 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*

References

Java 7 docs https://docs.oracle.com/javase/7/docs/api/java/lang/Runtime.html#exit(int) public void exit(int status) The argument serves as a status code; by convention, a nonzero status code indicates abnormal termination.
Python 3 docs https://docs.python.org/3/library/sys.html#sys.exit zero is considered “successful termination” and any nonzero value is considered “abnormal termination” by shells and the like. Most systems require it to be in the range 0–127, and produce undefined results otherwise. Some systems have a convention for assigning specific meanings to specific exit codes, but these are generally underdeveloped; Unix programs generally use 2 for command line syntax errors and 1 for all other kind of errors.
The Linux Documentation Project https://tldp.org/LDP/abs/html/exitcodes.html Exit Code Number 1 Meaning Catchall for general errors
Unix and linux system administration handbook 5. Evi Nemeth, Garth Snyder, Trent R.Hein, Ben Whaley, Dan Mackin. init (or systemd) also plays another important role in process management. When a process completes, it calls a routine named _exit to notify the kernel that it is ready to die. It supplies an exit code (an integer) that tells why it’s exiting. By convention, zero indicates a normal or “successful” termination.
Modern Operating Systems 4th Edition. Andrew S. Tanenbaum. It has one parameter, the exit status (0 to 255), which is returned to the parent in the variable status of the waitpid system call. The low-order byte of status contains the termination status, with 0 being normal termination and the other values being various error conditions.

iarna commented 3 years ago

There are many examples of standard unix tooling (grep for example) that use exit codes to indicate results other than system errors. This is critical to supporting composition in shells. This is why throwing on status codes is incorrect behavior and should be avoided, imo. (This is apparently a contentious opinion, given how many libraries do this, but no, please stop.)

I should also note that this is consistent with the behavior npm 6 and all previous versions.

nassau-t commented 3 years ago

Yes, grep and cmp for example don't use the conventional exit status codes.

But exit status code indicates if a execution was ok (code 0), or if it was any error with the execution (codes 1-255). It wasn't designed to indicate anything more than that. So these programs are faking an error. Unfortunately as npm outdated. The next code example is the empirical proof of what I'm saying.

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

It throws an error, only because there are outdated packages. That's the fact. It throws an error!

Exit status code is a convention because all aplications in a system can work on the same way. If you broke that convention, you must handle exceptions for every application that do this.

And as I showed before, there are other ways to retrieve if there are results or not, not really difficult to program, and without breaking any convention.

nassau-t commented 3 years ago

And I want to remember that the npm that came by default with node v16.x worked fine (in my opinion) until the last release v16.11.

nassau-t commented 3 years ago

So, it seems that instead of using

const execPromisified = util.promisify(require('child_process').exec);

Now, we can use

function execAsync (command) {
  return new Promise(function (resolve, reject) {
    exec(command, (error, stdout, stderr) => {
      if (stderr !== '') {
        reject(stderr);
      } else {
        resolve(stdout);
      }
    });
  });
}

To handle the exceptions on the calls to npm outdated.

As can be seen, more simple and smart... (ironic)

nassau-t commented 3 years ago

Perhaps another option could be a conventional exit status codes, and own exit status codes with a --ownCodes parameter.

nassau-t commented 3 years ago

A clear, smart program:

const util = require('util');
const execPromisified = util.promisify(require('child_process').exec);

async function main() {
  try {
    const resultat = await execPromisified('npm outdated -g --depth=0');
    console.log(resultat);
  } catch (err) {
    console.log(err);
  }
}

main();

That fails, evidently.

C:\temp>node p1.js
Error: Command failed: npm outdated -g --depth=0

    at ChildProcess.exithandler (node:child_process:397:12)
    at ChildProcess.emit (node:events:390:28)
    at maybeClose (node:internal/child_process:1064:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:301:5) {
  killed: false,
  code: 1,
  signal: null,
  cmd: 'npm outdated -g --depth=0',
  stdout: 'Package             Current   Wanted   Latest  Location                         Depended by\n' +
    '@types/node         16.10.5  16.10.8  16.10.8  node_modules/@types/node         global\n' +
    'ts-node              10.2.1   10.3.0   10.3.0  node_modules/ts-node             global\n' +
    'typescript            4.4.3    4.4.4    4.4.4  node_modules/typescript          global\n' +
    'webpack-dev-server    4.3.0    4.3.1    4.3.1  node_modules/webpack-dev-server  global\n',
  stderr: ''
}

2nd attempt, with a particular exception handled

const exec = require('child_process').exec;

function execAsync (command) {
  return new Promise(function (resolve, reject) {
    exec(command, (error, stdout, stderr) => {
      if (stderr !== '') {
        reject(stderr);
      } else {
        resolve(stdout);
      }
    });
  });
}

async function main() {
  try {
    const resultat = await execAsync('npm outdated -g --depth=0');
    console.log(resultat);
  } catch (err) {
    console.log(err);
  }
}

main();

More clear, as seen (ironic), but hey, it works!!!

C:\temp>node p2.js
Package             Current   Wanted   Latest  Location                         Depended by
@types/node         16.10.5  16.10.8  16.10.8  node_modules/@types/node         global
ts-node              10.2.1   10.3.0   10.3.0  node_modules/ts-node             global
typescript            4.4.3    4.4.4    4.4.4  node_modules/typescript          global
webpack-dev-server    4.3.0    4.3.1    4.3.1  node_modules/webpack-dev-server  global

Never forget plan B, whatever Schwarzenegger say ;)

nassau-t commented 3 years ago

There are many examples of standard unix tooling (grep for example) that use exit codes to indicate results other than system errors. This is critical to supporting composition in shells. This is why throwing on status codes is incorrect behavior and should be avoided, imo. (This is apparently a contentious opinion, given how many libraries do this, but no, please stop.)

I should also note that this is consistent with the behavior npm 6 and all previous versions.

I have to admit that I am very disappointed with the way the npm RFC issue works. I believed that the discussion would be dealt from a technical skill rather than ego. The only thing you contribute is "but grep works like this". And building a smokescreen (as it doesn't exist) to the code example that I posted that demonstrates this malfunction.

iarna commented 3 years ago

You... you know that I'm just a member of the public, right? You say "the npm RFC issue" but you're just disagreeing with another node user. (I was affiliated with the project previously but that was like 3 years ago now.) I have exactly as much weight here as you do.

I don't have a strong opinion for outdated in particular, because being notified in a shell scriptable way that some modules are outdated isn't obviously useful to me? But the user base size being the size that it is, you can be pretty sure any change will break someone. (As evidenced by your experience.) So caution is always warrented.

That said, I do have the opinion that unintentional breaking changes should be reverted, which is what happened to break you. As a node user I'd want a stronger justification to break from 10 years of behavior than an appeal to standards.

nassau-t commented 3 years ago

Well, It was not really with you. You have just an opinion, like me. My reply was for the three guys that said ok to your post. I expected some thoughts, not only an ok. Sorry for the misunderstood.

I saw that npm outdated were not working well. So I thinked this should be fixed up. There are simple solutions to follow the exit status code convention and without breaking nothing. For example, following the exit status code convention, and if you want to broke things, just add a --ownExitCodes parameter, and then npm outdated can return whenever you want and do another type of things, to be able to parse it in this way. But broken a default expected behaviour with the rest of the programs in a system (as seen, executing the catch block without really having any error) is not the way to go.

I just have used Node 16.x (with default npm), and it was working fine until 16.11. So, for me, npm 7.27.2 it's a breaking behaviour. But anyway, I think (my opinion is) that 10 years of bad behaviour can be fixed up, satisfaying conventions of the whole system. But wait, this was fixed on npm v7, so it was possible to change behaviour with a major release. The weird is that v7 worked in a way (ok for me) and suddenly it changed on 7.27.2. That is very rare, to broke the behavior in the end of 7.x.

iarna commented 3 years ago

Ah, gotcha, yeah, there was a discussion about this at the RFC meeting (the one that happens every Wednesday and that anyone can join!), and I imagine they thumbs upped in part as a result of that. We did discuss it for quite a while.

The change in 7.27.2 was, I'm sure, to their minds, fixing a bug, a regression introduced with the launch of 7.x. But I'm hopeful that if they'd realized it was going to break anyone that they'd have held off till 8. It can be hard to tell sometimes.

Honestly, I've only used npm 7 on personal projects because it breaks all my work repos, so I feel ya. (The other major topic at the RFC meeting was reversing a decision to call the behavior that broke my work environment "working as intended".)

nassau-t commented 3 years ago

I didn't know that I could attend the meeting. Anyway, I don't think anything would have changed. Things have to be discussed before the meetings.

The change in 7.27.2 was to return to a bad behaviour, so it should be avoided. v7 was a new big version, so it was a great moment to do that.

As I have show in the code above, I can use my execAsync to escape from that bad behaviour. I think that you could done something similar in your code (I try it and post in this rfc, and it isn't difficult to catch if a command return results or not). I was not thinking on me in this rfc. I was thinking in the future of npm and in fulfill with standards.

nassau-t commented 3 years ago

Let's review:

Exit status exit code convention, example that throws an error when there are not error, a v7 (big version, so it could change behaviour) and it was doing it, a revert to working bad at a 7.27.2 version, not a particular interest for me otherwise a npm that fulfill with standards (so it could be integrated with other programs)...

The truth is, the more we talk, the more I see that I am right.

lukekarrys commented 2 years ago

As was discussed in the RFC Meeting on 2021-10-13 (https://github.com/npm/rfcs/blob/main/meetings/2021-10-13.md#issue-473-rrfc-npm-outdated-npm--7242-exit-with-incorrect-status-code---nassau-t) this is working as intended. I plan to document this as part of the doc we discussed in the 2022-02-23 meeting around npm ideologies with output and logging.

kmturley commented 2 years ago

I agree this is really odd behavior...

Use-case:

Install package if it is not already installed
Upgrade package if it is outdated

// check whether the package is installed
npm list my-package --json

  // if not installed then install it
  npm install my-package --json

  // else check if package is outdated
  npm outdated my-package --json

     // if out-of-date then update
     npm update my-package --json

This logic breaks at npm outdated because the script Exit code is 1

@nassau-t solution is a good workaround to make it work!

EDIT: removed global flags

ljharb commented 2 years ago

It doesn’t make sense to run outdated globally; there’s no package.json to check.

kmturley commented 2 years ago

I have removed the global flags from the example code, the use-case still stands

omidantilong commented 2 years ago

@nassau-t thankyou for the suggested workaround - this behaviour was driving us absolutely nuts. After reading through this thread I can see the logic from both sides but I completely agree that sending a failure code when the command has executed successfully is very confusing.

It would be great if this could be made configurable somehow.

sir-gon commented 2 years ago

I'm trying to save some common commands of my development flow to a Makefile and just ran into the same behavior of "npm outdated" with exit code 1, which breaks my task in the Makefile.

npm --version                                                                                                                                                       22-06-22 / 
8.11.0

node --version                                                                                                                                                      22-06-22 / 
v18.3.0

AndreCunha1 commented 1 year ago

My scripts also broke and finally I found out why ☹️

Back in programming classes, I was taught that return exit code != 0 means execution error (which "proves" nothing, of course). Successfully reporting outdated packages does not seem like an execution error to me.

I like the suggested opt-in approach of a parameter (e.g. --ownExitCodes or whatever name fits best) to control this behavior.

PS: npm audit also has this same behavior and, although it is documented, should probably be considered together with npm outdated for consistency. Maybe there are more commands with similar behavior.

ljharb commented 1 year ago

@AndreCunha1 that's never been the case; for example, ls or grep return nonzero when they don't find a match, which is a successful execution that didn't find anything.

AndreCunha1 commented 1 year ago

@ljharb this was already mentioned by @iarna here and it did not change my opinion.

ljharb commented 1 year ago

Of course you are entitled to your opinion on what is better, but what you were taught is objectively incorrect because of the existence proof of these other tools. The mental model of exit codes is "what programs DO" and not "what people think programs SHOULD do", and it's important for npm to fit the prevailing mental model.

nassau-t commented 1 year ago

I opened this issue, and I think I sufficiently motivated it, citing bibliography. So, it's not only my opinion. Still I think @AndreCunha1 is right. @ljharb "objectively incorrect" is really "completely subjective": It's like justifying drug use because there are people who use drugs. And this isn't "what programs DO" neither "what people think programs should do". It's a convention on how programs should work.

AndreCunha1 commented 1 year ago

"what you were taught is objectively incorrect because of the existence proof of these other tools"

So anything can be proven incorrect by writing a tool that does otherwise?

Note that I am not implying that what I was taught is correct either, hence my previous "proves nothing" there.

Anyway, I am happy to find the cause of my broken scripts and have already adjusted them. Another successful outcome of this discussion could be to simply update the npm outdated documentation to explain this behavior, just like the documentation of npm audit already does.

wallyhall commented 1 year ago

I might be missing the obvious here - but is a (what seems to me perfectly reasonable middle-ground) straightforward option not to continue returning 1 if outdated packages are found, but to ensure any error scenario is >1?

Certainly, for us, we want (and arguably need) to validate if a problem occurred. I don't mind if the exit code is either 1 or 0 for success, provided non-successes are distinguishable.

chrillep commented 1 year ago

run npm outdated || true and it will always return exitcode 0 ?

xavierfoucrier commented 1 month ago

After trying a lot of things, I came to the same conclusion as many peoples in this thread: use a custom Promise base handler instead of trying to promisify the exec method. Using || true is also a good alternative as the method always return json / empty json. Sad to says that returning 1 as error code when there are outdated dependencies is clearly the right way to manage error code in programs.

I guess there could be an alternative with some arguments to the npm outdated method and I hope in a near future to see more flexibility on this.

😎

tiennou commented 3 weeks ago

npm outdated --json || true doesn't work if I'm interested in the output, since that will cause execSync to barf out an error and never provide me with the JSON.

And I can't fathom switching to the promisified-exec variant, because the code I'm in is sync code, and adding async to it would need me to recolor all the functions I'd be calling it from.

My suggestion would be to add an npm outdated --check that returns 1 if there's outdated dependencies and no output, for the tooling that wants the check done, then make the non-check cases return 0 unless there's an actual problem reporting the outdatedness (like network failure, or invalid package.json), as a user typing the command on their terminal doesn't exactly care about the error status.

npm / rfcs