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

Add JTDDataType to compile signature #1547

Merged
merged 2 commits into from
Apr 15, 2021

Conversation

erikbrinkman
Copy link
Collaborator

What issue does this pull request resolve?

#1489

What changes did you make?

  1. The semantics of ref for JTDDataType had to be changed. I don't entirely understand why, but my guess is that infer steps allow the compiler to "take a break" and so this helps with the recursion checking.
  2. Added SomeJTDSchemaType. This is necessary to prevent typescript from inferring a JTDDataType when actually it's a different schema. This is especially a problem for simple types, e.g. the empty schema is valid JTD so it can confuse things.

Is there anything that requires more attention while reviewing?

  1. Historically specifying a type for compile indicated that that was the return type. To keep that working, the overload signatures for the JTDDataType returns need to have their first parameter extend never, so that it's only inferred (or it could be specified manually with compile<never, ActualType>(...).
  2. In addition, SomeJTDSchemaType needs to use the empty type {}, there's a note linking to the issue that discusses how this is the one instance when this is actually what you want, and it's tested.
  3. Finally, this works with typescript 4.2.3. However, even with the infer trick, it's very close to the maximum complexity that typescript wants to deal with. Small changes in the overload signature or the way that typescript descides to handle this could result in compile erroring in typescript saying the type is too complex. In that event, this overload could always be removed, but I wanted to raise this potential risk now.

To get this to work, a few changes had to be made:
1. The semantics of `ref` for JTDDataType had to be changed. I don't
   entirely understand why, but my guess is that infer steps allow the
   compiler to "take a break" and so this helps with the recursion
   checking.
2. Added `SomeJTDSchemaType`. This is necessary to prevent typescript
   from inferring a JTDDataType when actually it's a different schema.
   This is especially a problem for simple types, e.g. the empty schema
   is valid JTD so it can confuse things.

Three other notes about the current implementation. Historically
specifying a type for compile indicated that that was the return type.
To keep that working, the overload signatures for the JTDDataType
returns need to have their first parameter extend never, so that it's
only inferred (or it could be specified manually with `compile<never,
ActualType>(...)`.

In addition, SomeJTDSchemaType needs to use the empty type `{}`, there's
a note linking to the issue that discusses how this is the one instance
when this is actually what you want, and it's tested.

Finally, this works with typescript 4.2.3. However, even with the infer
trick, it's very close to the maximum complexity that typescript wants
to deal with. Small changes in the overload signature or the way that
typescript descides to handle this could result in compile erroring in
typescript saying the type is too complex. In that event, this overload
could always be removed, but I wanted to raise this potential risk now.

fixes ajv-validator#1489
@epoberezkin
Copy link
Member

This is great!

The semantics of ref for JTDDataType had to be changed

Not sure I understand...

or it could be specified manually with compile<never, ActualType>(...)

ActualType is the type of schema in this case, correct? The inference with the type of data seems not changed...

@erikbrinkman
Copy link
Collaborator Author

by

The semantics of ref for JTDDataType had to be changed

I just mean that I changed how ref is parsed. In the diff I switched from using D[S["ref"]] to D extends {[K in S["ref"]]: infer V}. This stopped the recursive type definition error. It should be loosely the same type, but for some reason, typescript thinks it's simpler. I'm not sure why.

ActualType is the type of schema in this case, correct? The inference with the type of data seems not changed...

Yes, basically I had to make it take the form compile<ResultType, SchemaType>, so that manually specifying the result type still worked. However, since compile<FakeResultType extends never, ...> is easy to infer, it doesn't prevent type inference of the schema type, which is what was needed to then infer the ValidationFunction type.

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