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

chore: update typescript to 5.4.3 #2404

Closed
wants to merge 2 commits into from

Conversation

jasoniangreen
Copy link
Collaborator

@jasoniangreen jasoniangreen commented Apr 1, 2024

What issue does this pull request resolve?

  • Update to latest TypeScript 5.4.3

What changes did you make?

  • Updated TS and two small changes to get it working.

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

@jasoniangreen
Copy link
Collaborator Author

jasoniangreen commented Apr 2, 2024

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 [email protected].

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 jasoniangreen marked this pull request as ready for review April 4, 2024 16:44
@jasoniangreen
Copy link
Collaborator Author

@epoberezkin fyi

@erikbrinkman
Copy link
Collaborator

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
Copy link
Collaborator Author

Hmm, interesting. In the meantime @erikbrinkman what about this bump instead?
#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
Copy link
Collaborator Author

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

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