ajv-validator / ajv

The fastest JSON schema Validator. Supports JSON Schema draft-04/06/07/2019-09/2020-12 and JSON Type Definition (RFC8927)
https://ajv.js.org
MIT License
13.47k stars 864 forks source link

chore: update typescript to 5.4.3 #2404

Closed jasoniangreen closed 1 month ago

jasoniangreen commented 2 months ago

What issue does this pull request resolve?

What changes did you make?

Is there anything that requires more attention while reviewing? The change the the Nullable type.

jasoniangreen commented 2 months ago

Hi @erikbrinkman, EP suggested that you might be able to offer some guidance on this TypeScript error. No pressure though of course. This is only after upgrading to TypeScript@5.4.3.

Type '{ readonly type: "string"; }' is not assignable to type '{ type: "string"; } & StringKeywords & { allOf?: readonly UncheckedPartialSchema<any>[] | undefined; anyOf?: readonly UncheckedPartialSchema<any>[] | undefined; ... 4 more ...; not?: UncheckedPartialSchema<...> | undefined; } & { ...; } & { ...; }'.
        Property 'nullable' is missing in type '{ readonly type: "string"; }' but required in type '{ nullable: true; const?: null | undefined; enum?: readonly any[] | undefined; default?: any; }'.

The error seems to be referring to this line, but I can't fix it with any combination of adding the nullable property to that second item in the array. I tried adding the nullable prop to the string schema.

{type: "string", nullable: false} and {type: "string", nullable: true}

Whichever you put, however, you get either

Type 'false' is not assignable to type 'true'. or Type 'true' is not assignable to type 'false'.

Update 03/04/24: I managed to get everything working with one tiny change. However I don't have enough context to understand if this change is right or not. By changing the type to allow nullable to be optional, I am presuming that the code written in spec/types/json-schema.spec.ts is correct and the nullable shouldn't need to be passed, which sounds correct to me.

jasoniangreen commented 1 month ago

@epoberezkin fyi

erikbrinkman commented 1 month ago

Sorry for the delay here. Your fix is not correct, but I applaud tracking down something that works from these outrageous messages.

What you changed means if a type is undefined, you no longer need to mark it as nullable, but that's not correct, if a type is undefined, than the schema should indeed have nullable: true, it's not optional.

As far as the root cause, I'm still tracking it down. I find the typescript playground is helpful for figuring these things out.

The problem isn't in directly checking the definitions, it's actually in the way we define SomeJSONSchema, particularly for tuple types, and particularly for every member beyond the first of a tuple type. This manifests in the following way:

const _: SomeJSONSchema = {
    // ... and here ...
    type: "array",
    items: [{ type: "number" }, { type: "string", nullable: true }],
    minItems: 2,
    additionalItems: false,
}

type checks fine, but if you remove the nullable for the string item, it fails. As far as I can tell, this traces back to how we define:

type SomeJSONSchema = UncheckedJSONSchemaType<Known, true>
type Known = {
    key: string]: Known;
} | [Known, ...Known[]] | Known[] | number | string | boolean | null

particularly the tuple definition [Known, ...Known[]]

I need to understand better how typescript interprets this tuple expansion, but I think it went from being essentially a heterogeneous tuple, to being a tuple with one element, and then a homogenous array, which screws up the inference.

I think realistically the defintion of SomeJSONSchema is hacky, and should probably just be defined more manually, which will prevent these errors, but will make updates a little more difficult.

jasoniangreen commented 1 month ago

Hmm, interesting. In the meantime @erikbrinkman what about this bump instead? https://github.com/ajv-validator/ajv/pull/2406

Not sure if you're able to review PRs, but feel free to give it a tick if you think this is ok.

jasoniangreen commented 1 month ago

Closing for now as we have an update to 5.3.3 without any changes needed.