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

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

Closed
nassau-t opened this issue Oct 9, 2021 · 21 comments
Closed
Labels
npm Issues and PRs related to the npm client dependency or the npm registry.

Comments

@nassau-t
Copy link

nassau-t commented Oct 9, 2021

Version

v16.11.0

Platform

Microsoft Windows NT 10.0.19042.0 x64

Subsystem

No response

What steps will reproduce the bug?

require('child_process').execSync('npm outdated -g --depth=0')

How often does it reproduce? Is there a required condition?

every time

What is the expected behavior?

>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>

What do you see instead?

>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 >
}

Additional information

No response

@targos targos added npm Issues and PRs related to the npm client dependency or the npm registry. v16.x labels Oct 9, 2021
@targos
Copy link
Member

targos commented Oct 9, 2021

/cc @nodejs/npm

It seems that npm@8 now exits with code 1. npm@7 exited with code 0.

@Mesteery

This comment has been minimized.

@nassau-t
Copy link
Author

nassau-t commented Oct 9, 2021

C:\temp>npm -v
8.0.0

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
webpack-dev-server 4.3.0 4.3.1 4.3.1 node_modules/webpack-dev-server global

@nassau-t

This comment has been minimized.

@Mesteery

This comment has been minimized.

@nassau-t

This comment has been minimized.

@mscdex
Copy link
Contributor

mscdex commented Oct 9, 2021

Most likely the non-zero exit code is intended for use in scripting/automation to reliably know if there are outdated dependencies without having to try to parse the output, whose format could easily change.

Also you could look at it this way if you view all non-zero exit codes as "failures": npm failed to find no outdated dependencies.

@nassau-t

This comment has been minimized.

@mscdex
Copy link
Contributor

mscdex commented Oct 9, 2021

From my experience, non-zero exit codes for non-errors is very common in the *nix world. Even common utilities like grep do this (e.g. to signal if a match was made or not).

While Bash and others may have recommended meanings for various exit codes, there is no universal standard that everyone is required to follow. I would say the only commonly agreed upon meaning is for an exit status of 0, which can still be a bit vague but usually indicates some sort of positive ("success"/"good"/"ok") signal (again even in the case of grep it means one or more lines were matched, which can be viewed as a positive signal).

Since the change in npm seems to have originated in a new major version (of npm) and npm still uses semver, it seems a sensible breaking change to me. Perhaps the course of action here would be to submit an issue on the npm CLI issue tracker to ask for this particular change to be noted in their v8.0.0 changelog.

@nassau-t

This comment has been minimized.

@nassau-t

This comment has been minimized.

@Trott
Copy link
Member

Trott commented Oct 9, 2021

Off-topic stuff about grep return codes

This is a nonsense. If there is no error, it must exit with code 0.

Non-zero return status can indicate error but can also indicate other non-error failures. A well known example, and one that seems parallel to the case here, is grep, which returns 0 if it finds a match, 1 if it doesn't find a match (failure, but not an error), and 2 for errors.

$ grep bar README.md ; echo $?
bar
0
$ grep foo README.md ; echo $?
1
$ grep ajdkflajsdkl ajdkfljasdklfj ; echo $?
grep: ajdkfljasdklfj: No such file or directory
2
$ 

I'm going to close this because it is in the wrong repository. You may open this issue in https://github.com/npm/cli if you want to argue that npm outdated must return error codes according to your expectations. Node.js will use whatever npm hands it as the return status.

@Trott Trott closed this as completed Oct 9, 2021
@Trott Trott added the wrong repo Issues that should be opened in another repository. label Oct 9, 2021
@targos targos reopened this Oct 9, 2021
@targos
Copy link
Member

targos commented Oct 9, 2021

I'd like to keep this open because npm 8 was rushed into Node.js 16 with this message:

The purpose of this release is to drop support for old node versions and
to remove support for require('npm'). There are no other breaking
changes.

@Mesteery Mesteery removed the wrong repo Issues that should be opened in another repository. label Oct 9, 2021
@Trott
Copy link
Member

Trott commented Oct 9, 2021

I'd like to keep this open because npm 8 was rushed into Node.js 16 with this message:

The purpose of this release is to drop support for old node versions and
to remove support for require('npm'). There are no other breaking
changes.

Works for me. I certainly agree it's a breaking change!

@nassau-t

This comment has been minimized.

@nassau-t
Copy link
Author

nassau-t commented Oct 10, 2021

In parallel, I have opened an issue on npm site npm/cli#3871

@Trott Trott changed the title exec npm fails (unintended?) npm@8 breaking change: return status different for npm outdated Oct 10, 2021
@Trott
Copy link
Member

Trott commented Oct 10, 2021

I hid a bunch of comments here as off-topic. (@mscdex, I didn't hide yours because you're a Collaborator and subject to different rules, but if you feel like hiding or editing yours to keep this concise and focused, please do.) The issue at hand is that the return status is different between npm@7 and npm@8, and that that is apparently an unintended breaking change. Whether or not the changed behavior is correct is immaterial.

@ljharb
Copy link
Member

ljharb commented Oct 10, 2021

To clarify: this behavior did not actually change in npm v8; it changed in npm v7.24.2. In other words, it would have happened even if node had updated to npm 7 latest and not npm 8. npm v7.24.1 and below do not exit nonzero in this scenario.

@nassau-t
Copy link
Author

Created RFC npm/rfcs#473

@Trott
Copy link
Member

Trott commented Oct 11, 2021

From npm/cli#3871 (comment):

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.

As @nassau-t indicates in a previous comment, they've opened an RFC in the npm repository about it.

Given all this, I'm going to close this again. Because this wasn't a change in npm 8 (and therefore is not a result of #40369) and is not an issue with Node.js but with npm, the npm org is the right place for this to be discussed and for action (if any) to be taken.

@targos If you disagree and still think this should remain open, by all means, re-open this issue. Thanks!

@Trott Trott closed this as completed Oct 11, 2021
@nassau-t
Copy link
Author

If someone else have to handle this exception...

BAD SOLUTION: 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: ''
}

GOOD SOLUTION: 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Issues and PRs related to the npm client dependency or the npm registry.
Projects
None yet
Development

No branches or pull requests

6 participants