reject on return code > 0

[TanninOne]

I have a problem with these lines:

if (code > 0) {
  reject(new Error('Exited with code ' + code));
  return;
}

This assumes that an exit code of > 0 signals an error and other exit codes (0 or negative) signal success.
For one thing: Yes, the exit code is a signed integer and convention says everything other than 0 signals an error.
Second: It's purely a convention that an exit code of 0 signals success, not a standard. Depending on the OS and environment it could just as well signal true, false or "nothing has been processed". An exit code of 5 could signal error or it could signal "I have processed 5 items" or "main() had no return because the programming tutorial said the function signature is 'void main()' and 5 is what happened to be on the stack".

Especially on Windows far too many applications don't follow the 0=success convention and since opn opens a default application we can't know if the application that does get run follows convention, so what is the rejection good for
Essentially as soon as windows is one of your target platforms you have to catch and ignore rejection because there is a good chance it rejects for no reason, but thereby also ignore actual errors reported by the error callback.

[kevva]

Have you encountered any problems with it? I don't think we should remove these lines because they are valid and I don't see why we should cater to applications that have bad exit codes.

[TanninOne]

Yes, I do encounter problems with users reporting non-zero exit codes when the application ran fine, which is why I'm now forced to suppress exceptions from opn entirely or change this in my fork.

And again, there are no "bad exit codes", there is no rule that says an application has to return 0 on success.
And you wouldn't be catering to applications that return "bad" exit codes, you would be catering to developers who try to develop for the real world where those applications exist.
The point is: I don't control what applications users have set up as their default. I don't control what those applications return for their exit code.
What the user sees is that they get error messages running applications that work perfectly fine, that neither cmd nor the windows explorer nor any other windows application would ever report as having a problem.

So again, that leaves me, as someone who doesn't return "bad" exit codes in their own code in the crappy situation that I have to either suppress actual errors as colateral or annoy users.
How does that make sense?

At least make it optional or throw a separate exception type so I can suppress more granular via instanceof?

[E3V3A]

We also have a problem with this. Running this code results in:

(node:3869) UnhandledPromiseRejectionWarning: Error: Exited with code 3
    at ChildProcess.cp.once.code (/home/pi/MagicMirror/modules/MMM-Assistant/node_modules/opn/index.js:84:13)
    at Object.onceWrapper (events.js:272:13)
    at ChildProcess.emit (events.js:180:13)
    at maybeClose (internal/child_process.js:936:16)
    at Socket.stream.socket.on (internal/child_process.js:353:11)
    at Socket.emit (events.js:180:13)
    at Pipe._handle.close [as _onclose] (net.js:538:12)

(node:3869) UnhandledPromiseRejectionWarning: Unhandled promise rejection. 
This error originated either by throwing inside of an async function without a catch block, 
or by rejecting a promise which was not handled with .catch(). (rejection id: 1)

(node:3869) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. 
In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Maybe this could help?

[E3V3A]

Is there any workaround for removing these messages?

[E3V3A]

@sindresorhus
Can you please get back to us, if you have any updates on insights on this issue?
(or if we're barking up the wrong tree?)

[sindresorhus]

If anyone does a good PR to add an option to turn this behavior off, I'll merge it.

[dev-z]

Getting a similar problem when trying to run a vue.js project created with the vue-cli tool using the webpack template. @sindresorhus can you please have a look.

Listening at http://localhost:80

(node:8631) UnhandledPromiseRejectionWarning: Error: Exited with code 3
at ChildProcess.cp.once.code (/home/ishtiaque/MyWeekendProject/app-ui/node_modules/opn/index.js:84:13)
at Object.onceWrapper (events.js:317:30)
at emitTwo (events.js:126:13)
at ChildProcess.emit (events.js:214:7)
at maybeClose (internal/child_process.js:925:16)
at Socket.stream.socket.on (internal/child_process.js:346:11)
at emitOne (events.js:116:13)
at Socket.emit (events.js:211:7)
at Pipe._handle.close [as _onclose] (net.js:567:12)
(node:8631) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a pro
mise which was not handled with .catch(). (rejection id: 1)
(node:8631) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

[E3V3A]

@sindresorhus

If anyone does a good PR to add an option to turn this behavior off, I'll merge it.

We'd love to help, but we are probably not capable of doing so, which is why we reported the issue instead of fixing it. If you are the maintainer and writer of this library, you may want to reconsider, otherwise I'm afraid this could take months, before the right people happen to stop by here and both know how to and want to fix it for you.

Thanks in advance.

[dev-z]

@sindresorhus As far as I understood after going through the code, the error is thrown not because of opn, but because of the library which uses it. The code which requires opn, does not handle this rejection. In my case, the error was in the webpack configuration.
The original code went something like this:

opn(uri).then((err) => {
    console.error(err);
})

As you can clearly see, it does not handle the rejection, hence that warning is shown. If I change the code to:

opn(uri).then((res) => {
    console.info(res);
}, (err) => {
    console.error(err);
});

Then, the warning just goes away.

One other solution could be to resolve any exit code instead of resolving only exit code 0 while rejecting others. Change the following code:

cp.once('close', code => {
    if (code > 0) {
	reject(new Error('Exited with code ' + code));
    } else {
	resolve(cp);
    }
});

to

cp.once('close', code => {
    resolve(cp);
});

[TanninOne]

@dev-z The problem is that the caller can't distinguish between a rejection caused by "error code was > 0" and an "actual" problem so the caller has to either catch all error and ignore them or risk reporting a non-issue to the error log.
And this ambiguity is caused by opn.

[dev-z]

@TanninOne Thank you for explaining it to me. So what should be the pattern/approach followed to avoid such ambiguities?

[TanninOne]

Well, there is different approaches, depending on how you see the problem. I come from different programming languages where it's common to throw different exception types to signal different things, so intuitively I'd say: Throw a "NonZeroReturnCode" exception if the return code is not zero and a regular "Error" when it's a node error.
Then the caller can use "if (err instanceOf ...)" to handle the errors separately. Minimum amount of change, all existing code would work as before (assuming NonZeroReturnCode extends Error)

However, this is still a hackish solution because it ignores the underlying misconception. The point of opn is to call the user-configured default application so by its nature opn does not know what application it starts and what return codes that application uses to signal what problem (if any). Neither does the caller because if he knew what application he's starting, why use opn in the first place?
So by it's nature, using opn makes the return code useless, so why interpret it at all? Looking at the return code breaks the black box opn is supposed to establish.
Apart from shell tools where you need the return code to pipe commands together, no application will use it to signal anything of importance to the user.
Opn is used to open visual tools: web browsers, text editors, image editors, ... None of these tools need the return code to signal anything to the user, they will do it themselves and be much better at it.
The best any application using opn can do is to say "Something, i don't know what, went wrong" after either the application explained to the user what exactly went wrong, so we're just annoying the user telling him what he already knows but with less detail, or we tell him about a problem when the application, which - again - knows better what the problem was, felt it wasn't important enough to tell the user, so we're reporting a non-issue.
I'd actually be very interested in hearing a use case where anyone needs/wants opn to report an error on non-zero return code.

I just realize: All this is assuming opn actually reports the exit code of the application, not the exit code of the application starter (xdg-open, start, ...) in which case the return code might actually be interesting but would have to be mapped to a common error type such that the caller can handle the error in a platform-independent manner.

[E3V3A]

Yeah, we have a use case where we are trying to get an authentication token from the user. They are of course totally unknowing about errors and we'd like to provide some better feedback.

[sindresorhus]

For anyone that wants to work on this, see previous attempt in #161.