-
-
Notifications
You must be signed in to change notification settings - Fork 886
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
[RFC] JTDDataType #1458
[RFC] JTDDataType #1458
Conversation
yes
I'd wait for more usage/feedback to decide it... It seems correct to allow both:
|
In most practical settings the application would have some semantic validation step before passing to the application logic, so this type would be used in a narrow context - only after JTD but before semantic validation... I also think that parsing rather than validating can be a better approach - just in the process of making compiled parsers from the schemas (where these types would also be used)... I'd keep a union for now until there is some feedback. |
Can it maybe detect and alert if "string" type is used in "type" or "enum" and result in never for data? I am yet to review the actual code, a bit behind :) Thank you! |
No worries on the quick review, I don't specifically need to use this, so take your time in parsing through it. You make a good case for the union, so that will stay as is. I actually did already set it up so that enums without const become never instead of unknown. I just added some tests for this. As addressed elsewhere, there's not a good way to trigger a better user facing error, so this is basically it :/ |
9c10999
to
0e39579
Compare
@erikbrinkman reviewed it - it seems null changes (#1456) should be merged first, then master merged into this one and conflicts resolved - then I will check again... |
The only comment is to allow additionalProperties in JTDDataType if the schema allows it (unless it does allow them and I missed it:) |
New tests verify that incorrect types on an enum trigger a never type
I took this off the branch with the nulls so they can be addressed separately. I think the |
I am going to merge - anything that is missing here before it can be released? |
Outside of the one comment, which should be a non breaking change if done later, I think it's ready! |
Thank you! |
Added to the docs: https://ajv.js.org/guide/typescript.html#utility-type-for-jtd-data-type I don't think type inference would work though without adding type signatures - will do it tomorrow. |
The new theme looks great! I tried blindly adding the type to I was able to get it work by manually telling the compiler what the signatures of compile and verify were. Potentially it makes sense just to do this in jtd.ts, and that will prevent this problem. It'll also potentially fix the issue with type inference. I'm still looking into it though. |
I think to use JTDDataType with type inference we need some generic type that all JTD schemas extend. In the ideal world it would be JTDSchemaType without parameters. Then the additional type signature for compile and others would be something like:
The advantage of having a generic type for some JTD schema that it would also validate schema in place, not when it is used in JTDDataType Maybe it's possible to make inference work without this type, but I didn't manage to |
Interesting. I honestly had not thought ahead to how this would work in the function signature. Taking any type as I wrote the type transformer doesn't make sense to help people type things. I guess I ultimately imagined the JTD compile to look like:
ideally for the purposes of both of these functions, the
|
Yes, I was thinking there will be multiple signatures too, what’s currently missing is the 2nd (and some generic JTD schema type to use in it). |
What issue does this pull request resolve?
Adds JTDDataType that infers the validated type from a JTDSchema.
What changes did you make?
Adds the type and adds tests around it.
Is there anything that requires more attention while reviewing?
The diff is actually pretty simple, it's much simpler than the JTDSchemaType. There are three things that caught my eye:
JTDDataType<InvalidSchema>
will just evaluate tounknown
. That will probably throw errors somewhere if you're expecting a type to be validated, but it's maybe not the best.JTDDataType<{ type: "timestamp" }>
then the result type isstring | Date
which is probably not the type you want. Is this how Ajv currently works, e.g. if you have{ type: "timestamp" }
either will be validated? Is the the desired behavior? If so, it might make sense to have "options" forJTDDataType
that tell it which choice to choose from as the union is probably not super helpful. This type of option will probably be useful when Ajv allowsMap<string, ...>
for values types as well, as the union is also probably not super helpful.Note: this is stacked on a few commits, so only look at the last one