BREAKING CHANGE: make execPre async and drop callback support #39
Conversation
This file contains hidden or 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
hasezoey
approved these changes
Mar 20, 2025
There was a problem hiding this comment.
Code looks good to me, though as i have not worked with this much, i was quite confused about isAsync vs returning a Promise, but now i understand isAsync means to run it in parallel instead of waiting before calling the next hook. Maybe add some documentation to the isAsync parameters?
Another question that came up, should next be deprecated in favor of using Promises?
Also the documentation (just the README?) needs to be updated to reflect this (btw the README contains some acquit:ignore:end)
PS: if this PR is a WIP, maybe mark it as such?
vkarpov15
added a commit
to Automattic/mongoose
that referenced
this pull request
Apr 28, 2025
hasezoey
reviewed
Apr 28, 2025
| @@ -28,8 +28,7 @@ Kareem.overwriteResult = function overwriteResult() { | |||
| * Execute all "pre" hooks for "name" | |||
| * @param {String} name The hook name to execute | |||
| * @param {*} context Overwrite the "this" for the hook | |||
| * @param {Array|Function} args Optional arguments or directly the callback | |||
| * @param {Function} [callback] The callback to call when executing all hooks are finished | |||
| * @param {Array|Function} args arguments passed to the pre hooks | |||
There was a problem hiding this comment.
Just to confirm, this should still include Function even though the callback is removed?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Work in progress to make
execPre()andexecPost()use async functions where possible to better support Automattic/mongoose#15317. Tested in kareem repo as well as running Mongoose with these changes.Couple of breaking changes in this PR:
next(null, 'new arg')and the args to the next middleware would get overwritten by 'new arg'.execPre()next()calls. If your middleware function does something likenext(); throw new Error('Fail');, this new logic will treat the middleware function as errored with error 'Fail'. Kareem@2 ignores the 'Fail' error.@hasezoey your feedback would be much appreciated.