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

typescript: Allow null for optional document fields #12781

Merged
merged 4 commits into from
Jan 23, 2023

Conversation

JavaScriptBach
Copy link
Contributor

@JavaScriptBach JavaScriptBach commented Dec 8, 2022

Summary

When inferring types, allow null as well as undefined if the field is optional.

Fixes #12748

Examples

@Uzlopak Uzlopak closed this Dec 8, 2022
@Uzlopak Uzlopak reopened this Dec 8, 2022
@Uzlopak
Copy link
Collaborator

Uzlopak commented Dec 8, 2022

Accidently closed.

Null has a special meaning in mongo. I doubt if this is a desired behavior.

@JavaScriptBach
Copy link
Contributor Author

JavaScriptBach commented Dec 8, 2022

We can insert null into a Mongo collection, so why should that not be allowed for optional Mongoose fields?

@JavaScriptBach JavaScriptBach marked this pull request as ready for review December 9, 2022 04:54
@Uzlopak
Copy link
Collaborator

Uzlopak commented Dec 9, 2022

Nobody says that null is not a valid input. But while JSON.stringify removes keys with value null, the bson serializer serializes null values.
So the expectations can be different.

So we should keep this in mind when merging this.

@JavaScriptBach
Copy link
Contributor Author

I think it is the opposite actually. JSON.stringify() removes undefined keys, but keeps null keys: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#description

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.

This works, thank you for making this change. You are correct that you can set any non-required property to null.

My only concern is that this change might be a bit heavy for a patch release, so we'll save it for our next minor release.

@vkarpov15 vkarpov15 added this to the 6.9.0 milestone Dec 16, 2022
@hasezoey hasezoey added the typescript Types or Types-test related issue / Pull Request label Dec 20, 2022
@JavaScriptBach
Copy link
Contributor Author

It turns out what I really need for my use case is for optional strings to infer as foo: string | null | undefined instead of foo ?: string | undefined. But that seems a different direction than what was taken in this project, and I don't suppose you're willing to change the inference to a required field unioned with null and undefined?

So feel free to merge or not merge this PR, but I no longer have a stake in this issue now. Thanks for reviewing either way.

@vkarpov15 vkarpov15 changed the base branch from master to 6.9 January 23, 2023 22:40
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.

Merging into 6.9

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.

InferSchemaType: Allow null for optional fields
4 participants