uber-archive / multitransport-jsonrpc

JSON-RPC Client (Node.js & Browser) and Server (Node.js) aim at "natural looking" server and client code.
116 stars 22 forks source link

Error is overwritten in /lib/server.js #56

Closed gchudnov closed 10 years ago

gchudnov commented 10 years ago

Hello,

It looks the error is overwritten in the response of server.js. Any additional properties set in the Error are lost:

server.js, line 68-:

if(error) {
                        outObj.result = null;
                        if(error instanceof Error) {
                            outObj.error = {code: errorCode.internalError, message: error.message};
                        } else {
                            ...
                        }
                    }

^^ here only the message is copied. We have additional properties set in Error and they are lost.

Need to create a plain object instead of using an Error to provide additional properties.

Are there any rationale behind the behavior you implemented or is that a bug?

dfellis commented 10 years ago

There were a few reasons:

  1. The JSON-RPC spec defines errors to look this way, so we're following it. :)
  2. Error objects are actually fairly non-standard beasts -- each JS engine attaching its own useful things (like new Error().stack in Chrome) that won't deserialize back in useful ways depending on which JS VM is being used.
  3. Finally, what should the client app do with things like a remote stack trace? In many cases it would simply confuse things during debugging purposes, and in others you probably don't want the internals of your private code visible in the public-facing portion.

So we decided to go with what seemed the safer route: stick to the spec and don't expose internal server behavior by default, but provided the object escape. Note that it's anything that's not an Error object, so you could do something like this:

function VisibleError(message, arbitraryProperty, showStack) {
    this.message = message;
    this.arbitraryProperty = arbitraryProperty;
    if (showStack) this.stack = new Error().stack;
}

...

throw new VisibleError('we have a problem, 'arbitrary!', true);
gchudnov commented 10 years ago

got it, thank you!

Guess, the issue is closed now.