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: Discriminator generic type not being passed to schema #11898

Merged

Conversation

GCastilho
Copy link
Contributor

-- Fix instance created from model returned from discriminator having type 'any'
-- Fix model created from discriminator not inheriting the properties from the base model

Summary

A model created from a discriminator does not have types for the "document" (save, find, etc) as the original model does and does not inherit the properties from the base model

Examples

type EventSchema = {
    time: Date
}
const eventSchema = new mongoose.Schema<EventSchema>({ time: Date }, options);
const Event = mongoose.model('Event', eventSchema);

type ClickedLinkEventSchema = {
    url: string
}
const clickedLinkEventSchema = new mongoose.Schema<ClickedLinkEventSchema>({ url: String }, options)
const ClickedLinkEvent = Event.discriminator('ClickedLink', clickedLinkEventSchema);
const clickedLinkEvent = new ClickedLinkEvent({
    time: new Date(),
    url: 'https://mongoosejs.com/',
});

# Old
clickedLinkEvent.time  // time has type any
clickedLinkEvent.save() // save has type any

# New
clickedLinkEvent.time  // time has type Date
clickedLinkEvent.save() // save has the type from Document['save']

This is a remake of #11897 with fixed branch name and commits

-- Fix instance created from model returned from discriminator having type 'any'
-- Fix model created from discriminator not inheriting the properties from the base model
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.

Tests are failing because you also need to modify types/schematypes.d.ts, there's a couple places in that file where we use AcceptsDiscriminator:

class Array extends SchemaType implements AcceptsDiscriminator {
for example.

types/models.d.ts Outdated Show resolved Hide resolved
-- Fix model type returned from discriminator ignoring possible parent schema property overrides
…or as generic

-- Fix some of the classes in the Schema namespace that were not updated to use AcceptsDiscriminator as a generic
-- Those classes were updated to be generics with default type to any, so no change in behavior is expected
@GCastilho
Copy link
Contributor Author

I've updated the classes in types/schematypes.d.ts that implemented the AcceptsDiscriminator to be generics

Their 'B' type is passed to SchemaType and Schema so I believe it is "the type of the class", so "an Array of numbers", using your example

It defaults to any so no change in behavior is expected

@GCastilho GCastilho requested a review from vkarpov15 June 6, 2022 17:07
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.

Thanks 👍

@vkarpov15 vkarpov15 added this to the 6.3.6 milestone Jun 7, 2022
@vkarpov15 vkarpov15 merged commit 0612349 into Automattic:master Jun 7, 2022
@GCastilho GCastilho deleted the fix/discriminator-type-inheritance branch June 7, 2022 17:06
@vkarpov15
Copy link
Collaborator

In hindsight, I think I'm going to have to revert this PR. This really slows down our TS performance benchmark.

Before:

Run node ./benchmarks/typescript
simple instantiations: 42294
simple memory used: 1[6](https://github.com/Automattic/mongoose/runs/6776724234?check_suite_focus=true#step:5:7)5158.9
simple check time: 2.[7](https://github.com/Automattic/mongoose/runs/6776724234?check_suite_focus=true#step:5:8)21[9](https://github.com/Automattic/mongoose/runs/6776724234?check_suite_focus=true#step:5:10)999999999995
simple total time: 4.175999999999999

After:

Run node ./benchmarks/typescript
simple instantiations: 2557[1](https://github.com/Automattic/mongoose/runs/6747560604?check_suite_focus=true#step:5:1)4
simple memory used: 19440[7](https://github.com/Automattic/mongoose/runs/6747560604?check_suite_focus=true#step:5:8).2
simple check time: 3.99[8](https://github.com/Automattic/mongoose/runs/6747560604?check_suite_focus=true#step:5:9)
simple total time: 5.448

@GCastilho
Copy link
Contributor Author

Well, it does make sense that performance would slow down a little since the types were being defaulted to any and with this PR some generics were added for the types to be passed down

It is a new "feature", per se, it make sense for it to impact something

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants