Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for arbitrary keys in all "middleware-able" operation options #13629

Closed
2 tasks done
juona opened this issue Jul 19, 2023 · 2 comments
Closed
2 tasks done
Labels
needs clarification This issue doesn't have enough information to be actionable. Close after 14 days of inactivity

Comments

@juona
Copy link

juona commented Jul 19, 2023

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

馃殌 Feature Proposal

I propose to extend the solution implemented to address #10688 and add support for arbitrary option keys in all operations that invoke some middleware. I believe this is mostly an issue with the types, so I'll list those that could be improved IMO:

  • InsertManyOptions
  • SaveOptions

Perhaps there are some more?

Motivation

My motivation is the same as the one provided for #10688 - I wish to pass some data to my custom middleware, e.g. a flag to modify its behavior on a case-by-case basis.

Example

schema.pre("save", function (next, options) {
    if (!options.whatever) {
        doSomething(this);
    }
    next()
});

// ...

doc.save({ whatever: true }); // disable the middleware

Equivalent code for insertMany():

schema.pre("insertMany", function (this: Document, next: CallbackWithoutResultAndOptionalError, _: any, options: any) {
    if (!options.whatever) {
        doSomething(this);
    }
    next();
} as any); // I am forced to cast this middleware function to `any` because it does not recognize the `options` argument, but that's a separate problem

// ...

model.insertMany([{}, {}], { whatever: true });
@juona juona added enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature new feature This change adds new functionality, like a new method or class labels Jul 19, 2023
@vkarpov15 vkarpov15 added typescript Types or Types-test related issue / Pull Request and removed new feature This change adds new functionality, like a new method or class enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature labels Jul 22, 2023
@vkarpov15 vkarpov15 added this to the 7.4.3 milestone Jul 22, 2023
@vkarpov15 vkarpov15 modified the milestones: 7.4.3, 7.4.4 Aug 3, 2023
@vkarpov15
Copy link
Collaborator

The following script compiles fine, the only thing you really need to add is a : Record<string, any> param to your pre('save'). I suspect the as any cast is because you have that unnecessary _: any param.

import mongoose from 'mongoose';

const schema = new mongoose.Schema({ name: String });

schema.pre('save', function(next, opts: Record<string, any>) {
  if (opts == null) {
    return next();
  }
  if (opts.fail) {
    throw new Error('oops!');
  }
  return next();
});

schema.pre('insertMany', function(next, opts) {
  if (opts == null) {
    return next();
  }
  if (opts.fail) {
    throw new Error('oops!');
  }
  return next();
});

const Test = mongoose.model('Test', schema);

const doc = new Test({ name: 'foo' });
doc.save({ fail: true });

I'd rather not make this change because we're trying to keep our option types reasonably strict in terms of listing out all available key names. And adding & AnyObject would make for worse autocomplete on options.

@vkarpov15 vkarpov15 removed this from the 7.4.4 milestone Aug 13, 2023
@vkarpov15 vkarpov15 added needs clarification This issue doesn't have enough information to be actionable. Close after 14 days of inactivity and removed typescript Types or Types-test related issue / Pull Request labels Aug 13, 2023
@juona
Copy link
Author

juona commented Aug 21, 2023

Yeah, with #13633 complete I can get my code to look quite nice, this is fine : ]

@juona juona closed this as completed Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs clarification This issue doesn't have enough information to be actionable. Close after 14 days of inactivity
Projects
None yet
Development

No branches or pull requests

2 participants