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

Error improvements #3372

Closed
2 of 6 tasks
fregante opened this issue May 11, 2022 · 5 comments
Closed
2 of 6 tasks

Error improvements #3372

fregante opened this issue May 11, 2022 · 5 comments

Comments

@fregante
Copy link
Collaborator

fregante commented May 11, 2022

  • Avoid circular references by moving around some error classes and error helper functions. Maybe as described in this comment:
    - utils/errors/businessErrors.ts
    - utils/errors/clientRequestErrors.ts
    - utils/errors/generic.ts
    - utils/errors/errorHelpers.ts
  • Look for errors that are being passed around serialized
  • Move selectErrorFromEvent/selectErrorFromRejectionEvent out of selectError
  • Avoid NetworkErrorDetail/enrichBusinessRequestError duplication
  • Possibly merge selectServerErrorMessage/selectNetworkErrorMessage into enrichBusinessRequestError so that AxiosError never makes it out out the latter
  • Replace rejectOnCancelled with an inline cancellation mechanism

Follows

@twschiller
Copy link
Contributor

Look for errors that are being passed around serialized

The 3 places where serialization has to occur is: 1) messenger API, 2) IDB, 3) Redux store

@fregante
Copy link
Collaborator Author

fregante commented May 17, 2022

Yes, but they should not leave those areas IMHO. If an error is received by those three, they should immediately deserialize it before calling external functions, although I haven't looked into where this could happen exactly in RTK.

The messenger for example does this internally for thrown errors; for passed errors (like for RECORD_ERROR) we already deserialize them on reception:

maybeSerializedError: SerializedError,
context: MessageContext,
data?: JsonObject
): Promise<void> {
forbidContext(
"contentScript",
"contentScript does not have CSP access to Rollbar"
);
try {
// Ensure it's deserialized
const error = deserializeError(maybeSerializedError);

@twschiller
Copy link
Contributor

Yes, but they should not leave those areas IMHO. If an error is received by those three, they should immediately deserialize it before calling external functions, although I haven't looked into where this could happen exactly in RTK.

RTK is just a nice toolkit for Redux. What's happening is that the error is getting serialized before it's placed in the store

Then, in our components we use hooks, e.g., const {data, error, isFetching} = useMyCustomQuery(params) to get that data out of the store. So we're have to do one of the following:

  1. Modify the RTK library to deserialize the errors it passes back from its hooks, or
  2. In each of our components deserialize the error coming from RTK

Option 2 is a non-starter -- it's too tedious/error-prone. Option 1 could make sense in the future

With respect to IDB, we could deserialize upon retrieval from IDB. However, there's some places where we store the errors in Redux. So we'd need to serialize before putting it into the Redux store there and then deserialize

All in all, our codebase really wants everything to be serializable 😄 . I think we're fighting an uphill battle against wanting to rely on classnames/prototypes/instanceof

Copy link

This issue will be closed in 7 days unless the stale label is removed, or a comment is added to the issue.

@github-actions github-actions bot added the Stale label Dec 30, 2023
Copy link

github-actions bot commented Jan 6, 2024

This issue was closed because it has been stale for 7 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants