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 inference for JTDSchemaType #1475

Merged
merged 2 commits into from
Mar 6, 2021
Merged

Conversation

erikbrinkman
Copy link
Collaborator

What issue does this pull request resolve?
AJV required manually annotating compile and validate with the type to get inference to work. That's no longer the case.

What changes did you make?
I made two changes:

  1. I split the overload declarations into separate declarations instead of unions
  2. I upgraded typescript to 4.2

Both of these seem to be necessary. You can test in this playground, where removing either results in failed inference. However, I'm not sure why. I couldn't find much information on how the inference is done, and certainly not a spec that says when it works and when it doesn't. That aspect makes it seem like this behavior isn't great to rely on, but I guess as long as it works in test. I also scanned over the changes for typescript 4.2, but didn't see anything that indicated it would help with inference.

Is there anything that requires more attention while reviewing?
All of the tests pass, but this does upgrade typescript to 4.2, which maybe you don't want to do yet. Note, es-lint warns on typescript 4.1 and above, so that's not great, but it still works.

@epoberezkin
Copy link
Member

Great - I would have never figured it out! Thank you.

I will add the same to compileParser and compileSerializer.

@epoberezkin
Copy link
Member

Although need to check first - maybe inference works there, it's only for JTD so a bit simpler...

@epoberezkin epoberezkin merged commit 937643e into ajv-validator:master Mar 6, 2021
@erikbrinkman erikbrinkman deleted the infer branch March 6, 2021 22:58
@epoberezkin
Copy link
Member

yes, didn't work without splitting in the same way, even though there was no JSONSchemaType - added

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

Successfully merging this pull request may close these issues.

2 participants