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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

findByIdAndDelete should not be typed as returning ModifyResult #14233

Closed
2 tasks done
ext4cats opened this issue Jan 5, 2024 · 9 comments
Closed
2 tasks done

findByIdAndDelete should not be typed as returning ModifyResult #14233

ext4cats opened this issue Jan 5, 2024 · 9 comments
Labels
typescript Types or Types-test related issue / Pull Request
Milestone

Comments

@ext4cats
Copy link

ext4cats commented Jan 5, 2024

Prerequisites

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

Mongoose version

8.0.3

Node.js version

20.9.0

MongoDB server version

6.0.12

Typescript version (if applicable)

5.3.3

Description

findByIdAndDelete is still defined in TypeScript as returning ModifyResult; unlike findOneAndDelete, which is properly typed as returning the deleted document (#14153).

Steps to Reproduce

const Foo = mongoose.model(new Schema({
  field: String
}));

const foo: Foo | null = await Foo.findByIdAndDelete("1234"); // <- TypeScript error

Expected Behavior

findByIdAndDelete should not return ModifyResult.

@vkarpov15 vkarpov15 added this to the 8.0.4 milestone Jan 6, 2024
@vkarpov15 vkarpov15 added the typescript Types or Types-test related issue / Pull Request label Jan 6, 2024
@vkarpov15
Copy link
Collaborator

Looks like this was fixed in #14196, I can repro with 8.0.3 but not master using the following script:

import {Schema, model} from 'mongoose';

const Foo = model('Foo', new Schema({
  field: String
}));

async function run() {
  type FooDocument = ReturnType<(typeof Foo)['hydrate']>;
  const foo: FooDocument | null = await Foo.findByIdAndDelete("1234"); // <- TypeScript error
}

@ext4cats
Copy link
Author

ext4cats commented Jan 7, 2024

Great, waiting for the next patch then, thank you 🙏🏼

@cbnsndwch
Copy link

I see a new v7.6.8 package version in NPM but it didn't have the fix. Will this get backported? We're stuck on v7.x for the foreseeable future until we can migrate from NestJS 9 to 10

@ext4cats
Copy link
Author

ext4cats commented Jan 9, 2024

@cbnsndwch You could try to temporarily solve this issue by using pnpm patch or the patch-package utility to apply the changes made in #14153; however I am not aware of how different v7 is to V8, it might break some stuff.

Otherwise you can just use findOneAndDelete instead, which seems to be properly typed.

@cbnsndwch
Copy link

cbnsndwch commented Jan 9, 2024

@lmarinborges thanks for the suggestion!

The types in Mongoose are a not trivial to detangle and I have just three places in my app where this creates an issue so I just any-ed the Promise for now and added a note internally to document the why. It felt less disruptive this way.

The long term solution for us is likely migrating to Nest 10 and Mongoose v8 anyway, so this will serve as yet another incentive 😅

@cbnsndwch
Copy link

Otherwise you can just use findOneAndDelete instead, which seems to be properly typed.

Not seeing this on my end. Note, I'm on v7.6.8.

image

@vkarpov15
Copy link
Collaborator

@cbnsndwch this fix is only in 8.0.4, not on 7.6.8 currently. We will backport.

@vkarpov15 vkarpov15 reopened this Jan 11, 2024
@vkarpov15 vkarpov15 modified the milestones: 8.0.4, 7.6.9 Jan 11, 2024
@cbnsndwch
Copy link

@vkarpov15 thank you for the reply!

I've started migrating my projects to Nest 10.x and Mongoose 8.x now, so I'm halfway GTG. Glad to hear you're backporting the fix though, thanks 🙏!

@vkarpov15
Copy link
Collaborator

I cherry-picked 775aadf and 7f9406f into 7.x, so this fix will be released with 7.6.9 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript Types or Types-test related issue / Pull Request
Projects
None yet
Development

No branches or pull requests

3 participants