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

Fix types resolved from InferSchemaType #12450

Closed
wants to merge 5 commits into from

Conversation

HiiiiD
Copy link

@HiiiiD HiiiiD commented Sep 20, 2022

Summary

Currently InferSchemaType<> returns weird types when using Schema.Types.*, check #12431 for better explanation

Examples

@hasezoey hasezoey added the typescript Types or Types-test related issue / Pull Request label Sep 22, 2022
Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems to break some existing types tests (see test runs), could you maybe fix those and add new ones that this change is meant to address?

Note: in case you dont know how to run those types tests locally:

  • install everything with npm install
  • and run the types test with npm run test-tsd

@HiiiiD
Copy link
Author

HiiiiD commented Sep 22, 2022

Yes, I'm testing right now
I'll add new tests asap

@HiiiiD
Copy link
Author

HiiiiD commented Sep 22, 2022

I've rebased on top of master, but I have an issue that I don't know how to fix
I've tried for an hour to find out why required: true with Schema.Types.Decimal128 breaks the types but I haven't found a solution. (So the test at line 725 fails)
Any clues @hasezoey?

@hasezoey hasezoey mentioned this pull request Sep 23, 2022
2 tasks
@hasezoey
Copy link
Collaborator

The following would fix it for now as mentioned in #12431 (comment), though i dont know why it happens (probably a typescript bug?)

index 7c0038a869..ff9914217c 100644
--- a/types/inferschematype.d.ts
+++ b/types/inferschematype.d.ts
@@ -38,7 +38,16 @@ declare module 'mongoose' {
    * // result
    * type UserType = {userName?: string}
    */
-  type InferSchemaType<SchemaType> = ObtainSchemaGeneric<SchemaType, 'DocType'>;
+  type InferSchemaType<SchemaType> = ObtainDocType<SchemaType>; // "ObtainDocType" is used over "ObtainSchemaGeneric", because "ObtainSchemaGeneric" would return a modified result
+
+  /**
+   * @summary Obtains the "DocType" generic of a Schema, has to be used because with "ObtainSchemaGeneric" it would return a modified type
+   * @param {TSchema} TSchema A generic of schema type instance
+   */
+  type ObtainDocType<TSchema> =
+    TSchema extends Schema<any, any, any, any, any, any, any, infer DocType>
+      ? DocType
+      : unknown;
 
   /**
    * @summary Obtains schema Generic type by using generic alias.

inferschematype.patch.txt

@HiiiiD
Copy link
Author

HiiiiD commented Sep 23, 2022

I've applied your changes @hasezoey but have failing tests

@hasezoey
Copy link
Collaborator

i have updated my comment at #12431 (comment) because it (after some more testing) contained "wrong" information.

it seems like if we dont do infer TPathTypeKey but instead any, then the DocType for the thing this PR is meant to solve, solves it but creates other issues in other places, but as soon as infer SomeName is done again for the TPathTypeKey generic, then the type this PR is trying to solve, breaks again
but i have no clue what that generic is there for or what it affects or why typescript does not give the correct type

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this will be fixed by #12352, I'll check a little later.

* @param {TSchema} TSchema A generic of schema type instance
*/
type ObtainDocType<TSchema> =
TSchema extends Schema<any, any, any, any, any, any, any, infer DocType>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't do this. For some reason, not inferring all the generics (there's one in particular that is important that I forget) breaks stuff. There's a little more info on this PR: #12352 . I don't know why, TypeScript is filled with delightful little surprises like that.

@vkarpov15
Copy link
Collaborator

Confirmed this is fixed by #12352, without any test failures. I'm going to add the test to #12352 and close this issue. Thanks for pointing out this issue 👍

@vkarpov15 vkarpov15 closed this Oct 2, 2022
vkarpov15 added a commit that referenced this pull request Oct 2, 2022
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

Successfully merging this pull request may close these issues.

3 participants