feat(model): add create option "immediateError" #13544
Conversation
hasezoey
added
the
enhancement
hasezoey
requested review from
vkarpov15,
Uzlopak,
AbdelrahmanHafez and
IslandRhythms
| res = await Promise.all(args.map(async doc => { | ||
| // ".bind(Promise)" is required, otherwise results in "TypeError: Promise.allSettled called on non-object" | ||
| const promiseType = !immediateError ? Promise.allSettled.bind(Promise) : Promise.all.bind(Promise); | ||
| let p = promiseType(args.map(async doc => { |
There was a problem hiding this comment.
Why can't this be just Promise.allSettled(args.map(...)) ? bind() is relatively slow, so I would prefer to avoid using that.
There was a problem hiding this comment.
i had avoided that because using allSettled in all cases would mean to both wait until all operations have finished, only to then throw the first error and second the allSettled result has to be mapped because of what it returns (ie to keep the current behavior unless requested)
lib/model.js
Outdated
| @@ -2802,6 +2801,8 @@ Model.findByIdAndRemove = function(id, options) { | |||
| * | |||
| * @param {Array|Object} docs Documents to insert, as a spread or array | |||
| * @param {Object} [options] Options passed down to `save()`. To specify `options`, `docs` **must** be an array, not a spread. See [Model.save](https://mongoosejs.com/docs/api/model.html#Model.prototype.save()) for available options. | |||
| * @param {Boolean} [options.ordered] saves the docs in series rather than parallel. | |||
| * @param {Boolean} [options.immediateError] If a Error occurs, throw the error immediately, instead of collecting in a result, default: true (backwards compatability) | |||
There was a problem hiding this comment.
I'm not sure I like this option name. I was thinking continueOnError to be consistent with eachAsync() and createCollections(), but continueOnError aggregates an error object to throw when an error occurs. Do you have any other naming ideas? Maybe useAllSettled or aggregateErrors?
There was a problem hiding this comment.
i had named it immediateError because of the current (master) behavior where once a error is encountered the first one is directly thrown / rejected, but all operations should still run and any subsequent error is just silent
There was a problem hiding this comment.
Maybe we name the option aggregateErrors instead? I think that options that are treated as false by default are cleaner to work with.
| @@ -217,7 +221,7 @@ declare module 'mongoose' { | |||
| >; | |||
|
|
|||
| /** Creates a new document or documents */ | |||
| create<DocContents = AnyKeys<TRawDocType>>(docs: Array<TRawDocType | DocContents>, options?: SaveOptions): Promise<THydratedDocumentType[]>; | |||
| create<DocContents = AnyKeys<TRawDocType>>(docs: Array<TRawDocType | DocContents>, options?: CreateOptions): Promise<THydratedDocumentType[]>; | |||
There was a problem hiding this comment.
Need to add TypeScript types to indicate that, if immediateError: false, then the return type is Promise<Array<THydratedDocumentType | Error>>
Summary
This PR adds option
immediateErrortocreate, to not throw on the first error encountered and instead add the error to the result list (only applicable if the input is a array)Default:
truefor backwards compatibility (current master behavior)works with both
ordered: falseandordered: trueResult of
immediateError: false:is this implementation OK or should it maybe be split into returning a object of values and errors or some other way?
also moves option
orderedfrom "SaveOptions" (on .save) to "CreateOptions" (to .create) because from what i can tell, the optionorderedhas no effect on.saveitselffixes #1731