🚑️ hotfix: ObjectId infererence #12168

iammola · 2022-07-29T07:07:28Z

Summary

Note
I initially spoke about this issue here. I was trying to get @mohammad0-0ahmad's opinion on it.

A required ObjectId field in a schema caused the inferred type to be deeply checked for other schema properties.

Examples

const MySchema = new Schema({ 
  class: { type: Schema.Types.ObjectId, required: true },
  dob: Date
});

type MySchemaType = InferSchemaType<typeof MySchema>;

// When hovered over in VS Code
// type MySchemaType = {
//   dob?: unknown;
//   class?: {
//       _id?: ... | undefined;
//       equals?: {} | undefined;
//       id?: {
//           [x: number]: unknown;
//           equals?: {} | undefined;
//           set?: {} | undefined;
//           toJSON?: {} | undefined;
//           [Symbol.iterator]?: {} | undefined;
//           ... 99 more ...;
//           some?: {} | undefined;
//       } | undefined;
//       ... 6 more ...;
//       toString: {};
//   } | undefined;
// }

It only affects schemas with at least one required ObjectId field. This means that a schema like ⬇️ works fine.

const MySchema = new Schema({
  class: Schema.Types.ObjectId,
  dob: Date
});

type MySchemaType = InferSchemaType<typeof MySchema>;

// When Hovered in VS Code
type MySchemaType = {
    dob?: Date | undefined;
    class?: Types.ObjectId | undefined;
}

This partially reverts commit 5f9f79c

Uzlopak · 2022-07-29T07:16:21Z

@mohammad0-0ahmad

mohammad0-0ahmad · 2022-07-29T10:59:48Z

@mohammad0-0ahmad

Thanks @Uzlopak for mention, It might be helpful if you check this disscution

mohammad0-0ahmad-forks@d092fdc#r79603415

vkarpov15 · 2022-08-03T17:07:33Z

types/inferschematype.d.ts

@@ -160,7 +159,7 @@ type ResolvePathType<PathValueType, Options extends SchemaTypeOptions<PathValueT
          PathValueType extends DateSchemaDefinition ? Date :
            PathValueType extends typeof Buffer | 'buffer' | 'Buffer' | typeof Schema.Types.Buffer ? Buffer :
              PathValueType extends BooleanSchemaDefinition ? boolean :
-                PathValueType extends ObjectIdSchemaDefinition ? Types.ObjectId :
+                PathValueType extends 'objectId' | 'ObjectId' | typeof Schema.Types.ObjectId ? Types.ObjectId :


Why not just add 'objectid' to ObjectIdSchemaDefinition ?

@vkarpov15
Which string values that should be resolved to object id ?

@iammola
You might need to as well to make this change as well:

-PathValueType extends Record<string, any> ? ObtainDocumentType<PathValueType, any, TypeKey> : + PathValueType extends Record<string, any> ? IfEquals<PathValueType, Types.ObjectId, Types.ObjectId, ObtainDocumentType<PathValueType, any, TypeKey>> :

We can revert this change as well after getting response of @vkarpov15 :

-PathValueType extends 'objectId' | 'ObjectId' | typeof Schema.Types.ObjectId ? Types.ObjectId : +PathValueType extends ObjectIdSchemaDefinition ? Types.ObjectId :

Sorry, it is my mistake. I couldn't dig into the issue as I should.
I will check more to understand the actual reason for this one.
Thanks in advance. @iammola

@vkarpov15 Please tell me when you feel upset or that we can't improve the performance because of auto typed schema, so I can revert the entire feature, I wouldn't like to make mongoose worse. 😁

I ran benchmark test after deleting the entire types folder, I got this result:
We just might consider that, I think.

Deleting all the types is a quite radical approach, imho

I was just wondering what I will get if it has no types.

I think it is weird to get this result even if the package have no type definition files.

Instantiations 15723 Memory⠀used 129661K Check⠀time 1.36s Total⠀time 2.31s

I guess tsc loads also typings from node_modules and maybe global Modules

Sure, but It might be more helpful to be able as well to diagnose only specific package type definitions.

mohammad0-0ahmad · 2022-08-03T18:10:27Z

test/types/schema.test.ts

@@ -429,7 +429,7 @@ export function autoTypedSchema() {
    mixed3: Schema.Types.Mixed,
    objectId1: Schema.Types.ObjectId,
    objectId2: 'ObjectId',
-    objectId3: 'ObjectID',
+    objectId3: 'objectId',


@vkarpov15
It is a bit confusing about the supported strings that should be resolved to Types.ObjectId,
Please clarify which ones that are supported.

@iammola
Please add this test to the end of schema.test.ts

function gh12168() { const TermSchema = new Schema({ session: { type: Schema.Types.ObjectId, required: true, ref: '...' }, start: { type: Date, required: true, unique: true }, end: { type: Date, required: true, unique: true } }); const TermModel = model('test', TermSchema); expectType<{ session: Types.ObjectId; start: Date; end: Date; }>({} as InferSchemaType<typeof TermSchema>); }

@mohammad0-0ahmad I took a look and it doesn't look like we support 'objectid', the below JavaScript throws an error:

'use strict'; const mongoose = require('mongoose'); const schema = new mongoose.Schema({ fooId: 'objectid' // Throws "TypeError: Invalid schema configuration: `Objectid` is not a valid type at path `fooId`." }); console.log('Done');

So the 'objectid' change is unnecessary and incorrect

And I would rather avoid reverting the auto typed schema feature. It does make TypeScript a little slower, but on the other hand it also removes the need for document interfaces. We'll figure out some other way to make things faster. I've learned that there's always something you can do to come up with functionally equivalent types that don't cause tsc to grind to a halt, it's just a guessing game to figure out what that is :)

Thanks, @vkarpov15, That makes me feel better. Without doubt, we will find a solution for that :)

vkarpov15 · 2022-08-03T20:29:29Z

I'm going to close this, since it doesn't look like using 'objectid' instead of 'ObjectId' is supported

mohammad0-0ahmad · 2022-08-03T20:44:57Z

@vkarpov15
we need actually to have the following changes because the current types don't work correctly with this test, which was provided by @iammola in the mentioned discussion above.

// in schema.test.ts
function gh12168() {
  const TermSchema = new Schema({
    session: { type: Schema.Types.ObjectId, required: true, ref: '...' },
    start: { type: Date, required: true, unique: true },
    end: { type: Date, required: true, unique: true }
  });

  expectType<{ session: Types.ObjectId; start: Date; end: Date; }>({} as InferSchemaType<typeof TermSchema>);
}

// inferschematype.d.ts
//line 171
-PathValueType extends Record<string, any> ? ObtainDocumentType<PathValueType, any, TypeKey> :
+PathValueType extends Record<string, any> ? IfEquals<PathValueType, Types.ObjectId, Types.ObjectId, ObtainDocumentType<PathValueType, any, TypeKey>>:

iammola · 2022-08-08T00:55:39Z

#12205

mohammad0-0ahmad · 2022-08-10T06:56:38Z

Hello @vkarpov15 !
Would you like to recheck the mentioned issue by @iammola and reopen this PR?
So @iammola can refactor it and maybe apply the suggested changes.

vkarpov15 · 2022-08-24T00:37:49Z

@mohammad0-0ahmad yeah I made a note to check out #12205.

iammola added 3 commits July 29, 2022 07:36

🚑️ hotfix: ObjectId infererence

6855a4c

This partially reverts commit 5f9f79c

Restore inferschematype.d.ts

bf54e6e

🚨 lint: only change required fields

8eb696c

vkarpov15 reviewed Aug 3, 2022

View reviewed changes

mohammad0-0ahmad reviewed Aug 3, 2022

View reviewed changes

vkarpov15 closed this Aug 3, 2022

This was referenced Aug 24, 2022

InterSchemaType does not properly infer "required" field for Schema.Types.ObjectId type #12205

Closed

feat(types+schema): allow defining schema paths using mongoose.Types.* to work around TS type inference issues #12352

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚑️ hotfix: ObjectId infererence #12168

🚑️ hotfix: ObjectId infererence #12168

iammola commented Jul 29, 2022

Uzlopak commented Jul 29, 2022

mohammad0-0ahmad commented Jul 29, 2022 •

edited

Loading

vkarpov15 Aug 3, 2022

mohammad0-0ahmad Aug 3, 2022 •

edited

Loading

mohammad0-0ahmad Aug 3, 2022 •

edited

Loading

mohammad0-0ahmad Aug 3, 2022 •

edited

Loading

mohammad0-0ahmad Aug 3, 2022

Uzlopak Aug 3, 2022

mohammad0-0ahmad Aug 3, 2022

mohammad0-0ahmad Aug 3, 2022 •

edited

Loading

Uzlopak Aug 3, 2022

mohammad0-0ahmad Aug 3, 2022 •

edited

Loading

mohammad0-0ahmad Aug 3, 2022

mohammad0-0ahmad Aug 3, 2022 •

edited

Loading

vkarpov15 Aug 3, 2022

mohammad0-0ahmad Aug 3, 2022

vkarpov15 commented Aug 3, 2022

mohammad0-0ahmad commented Aug 3, 2022 •

edited

Loading

iammola commented Aug 8, 2022

mohammad0-0ahmad commented Aug 10, 2022

vkarpov15 commented Aug 24, 2022

🚑️ hotfix: ObjectId infererence #12168

🚑️ hotfix: ObjectId infererence #12168

Conversation

iammola commented Jul 29, 2022

Summary

Examples

Uzlopak commented Jul 29, 2022

mohammad0-0ahmad commented Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

mohammad0-0ahmad Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

mohammad0-0ahmad Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

mohammad0-0ahmad Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohammad0-0ahmad Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohammad0-0ahmad Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohammad0-0ahmad Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkarpov15 commented Aug 3, 2022

mohammad0-0ahmad commented Aug 3, 2022 • edited Loading

iammola commented Aug 8, 2022

mohammad0-0ahmad commented Aug 10, 2022

vkarpov15 commented Aug 24, 2022

mohammad0-0ahmad commented Jul 29, 2022 •

edited

Loading

mohammad0-0ahmad Aug 3, 2022 •

edited

Loading

mohammad0-0ahmad Aug 3, 2022 •

edited

Loading

mohammad0-0ahmad Aug 3, 2022 •

edited

Loading

mohammad0-0ahmad Aug 3, 2022 •

edited

Loading

mohammad0-0ahmad Aug 3, 2022 •

edited

Loading

mohammad0-0ahmad Aug 3, 2022 •

edited

Loading

mohammad0-0ahmad commented Aug 3, 2022 •

edited

Loading