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

JTDDataType signature for compile and verify #1489

Closed
erikbrinkman opened this issue Mar 13, 2021 · 3 comments
Closed

JTDDataType signature for compile and verify #1489

erikbrinkman opened this issue Mar 13, 2021 · 3 comments

Comments

@erikbrinkman
Copy link
Collaborator

What version of Ajv you are you using?

master (7 or 8)

What problem do you want to solve?

Add signatures to AjvJTD's compile and verify methods that would support something like:

const schema = { ... } as const
const verify = ajv.compile(schema)
if (verify(data)) {
    const typed: JTDDataType<typeof schema> = data // no type errrors
}

What do you think is the correct solution to problem?

Note entirely sure, opening this up for discussion, versus continuing to talk on PR #1458.

I've been looking at how to do this, and I'm starting by implementing SomeJTDSchemaType and modifying the signature of compile. Previously I remember talking about having the AjvJTD subclass have different type arguments from the core Ajv class. I vaguely remember you not wanting to. Is there a reason you don't want to do this? Is there a significant cost to the super call?

Will you be able to implement it?

Yes

@epoberezkin
Copy link
Member

7 or 8

It's ok to add to 7 I think

implementing SomeJTDSchemaType and modifying the signature of compile

That seems ok, as you suggested in the PR there would be extra signatures in the core class:

compile<T extends SomeJTDSchemaType >(schema: T): ValidateFunction<JTDDataType<T>>;

having the AjvJTD subclass have different type arguments from the core Ajv class

some risk of divergence, additional complexity, some cost of extra call. Is there a reason why adding extra signature could be bad? Also, If JTD type signature were to be in the subclass why would JSON Schema signature be in core?

I do not plan many schema languages, so keeping it in core seems ok.

Also, I just never liked it when I had to add any executable code to satisfy type system/improve type safety :) It would have been nice if typescript had some zero-cost abstractions to add method overloads in subclasses...

@erikbrinkman
Copy link
Collaborator Author

yeah, those are good points. I'll see why I can do. There's some complexity that seems to come from the mingling of JTDDataType and ValidateFunction that I'm trying to understand. Will update. My guess is this won't make it in until 8.

@erikbrinkman
Copy link
Collaborator Author

The more I've played with this, the more it seems that JTDDataType is teetering right at the limits of typescripts complexity. It works when used by itself, but adding one more indirection of inferring it as a function argument makes typescript fail.

The solution probably lies in making it a little simpler if possible, or maybe having compile only work for definitions without refs, as that seems to be the cause of a lot of the complexity.

I'm still playing around with ideas, because I think it would be really cool for

const schema = ... as const
const validator = ajv.compile(schema)

to just work without additional typing.

erikbrinkman added a commit to erikbrinkman/ajv that referenced this issue Apr 12, 2021
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
andriyl pushed a commit to Redocly/ajv that referenced this issue Jun 16, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants