2.6.0 breaks non-string errors

[netzkind]

2.6.0 introduced "Add the breaker name to circuit rejections".

This breaks existing installations where err.message is not a string:

"[Breaker: Basket] [object Object]" is not helpful. Please either check if err.message is a String, or make adding the circuit-name optional.

[awolden]

Thanks for calling this out @netzkind. This simple fix: #99 should resolve this issue. I will publish it soon.

[netzkind]

Thanks, @awolden

In our use-case, message is the response from an upstream-service, and it needs to be processed. Even with the current fix, this breaks our implementation. Would it be possible to include a toggle to disable prefixing?

[SimenB]

Error.message is a string according to the node docs: https://nodejs.org/api/errors.html#errors_error_message. How are you creating the error where message is not a string without breaking that contract?

[netzkind]

The software in question is based on Loopback 2.x. When requesting remote data from a datasource, and the http-response-code is in the "not-good"-range, Loopback will interpret this as an error, but still include the parsed body of the response as message.

While Error.message is defined as "user readable", i think that the output should remain untouched so it remains in a "known parseable" format. This should at least be an option.

[SimenB]

Where in the docs is it defined as "user readable"? It clearly says <string> in node's docs, which is the runtime of this library.

(FWIW, I agree we shouldn't break what worked before, but I don't buy that typeof err.message !== 'string' is correct.)

[netzkind]

@SimenB you are correct, it's not defined as "human readable" - i read that between the lines. But that makes no difference for this issue.

Concerning your "must be string"-argument: ECMA-262 (which is the standard that the V8 in node.js adheres to) defines that Error.prototype.message defaults to an empty string, but does not require it to be a string (see https://tc39.github.io/ecma262/#sec-error.prototype.message). So it's valid to assume that message does not have to be a string, despite it being depicted differently in nodejs-api.

That being said: i still would love to see a feature-toggle/option.

Otherwhise the change introduced in 2.6.0 marks a backwards-incompatible change, which should make the current version not 2.6.0, but 3.0.0.

[SimenB]

I fully support it behind an option 🙂

[awolden]

I agree an option flag is an easy implementation and guards against compatibility issues. I will can have a PR in tomorrow for the change unless @SimenB can get a PR in sooner.

[rodneyrehm]

Brakes is trying to modify the errors thrown by the callbacks it wraps, thereby causing an error itself:

TypeError: Cannot assign to read only property 'message' of object 'GustavError'
    at _execPromise.apply.tap.catch.err (/…/node_modules/brakes/lib/Circuit.js:97:23)
    …

Might I suggest having another look at the Error's thrown by brakes? I'd start with introducing BrakesError to allow easy differentiation from anything that might've been thrown elsewhere:

BrakesError extends Error { … }
CircuitBrokenError extends BrakesError { … }
TimeoutError extends BrakesError { … }
ExecutionError extends BrakesError { … }

And for the original problem here I'd introduce ExecutionError to wrap the error instead of mutating it here - that way we'd be able to access the original, unmodified error at (.e.g) executionErrorInstance.original, but we can still get the context [Breaker: ${this._brakes.name}] is trying to add. If you really like stack traces, you might find composite-error or nested-error-stacks interesting…