Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use serialize-error module (Error#cause support) #2392

Closed
fregante opened this issue Jun 8, 2022 · 8 comments
Closed

Use serialize-error module (Error#cause support) #2392

fregante opened this issue Jun 8, 2022 · 8 comments

Comments

@fregante
Copy link

fregante commented Jun 8, 2022

I noticed that RTK’s error serialization is rather simple:

export const miniSerializeError = (value: any): SerializedError => {
if (typeof value === 'object' && value !== null) {
const simpleError: SerializedError = {}
for (const property of commonProperties) {
if (typeof value[property] === 'string') {
simpleError[property] = value[property]
}
}
return simpleError
}
return { message: String(value) }
}

For example this does not support nested errors, like the new cause property: https://v8.dev/features/error-cause

I know RTK supports specifying a custom serializeError as an option, but it'd be great to support the cause property out of the box, perhaps using an existing solution for error serialization:

@phryneas
Copy link
Member

phryneas commented Jun 8, 2022

Honestly, this is working exactly as intended here.

createAsyncThunk knows two different kinds of errors:

  • known errors - this would be this kind of error: you know that they will happen, you know they can have a specific structure. You should handle those in your code and use rejectWithValue.
  • unknown errors - this could be an access to undefined.something, an out-of-bounds array access, a bundler error that only surfaces at runtime or a string that is being thrown somewhere in a library you are consuming and that you don't expect. We don't know anything about those errors. So the code you are pointing to tries to get them into a "predictable shape" so they can be at least handled at some way in the end.

So in the end: you should probably handle those errors and use rejectWithValue, or your well-known errors will get mixed up with unpredictable stuff.

@markerikson
Copy link
Collaborator

It'd be easy enough to add another field to the known list, but yeah, I don't want to add another dep here.

@markerikson
Copy link
Collaborator

Going to close this for now - I don't think this is urgent enough for us to change.

@markerikson markerikson closed this as not planned Won't fix, can't repro, duplicate, stale Jul 1, 2022
@kasparkallas
Copy link

kasparkallas commented Jul 12, 2022

I'd also like to see Error.cause serialized because it's a good convention which is now supported both in Node and browsers. I would expect to see it on the SerializedError object as well.

Or possibly provide a way to override serializeError for RTK-query?

@phryneas
Copy link
Member

@kasparkallas that hasn't come up yet. If you'd like to make a PR in that direction, we'd be interested.

@kasparkallas
Copy link

kasparkallas commented Jul 18, 2022

A. Make it possible to override serializeError for RTK-Query
B. Add error's cause field to the known list and recursively serialize the errors (i.e. chain the errors)

Interested in both?

Here are some references to evaluate the urgency of B:

@kasparkallas
Copy link

Ping! @phryneas @markerikson

@phryneas
Copy link
Member

Honestly, at this point only interested in B - A would probably make the types explode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants