Wrap errors thrown in user callbacks with descriptive messages#860
Wrap errors thrown in user callbacks with descriptive messages#860vikas90244 wants to merge 3 commits into
Conversation
Acconut
left a comment
Acconut
left a comment
There was a problem hiding this comment.
Thank you very much for working on this improvement! I have two minor comments, but otherwise this looks good :)
| this.options.onError(err) | ||
| } catch (callbackErr) { | ||
| const causeMsg = callbackErr instanceof Error ? callbackErr.message : String(callbackErr) | ||
| throw new Error(`tus: error thrown in 'onError' callback: ${causeMsg}`) |
There was a problem hiding this comment.
I believe we can set the cause property to expose the original error to the caller.
| throw new Error(`tus: error thrown in 'onError' callback: ${causeMsg}`) | |
| throw new Error(`tus: error thrown in 'onError' callback: ${causeMsg}`, { cause: callbackErr }) |
There was a problem hiding this comment.
Thanks for the suggestions!
I tried using { cause: callbackErr } in the Error constructor but it results in "Expected 0-1 arguments, but got 2" since ErrorOptions is not available with the current ES2018 target.
As an alternative I considered manually assigning wrappedErr.cause = callbackErr but TypeScript also rejects this without updating the lib compiler option.
For now I've kept the implementation without .cause, the original error message is already included in the wrapping message string via causeMsg, so the information is still visible to the caller. Happy to revisit if you have a preferred approach.
| this.options.onError(err) | ||
| } catch (callbackErr) { | ||
| const causeMsg = callbackErr instanceof Error ? callbackErr.message : String(callbackErr) | ||
| throw new Error(`tus: error thrown in 'onError' callback: ${causeMsg}`) |
There was a problem hiding this comment.
Is it possible to add a test case for this? So we know that errors from callbacks are properly caught, wrapped and don't send the upload logic into a retry loop.
|
Hi @Acconut , just a gentle bump on this PR when you get a chance. Thanks! |
2 participants
Fixes #294
When a user's callback (onProgress, onError, onChunkComplete, onBeforeRequest, onAfterResponse, onShouldRetry) throws an error, the error message now clearly indicates which callback caused it instead of appearing as an internal tus error.