Skip to content

No typescript error generated when using nested TypeBox types #636

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

Closed
sanderalvin opened this issue Oct 17, 2023 · 14 comments
Closed

No typescript error generated when using nested TypeBox types #636

sanderalvin opened this issue Oct 17, 2023 · 14 comments

Comments

@sanderalvin
Copy link

sanderalvin commented Oct 17, 2023

Hello

When using nested TypeBox types, then in some cases TypeScript errors are not generated.

Here is quite small code snippet where the issue is reproduced (on line 39 in problematicFunction body).

Using latest version "@sinclair/typebox": "0.31.17"
Using typescript version "typescript": "5.2.2"

import { Static, Type } from '@sinclair/typebox';

export type OA = Static<typeof OA>;
export const OA = Type.Object({
	uuid: Type.String(),
});

export type OAEnriched = Static<typeof OAEnriched>;
export const OAEnriched = Type.Object({
	uuid: Type.String(),
	uuid2: Type.Union([Type.Null(), Type.String()]),
});

export const OS = Type.Object({
	items: Type.Array(OA),
});

export type OSEnriched = Static<typeof OSEnriched>;
export const OSEnriched = Type.Object({
	items: Type.Array(OAEnriched),
});

export type OR = Static<typeof OR>;
export const OR = Type.Object({
	things: Type.Array(OS),
});

export type OREnriched = Static<typeof OREnriched>;
export const OREnriched = Type.Object({
	things: Type.Array(OSEnriched),
});

// when you comment out this function, then error from problematic function also disappears
function showProblem(or: OR): OREnriched {
	return or;
}

function problematicFunction(ors: OR[]): OREnriched[] {
	return ors; // this should trigger TS error, function signature expects OrderEnriched[] but Order[] is returned
}

When using native TS types instead of TypeBox, then the issue does not exist:

export type OA = {
	uuid: string;
};

export type OAEnriched = {
	uuid: string;
	uuid2: string | null;
};

export type OS = {
	items: OA[];
};

export type OSEnriched = {
	items: OAEnriched[];
};

export type OR = {
	things: OS[];
};

export type OREnriched = {
	things: OSEnriched[];
};

