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

SchemaTypeOptions.type is optional #9927

Closed
wants to merge 1 commit into from
Closed

Conversation

moander
Copy link
Contributor

@moander moander commented Feb 13, 2021

type must be optional to support schemas using the typeKey option and nested paths.

This PR reverts a change that was merged a few days ago:
968bd8a#diff-7aa4473ede4abd9ec099e87fec67fd57afafaf39e05d493ab4533acc38547eb8R1367

type must be optional to support schemas using the `typeKey` option.
moander referenced this pull request Feb 13, 2021
@vkarpov15
Copy link
Collaborator

Unfortunately this change prevents us from throwing an error if the type is incorrect. We'll investigate this further, I'd like to avoid merging this PR as is because of #9857, but we'll see if we can get the best of both.

@vkarpov15 vkarpov15 added this to the 5.11.17 milestone Feb 15, 2021
@vkarpov15 vkarpov15 added the typescript Types or Types-test related issue / Pull Request label Feb 15, 2021
@vkarpov15 vkarpov15 closed this in a485c40 Feb 16, 2021
@vkarpov15
Copy link
Collaborator

I took a closer look and the issue comes down to the fact that the value String is a valid SchemaTypeOptions because SchemaTypeOptions has no required keys. That leaves us in a tough spot with supporting checking incorrect schema definitions where the interface has a path set to number but the Schema has that path set to String while still supporting typeKey, because typeKey isn't necessarily known at compile time. The approach on a485c40 will handle the case where the user uses { type: String } rather than just String, that's the best we can do for now 👍

This was referenced Mar 5, 2021
This was referenced Mar 12, 2021
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.

2 participants