Rewrite error serialization #61
Conversation
FrederikBolding
force-pushed
the
fb/fix-error-serialization
branch
4 times, most recently
from
4fac448 to
d92428b
Compare
FrederikBolding
force-pushed
the
fb/fix-error-serialization
branch
from
132112f to
5a145b0
Compare
| // If the error does not match the JsonRpcError type, use the fallback error, but try to include the original error as `cause` | ||
| const cause = serializeCause(error); | ||
| const fallbackWithCause = { | ||
| ...fallbackError, |
There was a problem hiding this comment.
drops non enumerable properties
There was a problem hiding this comment.
What would you prefer then? Setting enumerable to true for fallbackError?
There was a problem hiding this comment.
In my opinion, this wouldn't be an issue since we specify the fallback error and it includes the cause that may still have a stack.
There was a problem hiding this comment.
Presumably what we'd want to do here is preserve the fallbackError as much as possible, mutating it only to add the cause property.
In this case the fallback error is already expected to be a "serializable object" though, right? So maybe we don't need to worry about non-enumerable properties. Though the JSDoc description should certainly clarify that expectation if that is the case.
There was a problem hiding this comment.
Yeah, I would expect it to be serializable according to the type. I can add it to the doc string
|
New dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No new dependency issues detected in pull request Bot CommandsTo ignore an alert, reply with a comment starting with Pull request alert summary
📊 Modified Dependency Overview:
|
#158) This ensures that non-empty string `error.message` properties of serialized errors are preserved by default, even if the serialized error is not [a valid JSON-RPC error](https://www.jsonrpc.org/specification#error_object). This behavior can be overridden by setting `shouldPreserveMessage: false`. In #61, our error serialization logic was considerably improved. One of the behavioral changes made at the time was to always overwrite the `message` property with that of the fallback error (practically always the "internal JSON-RPC-error"), regardless of whether a non-empty string message was present on the original error object. We have yet to ship this everywhere in our stack, in part because such a change may be breaking for our consumers. By reverting to the old behavior for the `message` property only, we avoid these potential breakages and improve the accessibility of potentially useful information to consumers (i.e. directly in the error message as opposed to buried in `error.data.cause.message`).
Rewrite error serialization to allow any error that conforms to the
JsonRpcErrorJSON-compliant type.If the error does not conform to this type, it will be wrapped in an Internal RPC error and the original error will be included as the
data.cause. Any non JSON-compliant props of the error will be removed.Fixes #51