BREAKING CHANGE: remove support for callbacks in pre middleware #15599
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes support for the next() callback parameter in pre middleware functions, requiring middleware to handle errors through throwing exceptions or returning rejected promises instead of passing them to next(). This is part of Mongoose's broader effort to remove callback-based APIs in favor of promises and async/await patterns.
- Remove the first
nextparameter from all pre middleware function signatures - Replace
next(error)calls withthrow errorstatements - Update error handling patterns to use exceptions instead of callback-style error passing
Reviewed Changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/types/schema.test.ts | Update TypeScript type definitions for pre middleware without next parameter |
| test/types/middleware.test.ts | Update middleware type definitions and remove next() calls |
| test/types/middleware.preposttypes.test.ts | Update pre middleware function signatures for type testing |
| test/types.documentarray.test.js | Remove next() calls from pre save middleware in document array tests |
| test/query.test.js | Update query middleware to throw errors instead of calling next() |
| test/query.middleware.test.js | Update query pre middleware to remove next() parameter and calls |
| test/query.cursor.test.js | Update cursor middleware to throw errors instead of using next() |
| test/model.updateOne.test.js | Remove next() calls from updateOne pre middleware |
| test/model.test.js | Update model pre middleware throughout test suite |
| test/model.middleware.test.js | Update model middleware tests and remove sync error after next() test |
| test/model.insertMany.test.js | Update insertMany pre middleware signatures |
| test/model.discriminator.test.js | Update discriminator pre middleware to remove next() calls |
| test/model.create.test.js | Update create middleware and convert parallel pre hook to async/await |
| test/index.test.js | Update plugin pre middleware to remove next() calls |
| test/document.test.js | Update document pre middleware throughout test suite |
| test/document.modified.test.js | Update document modification pre middleware |
| test/docs/discriminators.test.js | Update discriminator documentation tests |
| test/aggregate.test.js | Update aggregate pre middleware to return promises |
| package.json | Update kareem dependency to version supporting removal of callback support |
| lib/plugins/validateBeforeSave.js | Remove next parameter from validateBeforeSave pre middleware |
| lib/model.js | Remove extraneous blank line |
| lib/helpers/timestamps/setupTimestamps.js | Update timestamp pre middleware to remove next() calls |
| docs/migrating_to_9.md | Update migration documentation for removal of isAsync middleware |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| else if (countPre === 4) endTime = Date.now(); | ||
| resolve(); | ||
| }, 100); | ||
| }); |
There was a problem hiding this comment.
[nitpick] The middleware function should be declared as async and use async/await syntax instead of returning a Promise constructor for better readability and consistency with modern JavaScript patterns.
Copilot uses AI. Check for mistakes.
| ++called; | ||
| next(); | ||
| return Promise.resolve(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[nitpick] Returning Promise.resolve() is unnecessary for synchronous operations. Either declare the function as async or simply remove the return statement since the middleware doesn't need to return anything for synchronous operations.
| }); |
Copilot uses AI. Check for mistakes.
| this.options.collation = { locale: 'en_US', strength: 1 }; | ||
| next(); | ||
| return Promise.resolve(); | ||
| }); |
There was a problem hiding this comment.
[nitpick] Returning Promise.resolve() is unnecessary for synchronous operations. Either declare the function as async or simply remove the return statement since the middleware doesn't need to return anything for synchronous operations.
Copilot uses AI. Check for mistakes.
| this.append({ $limit: 1 }); | ||
| next(); | ||
| return Promise.resolve(); | ||
| }); |
There was a problem hiding this comment.
[nitpick] Returning Promise.resolve() is unnecessary for synchronous operations. Either declare the function as async or simply remove the return statement since the middleware doesn't need to return anything for synchronous operations.
Copilot uses AI. Check for mistakes.
| s.pre('aggregate', function(next) { | ||
| next(new Error('woops')); | ||
| s.pre('aggregate', function() { | ||
| return Promise.reject(new Error('woops')); |
There was a problem hiding this comment.
[nitpick] Instead of returning a rejected promise, use throw new Error('woops') for consistency with the new error handling pattern established in this PR.
| return Promise.reject(new Error('woops')); | |
| throw new Error('woops'); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Curious if it'll make a difference to throw an error within a synchronous function compared to an async function. Will both errors be handled the same way?
| ++calledPre; | ||
| next(); | ||
| return Promise.resolve(); | ||
| }); |
There was a problem hiding this comment.
[nitpick] Returning Promise.resolve() is unnecessary for synchronous operations. Either declare the function as async or simply remove the return statement since the middleware doesn't need to return anything for synchronous operations.
| }); | |
| }); |
Copilot uses AI. Check for mistakes.
| ++calledPre; | ||
| next(); | ||
| return Promise.resolve(); | ||
| }); |
There was a problem hiding this comment.
[nitpick] Returning Promise.resolve() is unnecessary for synchronous operations. Either declare the function as async or simply remove the return statement since the middleware doesn't need to return anything for synchronous operations.
| }); | |
| }); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Sound good to me, consistent API is great.
Some changes to do though:
- remove
nextfromPreMiddlewareFunction(and likely other types) - update
docs/middleware.md(and maybe other docs?) - update the JSDoc for
Schema.prototype.pre - fix lint
Will the same be done for .post too?
hasezoey
added
backwards-breaking
developer-experience
|
Debating whether to remove |
There was a problem hiding this comment.
Only one question about error handling in normal functions compared to async functions, and whether they would have a difference for error handling in the context of middlewares.
Also, I think Copilot makes good points.
Other than that, I think it's a good change, callbacks shouldn't pollute the API anymore.
| s.pre('aggregate', function(next) { | ||
| next(new Error('woops')); | ||
| s.pre('aggregate', function() { | ||
| return Promise.reject(new Error('woops')); |
There was a problem hiding this comment.
Curious if it'll make a difference to throw an error within a synchronous function compared to an async function. Will both errors be handled the same way?
Fix #11531
Summary
The key issue in #11531 is that
next()takes up some prime real estate inpre()middleware as the first parameter to the middleware function. Withpre('insertMany')andpre('bulkWrite'), you can't do anything non-trival without puttingnext.This PR removes support for
next(): all pre middleware just gets the parameters to the wrapped function, no callbacks. This is consistent with our removing of callbacks throughout Mongoose.@hasezoey @AbdelrahmanHafez what do you think?
Examples