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(type): add caster for SchemaType #10865

Closed
wants to merge 1 commit into from

Conversation

rap2hpoutre
Copy link

Summary

Adds caster in type definition.

According to this issue #10418, no concrete use case was found for adding caster in type definition files:

If there's a reason for them to be user facing I'm happy to add them to the TS bindings.

I actually found use case for plugins and introspection tools. I saw two examples in two different repositories (see examples below)

Examples

It's my first contribution to mongoose, let me know if I can improve my PR.

@@ -2948,6 +2948,9 @@ declare module 'mongoose' {
/** String representation of what type this is, like 'ObjectID' or 'Number' */
instance: string;

/** The caster instance associated with this schematype (for embedded). */
caster?: SchemaType;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be better to put this property on Subdocument and DocumentArray schema types. Those are the only schema types on which this property is defined.

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.

One quick suggestion/discussion point: I don't think all schema types should have a caster property, only subdocs and document arrays. What do you think?

@rap2hpoutre
Copy link
Author

@vkarpov15 Thank you for your answer! 🙏

I choose to put caster (optional) on SchemaType to be sure this kind of code will not trigger type error:

if (schema.paths[someRandomValue].caster.instance === "String") {}

I considered it would not be too noisy to add it on SchemaType. Still, maybe I'm wrong, if you feel like I should change my PR, I can change it 👍 (I'm not sure the code I submitted will be considered valid then).

@vkarpov15 vkarpov15 closed this in 7f49d40 Oct 21, 2021
@vkarpov15
Copy link
Collaborator

@rap2hpoutre I made an alternative fix in 7f49d40 - turns out caster isn't a SchemaType for DocumentArray, so I ended up having to add that property to both Array and DocumentArray

@vkarpov15 vkarpov15 added this to the 6.0.12 milestone Oct 21, 2021
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