Skip to content

Conversation

@fenos
Copy link
Collaborator

@fenos fenos commented Nov 30, 2023

This PR replaces #518 in favour of a new approach.

A legitimate use case for this hook which I think is worth considering is:

  • Custom logging. In the PR you mentioned above doesn't really solve the issue because we might want to send the error somewhere in Sentry or in any error reporter of choice in a specific format. Having this hook would make it possible to do just that.

  • Centralizing the mapping of application-specific errors to Tus HTTP errors.
    This use case I think is quite important since application-specific errors can be thrown in many different places not only on onUploadCreate , onUploadFinish but potentially in all the events hooks emitted as well in custom Locks implementations.

By centralising the mapping of error handling we don't need to try/catch everywhere and do the mapping in place, this allows for better maintainability in the long run.

I have changed the implementation slightly from the original PR and now the onErrorResponse doesn't actually allow to write the response but can be used as follows:

new Server({
 ...
 onErrorResponse: async (req, res, error) => {
   await customErrorReporter.error(error)
   
   if (error instanceof CustomError) {
      return { status_code: error.statusCode, body: error.message }
   }
 }
})

overall is a neat way to keep things tidy and centralised

Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay to add this indeed, it offers some new possibilities and the code is minimal 👍

My feedback:

  • We need docs for this
  • Would be nice to add the same docs to the types so it's also available in editors.
  • A simple test would be nice (Server.test.ts)

@Murderlon Murderlon changed the title Feat: onErrorResponse hook @tus/server: introduce onErrorResponse hook Dec 4, 2023
@Murderlon Murderlon linked an issue Dec 4, 2023 that may be closed by this pull request
@fenos
Copy link
Collaborator Author

fenos commented Dec 5, 2023

  • We need docs for this ✅
  • Would be nice to add the same docs to the types so it's also available in editors. ✅
  • A simple test would be nice (Server.test.ts) ✅

@Murderlon all ready!

Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@Murderlon Murderlon merged commit f1a4ac3 into tus:main Dec 6, 2023
@fenos fenos deleted the feat/custom-error-handler branch December 6, 2023 17:07
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

Successfully merging this pull request may close these issues.

onError hook

2 participants