Returning errors from Node-side code should be more consistent

[peterflynn]

Right now we have several approaches to signaling errors in code that ran on the node side:

1. Invoke callback with first arg an Error object _(used in ExtensionManagerDomain.cmdInstall())
2. Invoke callback with first arg something else (e.g. an enum string indicating an error code, or an array including an error code and additional info) _(used in StaticServerDomain._createServer(), ExtensionManagerDomain.cmdDownloadFile(), etc.)
3. Invoke callback with first arg null, and second arg contains an error, errors, etc. property _(used in ExtensionManagerDomain.cmdInstall())

All three have some downsides:

1. ConnectionManager.sendCommandError() doesn't properly serialize Error objects (because they don't JSON.stringify() nicely). We'd also need an expando convention or standard subclass if we wanted to pass any info beyond a string error code.
2. Passing something other than an Error as the first arg seems uncommon in the Node world.
3. It feels counterintuitive to pass a success object in the case of failure, and it means client code needs per-command logic to decide whether to call resolve() vs. reject(). (See Package.installFromURL() for an example -- there are three different ways that install() can lead to a reject()). This also seems fairly uncommon in Node.

Extension install/validation is an especially tricky use case because it has some gray areas. For example, if you call validate() and it is able to correctly unpack the ZIP file but it finds that the extension inside it is invalid, is that a successful validation that returns errors or is it a failure? (I think Kevin and I would both say the former). But if you call install() and it decides to abort because the extension failed validation, that is probably a failure not a success. But what if the extension was 'successfully installed' into the disabled folder? Is that success, or failure? (I think I'd argue that should be a failure in both cases, since the ultimate goal of the action -- installing an extension that we can then attempt to invoke -- did not actually succeed. Distinguishing partial failure (disabled folder) vs. complete failure (we didn't unpack the ZIP anywhere at all) is still valuable but that could still be done via properties or error codes on the failure object).

[dangoor]

That's a great summary, Peter.

One concrete thing that I think we both agree on: it should be acceptable to call the callback in Node with an Error object. I'd be inclined to start with a convention for how an Error object is serialized (and which properties on it we look for automatically).

Beyond that, I look at callback(new Error("foo"), null) as being equivalent to throw new Error("foo"). A little bit of googling didn't turn up whether anyone else views it that way. I kind of get the feeling that others don't see it that way, perhaps instead viewing it more like error return values in C.

I think the distinction you make between installation and validation is reasonable, and it would be okay to change _cmdInstall to return error conditions for any case that does not actually result in the extension being successfully installed. (That would be a clear-cut signal to the client side code to go ahead and turn on the extension.)

[pthiess]

Reviewed - Medium Priority @dangoor