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

Definition of type UpdateQuery is not very accurate cause code hint not work #13630

Closed
2 tasks done
taozi0818 opened this issue Jul 19, 2023 · 3 comments · Fixed by #13699
Closed
2 tasks done

Definition of type UpdateQuery is not very accurate cause code hint not work #13630

taozi0818 opened this issue Jul 19, 2023 · 3 comments · Fixed by #13699
Labels
typescript Types or Types-test related issue / Pull Request
Milestone

Comments

@taozi0818
Copy link

taozi0818 commented Jul 19, 2023

Prerequisites

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

Mongoose version

7.x

Node.js version

18.x

MongoDB server version

4.x

Typescript version (if applicable)

4.x

Description

The definition of type UpdateQuery is not very accurate cause code hint not work.

Steps to Reproduce

The definition of types UpdateQuery for now:

export type UpdateQuery<T> = _UpdateQuery<T> & AnyObject;

Typescript can not do code hint with using it, when you add some field not belong to this schema. Like this:

// schema
interface SchemaDemo {
  field_1: string;
  field_2: string;
}

function update(doc: UpdateQuery<SchemaDemo>) {}

update({ test_field: 'test' }); // Will not get types error

update({ test_field: 'test' }) It should get a types error, because test_field is not a field of interface SchemaDemo. Types UpdateQuery is an intersection type which include AnyObject, so test_field matched AnyObject. In a sense, UpdateQuery make no senses for now.

Expected Behavior

Don't use AnyObject, it make type system not work. If user want to do something about custom type, just use generic type to do that. I think UpdateQuery should by defined like that:

export UpdateQuery<TSchema = AnyObject>  = _UpdateQuery<TSchema>  & Partial<TSchema>

In this design, more flexibility for users. Usage case:

type UpdateQuery<TSchema = AnyObject>  = _UpdateQuery<TSchema>  & Partial<TSchema>

// schema
interface SchemaDemo {
  field_1: string;
  field_2: string;
}

function update(doc: UpdateQuery<SchemaDemo>) {}

update({ field_test: 1 }); //  ❌ no fields
update({ field_1: 2}); // ❌ get erro type is not matched
update({ field_1: '' }); //  ✅
update({ $set: { field_1: '' }}); 

function update2(doc: UpdateQuery) {}

update2({  aaa: 1 }); // ✅  used as any 

Tasks

No tasks being tracked yet.
@IslandRhythms IslandRhythms added the typescript Types or Types-test related issue / Pull Request label Jul 19, 2023
@vkarpov15
Copy link
Collaborator

The current type definition, as written, is correct for several reasons:

  1. Calling updateOne() on a field that doesn't exist in the schema won't throw an error by default. Mongoose will just ignore the unknown field.
  2. With strict mode set to false, you actually can set unknown fields in updateOne().
  3. Mongoose allows using dotted field names for updates, like updateOne({}, { 'nested.path': 42 }). And, while it is theoretically possible to calculate possible dotted path names in TypeScript, it is extremely slow. Too slow to implement in practice.

Thanks for the suggestion though 👍

@taozi0818
Copy link
Author

taozi0818 commented Jul 20, 2023

The current type definition, as written, is correct for several reasons:

  1. Calling updateOne() on a field that doesn't exist in the schema won't throw an error by default. Mongoose will just ignore the unknown field.
  2. With strict mode set to false, you actually can set unknown fields in updateOne().
  3. Mongoose allows using dotted field names for updates, like updateOne({}, { 'nested.path': 42 }). And, while it is theoretically possible to calculate possible dotted path names in TypeScript, it is extremely slow. Too slow to implement in practice.

Thanks for the suggestion though 👍

Thanks for ur reply.

Please reconside about this definition. Reasons you listed are not convincing.

  1. unknown field. If you want to use unknown field, just use AnyObject or any as generic type, like this UpdateQuery<AnyObject>
  2. nested field. In most case, updateOne() called with a model as params. Even if it is a params like { 'nested.path': 42} , you can use AnyObject like UpdateQuery<AnyObject> or just UpdateQuery(default to AnyObject, equals to the definition existed type UpdateQuery<T> = _UpdateQuery<T> & AnyObject;).
  3. There is no way to do something about type to restrict for using unknown field for now, because exported type UpdateQuery is an intersection type included AnyObject. As a library not a framework, I think this feature should be provided in mongoose. If mongoose team don't want to modify the definition, it should export a type like _UpdateQuery to let the users can do that above at least.

@vkarpov15 vkarpov15 reopened this Jul 24, 2023
@vkarpov15 vkarpov15 added this to the 7.4.3 milestone Jul 24, 2023
vkarpov15 added a commit that referenced this issue Aug 8, 2023
types: add UpdateQueryKnownOnly type for stricter UpdateQuery type checking
@vkarpov15
Copy link
Collaborator

We'll add a UpdateQueryKnownOnly type in 7.4.3 for this use case.

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
3 participants