-
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
Open
UncleClapton
wants to merge
5
commits into
hoangvvo:main
Choose a base branch
from
UncleClapton:async-middleware
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Async middleware #148
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
6fc532f
✨ Add ability to form async middleware chains
UncleClapton c2300b8
✅ Update async test to include async middleware execution
UncleClapton d2de080
📝 Add documentation on async next()
UncleClapton 1bc8e26
Revert "✅ Update async test to include async middleware execution"
UncleClapton 4e32dcf
✅ add test for async middleware
UncleClapton File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 givesonError()
an option to callnext()
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: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.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?