Definitelytyped: @types/mongoose create schema doesn't allow string source to objectid

[tomyam1]

• [x] I tried using the @types/mongoose package and had problems.
• [x] I tried using the latest stable version of tsc. https://www.npmjs.com/package/typescript
• [x] [Mention](https://github.com/blog/821-mention-somebody-they-re-notified) the authors (see Definitions by: in index.d.ts) so they can respond.
  
  
  
  • Authors: @andreialecu
  
  

@andreialecu This is another example of how your PR breaks totally valid code.

I'm going to post a PR that reverts it soon.

This is not the first attempt made at creating a schema for document creation.
See
https://github.com/DefinitelyTyped/DefinitelyTyped/commit/26587aa3aa75e0021180a9a7f9b4cb45e036002b#diff-f77e4c92e873042528b4ef79285cdf3c
https://github.com/DefinitelyTyped/DefinitelyTyped/commit/22761c884a9542171583f83ccd178acd0c1e3b11#diff-f77e4c92e873042528b4ef79285cdf3c

This attempt was also not adequately testd and had PLENTY of issues.

I reverted it in
https://github.com/DefinitelyTyped/DefinitelyTyped/commit/c860c1dc7ba152a7e09b518eee900125a1a3bc3d#diff-f77e4c92e873042528b4ef79285cdf3c

Please STOP adding schema to create without testing all the possibilities. This is not that simple and causes the rest of us to waste a lot of time.

import * as Mongoose from 'mongoose'

interface IDocument extends Mongoose.Document {
    objectId: Mongoose.Types.ObjectId
}

const schema = new Mongoose.Schema({
    objectId: Mongoose.Types.ObjectId,
})

const model = Mongoose.model<IDocument>('model', schema);

/*
TS2769: No overload matches this call.   The last overload gave the following error.     Type 'string' has no properties in common with type '{ generationTime?: number; }'.  tmp.ts(4, 5): The expected type comes from property 'objectId' which is declared here on type 'Pick<{ _id?: any; objectId?: { generationTime?: number; }; } & { _id: any; }, "objectId"> & { _id?: any; }' index.d.ts(3259, 5): The last overload is declared here.
 */
model.create({
    objectId: 'valid-object-id-source',
});

// passes
model.create({
    objectId: new Mongoose.Types.ObjectId(),
});

[andreialecu]

Or you can just use typescript properly:

interface IDocument extends Mongoose.Document {
    objectId: Mongoose.Types.ObjectId | string
}

Or better yet as a general code pattern:

model.create({
    objectId: new ObjectId('valid-object-id-source'),
});

[andreialecu]

FYI you can also revert to non-type checked create via model.create<any>()

[tomyam1]

I'm using typescript properly, you're not.

This is correct:

interface IDocument extends Mongoose.Document {
    objectId: Mongoose.Types.ObjectId
}

This is NOT correct:

interface IDocument extends Mongoose.Document {
    objectId: Mongoose.Types.ObjectId | string
}

In the document objectId is never a string, always an ObjectId

Using model.create<any>() is not a good solution.

[andreialecu]

model.create({
    objectId: new ObjectId('valid-object-id-source'),
});

Is the good solution.

Or you can try your hand at a PR that handles ObjectId and changes it into ObjectId | string as part of CreateQuery<T>.

model.create<any>() is a better solution than reverting something that is obviously helping a lot of people and has been adopted downstream (typegoose being one example) because of a minor nit-pick of having to type a few additional characters.

You're obviously over-reacting here and the issues you are reporting are due to less than ideal usage of mongoose with typescript.

[tomyam1]

model.create({
    objectId: new ObjectId('valid-object-id-source'),
});

Is the good solution.

In this simple example it is.
BUT it won't work when you have a nested object with plently of strings that should be converted to objectIds.

You're obviously over-reacting here and the issues you are reporting are due to less than ideal usage of mongoose with typescript.

I disagree.

[andreialecu]

I will push a PR in about an hour that handles the implicit conversion of ObjectId to ObjectId | string

[tomyam1]

Please don't bother.

I'm sending a PR for reverting this altogether.

This is the second attempt made at this and yours suffer from the same issues as the previous one.
And the amount of testing you made is FAR FAR from sufficient for this.

This will not end in this fix.

You can see more issues at
https://github.com/DefinitelyTyped/DefinitelyTyped/pull/37823

[andreialecu]

https://github.com/DefinitelyTyped/DefinitelyTyped/pull/45610

done

[andreialecu]

The amount of testing has been more than sufficient. I have several large scale projects that have no problem using this. Additionally, the typegoose repository has no problems using this, and has an extensive test suite.

You can see how many tests there are for the functionality, it has been extensively tested.

It's only breaking badly written projects at this point or minor edge cases that can be either fixed by additional PRs or by defining your own create type.

There is the override for cases where it cannot be handled directly.

[tomyam1]

Adding a create type doesn't work because you apply CreateQuery on the create type.

[andreialecu]

What?

[tomyam1]

Due to CreateQuery<TCreate>, in:

create<TCreate = T>(doc: CreateQuery<TCreate>, options?: SaveOptions): Promise<T>;

CreateQuery forces all the properties, including nested properties, to be required

[andreialecu]

I mean, what doesn't work because of it? Do you have a specific example? It can be flipped around if necessary.

This entire issue is fixed by #45610 as you can see from the tests here:
https://github.com/DefinitelyTyped/DefinitelyTyped/pull/45610/files#diff-fe02473477a238e9eb9f7b0c22653fbaR776-R797

[andreialecu]

CreateQuery forces all the properties, including nested properties, to be required

CreateQuery<T> does not force anything that is nested to be required, unless you can show an example of a bug. Only if it's already required in T it is kept required.

[tomyam1]

I retracts the last comment, using CreateQuery doesn't force nested properties to be readonly.