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

additionalProperties shouldn't affect TypeScript types from JTDDataType #2405

Closed
jaketrent opened this issue Apr 4, 2024 · 10 comments
Closed

Comments

@jaketrent
Copy link

jaketrent commented Apr 4, 2024

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:

  • In schema validation: Most of the time, it's ok if a schema contains extra information. Downstream consumers of a JS object can just ignore extra fields. They don't even have to know it's there.
  • In type-checking code: If an object is relied on to have certain fields, they must be present in the declared interface. If code relies on more than is the interface, TypeScript will assist the programmer and indicate a potential problem.

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:

const STRONG_MY_JTD_SCHEMA = { ...MY_JTD_SCHEMA, additionalProperties: false }
type MyType = JTDDataType<typeof STRONG_MY_JTD_SCHEMA>

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.

@jasoniangreen
Copy link
Collaborator

jasoniangreen commented Apr 4, 2024

Isn't this what already happens.

Here I have a JTD schema describing an object with an a prop and only that. When I use JTDDataType and try to add b prop it gets the red underline and the error is Object literal may only specify known properties, and 'b' does not exist in type.

Screenshot 2024-04-04 at 23 36 52

Is this what you expect?

@jaketrent
Copy link
Author

jaketrent commented Apr 4, 2024

Hi @jasoniangreen Thanks for your attention.

Yes, the example and expected tsc error you describe make sense.

The issue I'm describing always involves additionalProperties: true.

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:

https://github.com/jaketrent/demo-ajv-ts-issue

@jasoniangreen
Copy link
Collaborator

Hi @jaketrent, in case 4 you state:

// no 'may only specify known properties' error
const p2: Person = { name: "Alice", age: 32 }

But in case 4 you have specified that Person has additionalProperties: true so I would expect that you can add an age prop without it being considered an error.

What am I missing?

@jaketrent
Copy link
Author

jaketrent commented May 14, 2024

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 age on a Person instance.

Still in Case 4: The p1 object is valid. This is great, because additionalProperties: true is set on the schema. And I believe this is the common case (eg, That we don't care about extra fields coming through on network request JSON).

But the p2 line also doesn't TypeError (as is shown useful in Case 1). This is because in Case 4, we're creating a Person type using the JDDataType util. But this goes against the common case, where in one's code, we want stricter types.

So I argue that the JDDataType should by default return a type as if additionalProperties: false is set, whether it's set that way in the schema or not. This would match the use cases of validation of code typing better.

@jaketrent
Copy link
Author

If you're interested, my current best workaround is explained here: https://jaketrent.com/post/strong-type-check-ajv-additional-properties/

@jasoniangreen
Copy link
Collaborator

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.

@erikbrinkman
Copy link
Collaborator

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 interface Both { a: number; b: number } can be assigned to interface One { a: number }. Adding properties, or other qualities to a type, subtracts from the space of unknown by specifying that a property must exist, and must have this type. In this sense the idea of adding Record<string, unknown> is antithetical to typescript, because all objects are a Record<string, unknown> inherently.

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 additionalProperties keyword. Is it:

  1. a validation directive, saying to ignore any properties not listed when validating
  2. a structural directive, telling you something about the type you're describing, in the same way properties and optionalProperties arguably are

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;dr

Ultimately, 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

@jaketrent
Copy link
Author

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, additionalProperties: true is a validation directive, for sure.

But ajv-validator also has this JDDataType util for generating types. When feeding a schema into this util, out comes a typedef, which makes the schema input seem like a structural directive.

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.

@jasoniangreen
Copy link
Collaborator

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 additionalProperties: true that the type would allow them, so much so that it took me a while to understand the issue.

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!

@jaketrent
Copy link
Author

That works for me. I'm glad there's a workaround. This tool's great. Thanks for the conversation.

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

No branches or pull requests

3 participants