-
Notifications
You must be signed in to change notification settings - Fork 65
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
Async middleware #148
base: main
Are you sure you want to change the base?
Async middleware #148
Conversation
|
Codecov Report
@@ Coverage Diff @@
## master #148 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 80 78 -2
=========================================
- Hits 80 78 -2
Continue to review full report at Codecov.
|
test/index.test.js
Outdated
@@ -39,20 +39,21 @@ describe("nc()", () => { | |||
.expect("quick math"); | |||
}); | |||
|
|||
it("supports async handlers", async () => { | |||
it("supports async handlers and middleware", async () => { |
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.
Can you add them as new test cases instead of editing existed ones?
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.
Got it 👍
Hey! Just bumping this PR in case you missed it. Requested changes have been made! |
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.
So sorry for the late review. I just have some questions.
}; | ||
const next = (err) => i < len | ||
? err | ||
? onError(err, req, res, next) |
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.
What would be its behavior when an error is thrown. In this case the execution inside the handler still goes on after await next()?
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.
Yeah that's something I totally missed in my testing. my bad! Indeed errors aren't halting execution after await next()
.
It seems it's behaving this way because the error is handled inside of next()
and never returned back. I found a solution that ensures that errors are propagated, but I can't find one that gives onError()
an option to call next()
and allow middleware to continue execution after doing so. Any ideas?
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.
We may need to decide on a behavior.
One thing we can do is to let next()
throws and the error should propagate all the way up and only attach the error handler to the first handler:
handler1: await() -- onError()
middleware1: await next() -- ⬆️
middleware2: await next() - ⬆️
middleware3: throw new Error()
But that is a breaking change since it will break if user have not been doing await next()
, which is probably the case now.
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.
My pending changes resolves error handling without having to unravel the stack. In the event that the error handler wishes to ignore the error and call next()
, however, it leads to the following unexpected behaviour.
// ❌ = does not execute
// Middleware listed after .get() represents code to be executed after await next().
// Example: GET /
middleware1: await next()
middleware2: await next()
middleware3: throw new Error() -> onError() -> next()
.get(handler):
middleware3: ❌ -- not expected to run, it gave up control after throwing
middleware2: ❌ -- expected to run
middleare1: ❌ -- expected to run
This is where I'm running into problems with finding a decent solution. Do you think it's worth preserving this trait of error handling?
closes #146