-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix(rest): improves error handling for express middleware #4532
Conversation
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.
Thank you for opening a PR to fix the crash. I have different opinions on how to fix the problem, please see my comments below.
@bajtos PTAL. |
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.
The new proposal looks much better now 👍
Can you please clean up the code a bit per my comment below?
I am also a bit concerned that by invoking the new error handler also for errors leaked by the sequence, we may end up with calling reject
two times:
- First, we call
reject
from the sequence. Something unexpected happens and an error is thrown. - Then we call
reject
again from the last-resort error handler.
Maybe this is not a problem? IDK.
Good question. I think it's a rare edge case and probably not too bad to call it twice. |
9f4e439
to
5495861
Compare
5495861
to
42c4cbd
Compare
Fixes #4530
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