function showProblem(or: OR): OREnriched {
	return or;
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
function problematicFunction(ors: OR[]): OREnriched[] {
	return ors; // this  triggers TS error, which is good!
}

@sanderalvin
Copy link
Author

sanderalvin commented Oct 17, 2023

Coworker made even a smaller isloation

@sinclairzx81
Copy link
Owner

sinclairzx81 commented Oct 17, 2023

@sanderalvin Hi, thanks for reporting

Yes, this is a very unusual issue (and I'm somewhat at a loss to explain it other than it being a compiler optimization for deeply nested types). I do have a vague recollection of seeing this issue a few years back on an earlier version of the compiler, and I don't recall there being a easy solution (not without TB dropping functionality)

I've submitted a ticket to the TypeScript repo to dig a bit deeper into the issue (and to get some insights from the community). You can track things on at the following link.

microsoft/TypeScript#56138.

Workaround

In the meantime, you can get around this issue by implementing your own version of Static<T> (which is very easy to do). See the following link which resolves the issue.....

TypeScript Link Here

import { Type, TSchema } from '@sinclair/typebox'

type Static<T extends TSchema> = T['static']

Just note this version of Static<T> won't support recursive type inference, but all other types should work as expected.

Hope this helps
S

@RyanCavanaugh
Copy link

I'm still working on the bug from the TypeScript side, but a key part of this is P here:

export type Static<T extends TSchema, P extends unknown[] = []> = (T & { params: P })['static']

Intersecting { params: P } can't change the type of (T & { params: P })['static'] - what's it doing here?

@RyanCavanaugh
Copy link

Thinking on it more, I guess it can cause it to become never

@sinclairzx81
Copy link
Owner

sinclairzx81 commented Oct 17, 2023

@RyanCavanaugh Hey, thanks for taking a look into this.

Intersecting { params: P } can't change the type of (T & { params: P })['static'] - what's it doing here?

The P parameter is being used specifically for recursive inference (via HKT intersect). The P is used to capture the This argument (and return) in the following type.

TypeScript Link Here

const T = Type.Recursive(This => Type.Object({
  id: Type.String(),
  nodes: Type.Array(This)
}))

With the supporting types...

export interface TThis extends TSchema {
  [Kind]: 'This'
  static: this['params'][0] // captured here
  $ref: string
}
export type TRecursiveReduce<T extends TSchema> = Static<T, [TRecursiveReduce<T>]>
export interface TRecursive<T extends TSchema> extends TSchema {
  [Hint]: 'Recursive'
  static: TRecursiveReduce<T>
}

...

declare function Recursive<T extends TSchema>(callback: (thisType: TThis) => T): TRecursive<T>

It is possible to omit intersection from (T & { params: P })['static'], and just T['static'] but would come at a cost of losing singular recursive inference in TypeBox (although there may be more elegant ways to express this)

@sinclairzx81
Copy link
Owner

sinclairzx81 commented Oct 18, 2023

@sanderalvin
Copy link
Author

sanderalvin commented Oct 18, 2023

Thank you for investigating this @sinclairzx81 and @RyanCavanaugh!

Am I understanding correctly that this PR microsoft/TypeScript#55638 fixed the issue on TS side and it will be fixed with the next TS version update?

I will use the workaround until then time

@sinclairzx81
Copy link
Owner

@sanderalvin Hi,

Am I understanding correctly that this PR microsoft/TypeScript#55638 fixed the issue on TS side and it will be fixed with the next TS version update?

Not exactly. From my understanding, the microsoft/TypeScript#55638 PR was an updated compiler strategy for better handling deeply mapped types (where the compiler would give up checking deep properties after a few levels). Unfortunately, it appears TB is unable to take advantage of the updated compiler strategy due to the way it handles recursive mapped inference.

As this issue seems to be related to compiler internals, there isn't much TypeBox can do to resolve things at a library level (at least there's no trivial resolution to the issue I can think of). There may however be an alternate inference strategy offered by the TS team that may prove more conducive towards internal compiler optimizations, will need to track microsoft/TypeScript#56138 for updates and commentary from the team and community regarding this.

For now, the recommendation would be to leverage the user defined Static<T> if you need exact assertions for deep objects.
Hope this brings some insights
Cheers
S

@sinclairzx81
Copy link
Owner

Possible library level workarounds (but not sure how well this would scale)

Workaround 1 (Inline)

Workaround 2 (Integrated)

@sinclairzx81
Copy link
Owner

Compiler level fix has become available at the following PR

microsoft/TypeScript#56169

@sinclairzx81
Copy link
Owner

sinclairzx81 commented Oct 24, 2023

@sanderalvin Hi,

Hey, can confirm this issue has been resolved on the nightly builds of TypeScript. You can test by installing the following.

$ npm install typescript@next  # installs TS 5.3-dev

I'm not sure when TypeScript 5.3 is scheduled for release, but it's unlikely I'll be updating TypeBox with the above workarounds (as they would almost certainly implicate performance). For now, the recommendation will be to keep on with the user defined Static<T> or simply typeof T.static if you need deep property assertions.

const T = Type.String()

type T = typeof T.static // this also works

I may close up this issue for now as there is nothing to action in TypeBox, but will be keep an eye on the TS 5.3 release and possibly notify on this thread (just as a reminder to upgrade).

Thanks again for reporting this issue!
All the best
S

@sanderalvin
Copy link
Author

@sinclairzx81

I tried with a TS nightly build and I cannot concur that the issue is 100% fixed.

Check out this example

I am expecting a TS error on line 58, but it is not there.

@sinclairzx81
Copy link
Owner

sinclairzx81 commented Oct 31, 2023

@sanderalvin Hi,

Hmm, have also tested and also note the behavior (on 5.3.0-dev.20231031). These issues appear very much related to compiler level optimizations (and possibly caching issues) and there isn't much I can do to resolve at a TypeBox level (not without dropping recursive inference or potentially hindering inference performance (as per workaround)).

There is however a way to get TS reporting the error, either using the custom Static (as mentioned previously), or you can implement things this way (expressing the return type as a generic)

TypeScript Link Here

function bar<T extends BigTypeboxType[]>(param: SmallTypeboxType[]): T {
    return param; // this now errors
}

While not ideal, this forces TS to correctly evaluate the type.

It would be worth submitting this issue to the TypeScript repository as they seemed fairly receptive to resolving this problem (and it was flagged as a TS bug, not necessarily a TypeBox one). At most, TypeBox can produce an accurate type (as reported by the language service, but statically asserting that type is very much the responsibility of the compiler).

Did you want to submit a new issue to the TS repo?

@sanderalvin
Copy link
Author

Created a issue: microsoft/TypeScript#56291

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

No branches or pull requests

3 participants