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

[wraithgar]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

See #1109

Fix will be.

diff --git a/lib/outdated.js b/lib/outdated.js
index b3b630421..68002bf19 100644
--- a/lib/outdated.js
+++ b/lib/outdated.js
@@ -88,13 +88,14 @@ class Outdated extends ArboristWorkspaceCmd {
     // sorts list alphabetically
     const outdated = this.list.sort((a, b) => localeCompare(a.name, b.name))
 
-    if (outdated.length > 0)
-      process.exitCode = 1
-
     // return if no outdated packages
     if (outdated.length === 0 && !this.npm.config.get('json'))
       return
 
+    if (!this.npm.config.get('json'))
+      process.exitCode = 1
+
+

Expected Behavior

No response

Steps To Reproduce

No response

Environment

• npm: 7.24.2

[ljharb]

Why not?

[wraithgar]

See #1109

[wraithgar]

I did try to replicate this and npm outdated --json > output piped the outdated content to stdout.

[wraithgar]

This may not be a problem, this PR is where we can find out

[ljharb]

To me it seems identically a problem - or not a problem - regardless of the output format.

It will be very surprising to me if changing the output format has ANY impact on the exit code; the two are not semantically related.

[wraithgar]

Perhaps @boostvolt can elaborate on how what is in place is broken?

[lukekarrys]

It will be very surprising to me if changing the output format has ANY impact on the exit code; the two are not semantically related.

I can confirm that this is not the current behavior, and I agree it shouldn't be. It might have been in the past, if I'm reading #1109 correctly? I would consider #1109 still resolved, since we now output npm outdated --json to stdout.

I think it's worth noting that there were other requests in #1109 to never have npm outdated set the exit code, regardless of --json. That's directly opposed to #2556 which was fixed in #3799. So I think that is what needs discussion here.

[ljharb]

I have no opinion on whether outdated sets the exit code or not, altho i can see the argument for both.

I have a strong opinion that the output format should never dictate the exit code.

[boostvolt]

When I use npm outdated --json with npm 7.24.2 I get: Command failed with exit code 1: npm outdated --json and the generated json is printed as error to the console. But if I use npm outdated everything works fine. On npm 7.24.1 npm outdated --json as well as npm outdated worked without problems.

[lukekarrys]

When I use npm outdated --json with npm 7.24.2 I get: Command failed with exit code 1: npm outdated --json and the generated json is printed as error to the console. But if I use npm outdated everything works fine. On npm 7.24.1 npm outdated --json as well as npm outdated worked without problems.

I'm not able to reproduce that behavior. I see the only output printed to stdout and the only difference being the exit code set by the command. Here's what I'm getting in 7.24.1 vs 7.24.2:

7.24.1

$ npm --version
7.24.1

$ npm outdated --json 2> /dev/null; echo "code: $?"
{
  "@npmcli/arborist": {
    "current": "3.0.0",
    "wanted": "2.10.0",
    "latest": "3.0.0",
    "dependent": "3844",
    "location": "/Users/lukekarrys/Desktop/npm-sandbox/3844/node_modules/@npmcli/arborist"
  }
}
code: 0

$ npm outdated 2> /dev/null; echo "code: $?"
Package           Current  Wanted  Latest  Location                       Depended by
@npmcli/arborist    3.0.0  2.10.0   3.0.0  node_modules/@npmcli/arborist  3844
code: 0

7.24.2

$ npm --version
7.24.2

$ npm outdated --json 2> /dev/null; echo "code: $?"
{
  "@npmcli/arborist": {
    "current": "3.0.0",
    "wanted": "2.10.0",
    "latest": "3.0.0",
    "dependent": "3844",
    "location": "/Users/lukekarrys/Desktop/npm-sandbox/3844/node_modules/@npmcli/arborist"
  }
}
code: 1

$ npm outdated 2> /dev/null; echo "code: $?"
Package           Current  Wanted  Latest  Location                       Depended by
@npmcli/arborist    3.0.0  2.10.0   3.0.0  node_modules/@npmcli/arborist  3844
code: 1

[boostvolt]

I just tried this out as well. If I run the command in the terminal everything works. As soon as I call it as Node.js child process I get the exit code 1 error message.

[lukekarrys]

@boostvolt thanks for the follow up information. As I mentioned in #3871 (comment), this is working as intended right now, so an RFC issue with some details on how this should be changed is the best way forward.