-
-
Notifications
You must be signed in to change notification settings - Fork 864
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
additionalProperties shouldn't affect TypeScript types from JTDDataType #2405
Comments
Hi @jasoniangreen Thanks for your attention. Yes, the example and expected tsc error you describe make sense. The issue I'm describing always involves I've created a little test file so that you can both build TS and exercise runtime validations. I've added comments for a couple test cases. I hope it's helpful in seeing what I'm pointing out: |
Hi @jaketrent, in case 4 you state:
But in case 4 you have specified that Person has What am I missing? |
Hi @jasoniangreen Thanks for coming back to this. I will attempt to clarify a bit. Case 4 is in contrast to Case 1, where we got a type error when we tried to specify an Still in Case 4: The But the So I argue that the JDDataType should by default return a type as if |
If you're interested, my current best workaround is explained here: https://jaketrent.com/post/strong-type-check-ajv-additional-properties/ |
Thanks for the very detailed response @jaketrent, I do see your point now. I'd like to get some more opinions so will tag @erikbrinkman who has implemented a lot of the typescript functionality and of course @epoberezkin. |
I think my thoughts here are complicated, and I don't have a consistent way to view this problem. As @jaketrent says, typescript types are inherently subtractive, that is That being said, typescript is an inconsistent type system, and as you demonstrate they do have different behaviors. In addition, the subtractive behavior is different in once instance, and that's assigning object literals (any maybe const type inference). This is generally the case that the typescript jtd types apply to, since they try to function more like additive types in the first place. This playground recreates some of what you point out in your examples, namely that if you don't have the record you can't assign object literals with extra properties, or access extra properties without encountering an error in typescript. As I write this, I think it ultimately comes down to how you view the
If you're in camp 1, then it shouldn't append the record, if you're in camp 2, then it should. I think in your arguments, you make the case that more people are in camp 1. I'm not entirely sure, but don't really have a pony in that race. tl;drUltimately, I think I still stand in camp 2, that adding the record is a more faithful reconstruction of the type, and fits with how JTD is structured. I think there are arguments for camp 1, and it's ultimately up to true maintainers e.g. @jasoniangreen or @epoberezkin to decide. Changing it would be breaking though and should probably be reserved for v9 if that's the desired route. I should probably also mention that both types are assignable to one another (as shown in the playground), so it's not too difficult to to pick how you want the type to be checked after the fact. Also, I'm not aware of a nice way to achieve both, that is ti make it user configurable. @jaketrent let me know if I misinterpreted or got anything wrong |
Hey @erikbrinkman. I appreciate the feedback on the idea. Your interpretation seems accurate. Yes, I think it comes down to what one thinks is the common case for most devs. I think most devs use ajv-validator for the validation. There, But ajv-validator also has this So the schema and its directives fulfill both use cases. And this is great. No duplication of the directives is the main reason I chose ajv vs. a competitor. This feature has value, at least to me, and I'm glad it's there, straddling both use cases. Also agreed that the perspectives are nuanced and that this is a breaking change. Thanks for your consideration. |
Thanks both for your detailed thoughts on this, it's really very useful and interesting. I have spoken to @epoberezkin about this and we're both in camp 2 as well with no desire to change the behaviour. For me it just made sense and was intuitive that given Speaking with EP he also suggest the exact same work around you wrote about @jaketrent in your blog post and his position is that this logic makes sense in the application/consuming codebase rather than within AJV. So on this note I will close the issue, but I am glad it will be discoverable by others asking the same questions as it was very detailed and thoughtful discussion. Thanks! |
That works for me. I'm glad there's a workaround. This tool's great. Thanks for the conversation. |
What version of Ajv you are you using?
8.12.0
What problem do you want to solve?
I would like stronger type checking on types returned from JTDDataType.
What do you think is the correct solution to problem?
JTDDataType should return a type as though
additionalProperties
is always false.The needs of the two scenarios are different:
The JTDDataType utility tries to bridge two worlds: generating types from schemas, which is great. But adding
& Record
to the end of such generated types seems to break useful, default-case type checking.There is a workaround, but it feels icky for what seems like the default case:
Will you be able to implement it?
Not sure.
Presumably, the
& Record
needs removed from the type that's output. But I'm unsure of the related consequences of such a change.Thank you for the great tool and your consideration of this feedback.
The text was updated successfully, but these errors were encountered: