-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Clean-up error handler & fix message for unknown API errors #22379
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: d76629c The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nitpick, otherwise it looks super clean
Co-authored-by: Hannes Küttner <[email protected]>
ffb7eea
to
21c19cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, some comments I feel are unnecessary or overkill but they're not hurting anyone so I dont think those matter and would just add noise in the comments 😄
api/src/middleware/error-handler.ts
Outdated
|
||
function asyncErrorHandler(fn: ErrorRequestHandler) { | ||
return (err: any, req: Request, res: Response, next: NextFunction) => | ||
Promise.resolve(fn(err, req, res, next)).catch((error) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets add a comment here to make it very clear that this Promise.resolve
is here to support both regular and async functions as fn()
. I personally think thats not directly clear from just the code/types and may be a footgun for future refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good thought! The function has initially been adapted from
directus/api/src/utils/async-handler.ts
Lines 3 to 4 in 7e6980d
const asyncHandler = (fn: RequestHandler) => (req: Request, res: Response, next: NextFunction) => | |
Promise.resolve(fn(req, res, next)).catch(next); |
Promise.resolve
wrapping.
Co-authored-by: Brainslug <[email protected]>
- Omit Promise.resolve as we know we receive a promise in this case - Use args from ErrorRequestHandler
f13e167
to
d76629c
Compare
Scope
toArray
util to transform errors - would split string errors by commasInternalServerError
Potential Risks / Drawbacks
None
Review Notes / Questions
None
Fixes #22420, partially addresses #22416