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

Composite not working with union of object types #789

Closed
aleclarson opened this issue Mar 13, 2024 · 11 comments · Fixed by #791
Closed

Composite not working with union of object types #789

aleclarson opened this issue Mar 13, 2024 · 11 comments · Fixed by #791

Comments

@aleclarson
Copy link

TypeScript Link Here

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

const Foo = Type.Object({
    foo: Type.String(),
})

const Bar = Type.Object({
    bar: Type.String(),
})

const Result = Type.Composite([
    Type.Union([Foo, Bar]),
    Type.Object({
        result: Type.Number(),
    })
])

type Result = Static<typeof Result>
/*
  type Result = {
    result: never; 😢
  }
*/
@aleclarson
Copy link
Author

aleclarson commented Mar 13, 2024

Okay, this behavior makes sense, as it matches TypeScript, but is there something like Composite that outputs a Union type when a union is passed to it? Or maybe that's how Composite should work and we need to fix it?

@sinclairzx81
Copy link
Owner

@aleclarson Hi, thanks for reporting.

Yeah, this does look like a bug / oversight to me. I've pushed a quick fix for this on the composite branch if you would like to test it out. The fix aligns TS union evaluation for Indexed type specifically (so actually applies both to Composite and Mapped types).

The updates fixes for the following 2 cases.

Updates

master...composite

Case 1: Non-Overlapping Union

The following evaluates to { a: number } as the union of A and B cancel out due to non-overlapping properties.

TypeScript Reference

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

const A = Type.Object({ a: Type.String() })
const B = Type.Object({ b: Type.String() }) // non-overlapping property
const C = Type.Composite([
  Type.Union([A, B]),
  Type.Object({
    a: Type.Number(),
  })
])

type C = Static<typeof C> // type C = {
                          //   a: number;
                          // }

Case 2: Overlapping Union

The following evaluate to { a: never } as the union of A and B both have overlapping properties (both string). However the Composite type evaluates to { a: never } due to the latter intersection evaluation of string & number for the property a

TypeScript Reference

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

const A = Type.Object({ a: Type.String() })
const B = Type.Object({ a: Type.String() }) // overlapping property
const C = Type.Composite([
  Type.Union([A, B]),
  Type.Object({
    a: Type.Number(),
  })
])

type C = Static<typeof C> // type C = {
                          //   a: never;
                          // }

Will do a bit more testing on this before publishing. If you can test out the updates against your local types, any feedback / additional bugs reported would be amazing!

Thanks again!
S

@sinclairzx81
Copy link
Owner

@aleclarson See PR #791 for these updates. This PR also adds path to transform errors.

@aleclarson
Copy link
Author

I don't think I understand what TS feature Composite is meant to mirror, but I guess it's not what I'm looking for, as I want the output type to be this:

type T =
  | { foo: string; result: number }
  | { bar: string; result: number }

So maybe a Spread type is warranted? If at least one TUnion is passed to Spread, a union type would be returned. If only TObject types are passed to Spread, then an object type is returned.

@sinclairzx81
Copy link
Owner

sinclairzx81 commented Mar 13, 2024

So maybe a Spread type is warranted? If at least one TUnion is passed to Spread, a union type would be returned. If only TObject types are passed to Spread, then an object type is returned.

Mmm, I don't think I'm going to be able to add additional types to TypeBox (like Spread) that would help map the original Foo, Bar and Result into the given T structure above (as this kind of transformation is quite specialized), however you could try something like the following.

TypeScript Link Here

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

type TCompositeUnion<T extends TSchema[], U extends TSchema, Acc extends TSchema[] = []> = 
  T extends [infer L extends TSchema, ...infer R extends TSchema[]]
    ? TCompositeUnion<R, U, [...Acc, TComposite<[L, U]>]>
    : TUnion<Acc>

function CompositeUnion<T extends TSchema[], U extends TSchema>(t: [...T], u: U): TCompositeUnion<T, U> {
  return Type.Union(t.map(schema => Type.Composite([schema, u]))) as never
}

// Example

const T = CompositeUnion([
    Type.Object({ foo: Type.String() }),
    Type.Object({ bar: Type.String() })
], Type.Object({ result: Type.Number() }))

type T = Static<typeof T> // type T =
                          //  | { foo: string; result: number }
                          //  | { bar: string; result: number }

Unfortunately, the above does require a little bit of type programming to make work, but it is generally the recommended way to approach specialized constructs if you need them. There may be other ways to express this type also, so have an experiment to see if there is a better way to express the types specific to your use case based on the above.


Composite

I don't think I understand what TS feature Composite is meant to mirror, but I guess it's not what I'm looking for, as I want the output type to be this ...

The Composite type is essentially a Evaluated Intersect type. It performs a similar role to the Simplify type you submitted on #784 in that it takes a potentially unevaluated type (for example an Intersect like A & B) and transforms it into a singular Object type whose properties are evaluated using the Intersect/Union rules of TS (this transform is both a static and runtime transform)

The Composite type was originally added to TypeBox to provide a backwards compatibility option when it started support for Json Schema draft 2019-09 (which changed the way intersections get represented at runtime). Since then however, the type has been enhanced several times (as users tend to prefer the singular Object representation over the allOf) and is generally viewed as an alternative to Intersect. However the type tends to share more in common with Type.Mapped such that the following two types A and B are actually equivalent.

const T = Type.Intersect([
  Type.Object({ x: Type.Number() }),
  Type.Object({ y: Type.Number() })
])

// The following types A and B are equivalent. 

const A = Type.Composite([T])

const B = Type.Mapped(Type.KeyOf(T), K => Type.Index(T, K))

In mapping scenarios, (such as the CompositeUnion above) the type can also sometimes be useful as a utility type and is generally faster to compute in the type system than Mapped (which is VERY expensive). As for mapping to a TS feature, it doesn't really (and has more to do with the underlying schema representation). However, future versions of TypeBox may recommend Mapped in favor of Composite if it's possible to get Mapped types optimal enough (and where the Composite type could be constructed in a similar way to the CompositeUnion shown above) and where Mapped is a representation of TS's Mapped Types feature .... but updates in this area are a long way off ...


So, the updates submitted on #791 are to do with fixing up a incorrect union evaluation that was giving the result: never; 😢 inference (which was actually a bug). This should be resolved with these updates, but will need a bit more testing before rolling out. (will be looking to publish over the next few days though)

Hope this helps! :)
S

@sinclairzx81
Copy link
Owner

sinclairzx81 commented Mar 14, 2024

@aleclarson So, I've spent a bit more time looking into this...specifically the following....

TypeScript Link Here

type S<T> = { [K in keyof T]: T[K] } & {}

type A = { a: string }
type B = { b: string }
type C = (A | B) & { c: number }

// This is what typebox implements
type R0 = { [K in keyof C]: C[K] } & {}  // type R0 = {
                                         //   c: number
                                         // }

// but where distributive rules apply for TS generics only (undocumented)
type R1 = S<C>                           // type R1 = { 
                                         //   a: string;
                                         //   c: number;
                                         // } | {
                                         //   b: string;
                                         //   c: number;
                                         // }

So, TypeBox supports the R0 but not the R1 (as there isn't a way (afaik) for TB to tell if types are being composed via generic or raw type (as is the case for R0)). However, I do think this is something worth exploring in future (just not part of this issue/PR as it's likely going to take a lot of research to figure out and get right). Just note that the implementation of R1 (as the default) would be extremely expensive to compute in the type system (via Mapped or Indexing), as TB static types would need to distribute over each union variant (I'm not sure how feasible this would be, but am considering a "fast path" specifically for Composite, if one is possible)

For now, the rather awkward CompositeUnion type above should get the job done. Alternatively, If you're not concerned about the static type and runtime type lining up, you can use Unsafe + Simplify in the following way...

TypeScript Link Here

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

export type Simplify<T> = { [K in keyof T]: T[K] } & {}

export function SimpliedIntersect<T extends TSchema[]>(schemas: [...T]) {
  return Type.Unsafe<Simplify<Static<TIntersect<T>>>>(Type.Intersect(schemas))
}

const T = SimpliedIntersect([                   
  Type.Union([
    Type.Object({ a: Type.String() }),
    Type.Object({ b: Type.String() })
  ]),
  Type.Object({ c: Type.Number() })
])

type T = Static<typeof T>      // type T = {
                               //   a: string;
                               //   c: number;
                               // } | {
                               //   b: string;
                               //   c: number;
                               // }

This may prove the simplest option.

@aleclarson
Copy link
Author

Hmm, I tried reproducing the issue I encountered, but couldn't.. 😅

https://stackblitz.com/edit/stackblitz-starters-wxdjg2?file=main.ts

Here's the reproduction code for posterity:

import { Type } from '@sinclair/typebox';
import { Value } from '@sinclair/typebox/value';

const Test = Type.Object({
  foo: Type.Optional(Type.Union([Type.String(), Type.Array(Type.String())])),
});

console.log(Value.Decode(Test, {}));
console.log(Value.Decode(Test, { foo: undefined }));
console.log(Value.Decode(Test, { foo: '' }));
// console.log(Value.Decode(Test, { foo: 0 }));

Though, I did notice something else.. It seems that Value.Decode(Test, { foo: undefined }) should be throwing an error, but it doesn't?

@sinclairzx81
Copy link
Owner

sinclairzx81 commented Mar 14, 2024

Though, I did notice something else.. It seems that Value.Decode(Test, { foo: undefined }) should be throwing an error, but it doesn't?

Edit: Response specific to issue #782 (click to expand)

Ah yeah, you will need to switch on exactOptionalPropertyTypes to have it throw an error here. By default, it is permissive of undefined for optional (same as TS). Generally you would want to configure tsconfig.json and the TB assert policy to be in sync (I usually leave this option off)

import { Type } from '@sinclair/typebox';
import { Value } from '@sinclair/typebox/value';
import { TypeSystemPolicy } from '@sinclair/typebox/system'

TypeSystemPolicy.ExactOptionalPropertyTypes = true

const Test = Type.Object({
  foo: Type.Optional(Type.Union([Type.String(), Type.Array(Type.String())])),
});

console.log(Value.Decode(Test, { foo: undefined })); // now throws

@aleclarson
Copy link
Author

aleclarson commented Mar 14, 2024

Oops I posted that comment in the wrong thread!

Should've been in #782

@sinclairzx81
Copy link
Owner

sinclairzx81 commented Mar 14, 2024

@aleclarson Heya,

So have been thinking about this issue more, and I don't think I'll be able to update Composite to support the R1 type listed above. Rather, I think this probably signals the need for a new type, which Ill refer to as Evaluate (as this is the internal name TB gives to it's own Simplify TS type)

The difficulty here is that it's not possible to determine whether a type should be composited using distributive / non-distributive rules (noting that TS differentiates these cases based on if a type is a generic type parameter or a concrete type). In TypeBox all the types are concrete.

This Evaluate type (which has loosely been explored before here) would effectively perform a Simplify operation using distributive rules, with the envisioned usage as follows.

const T = Type.Intersect([                   
  Type.Union([
    Type.Object({ a: Type.String() }),
    Type.Object({ b: Type.String() })
  ]),
  Type.Object({ c: Type.Number() })
])

const E = Type.Evaluate(T)   // const E: TUnion<[
                             //   TObject({
                             //     a: TString,
                             //     c: TNumber
                             //   }),
                             //   TObject({
                             //     b: TString,
                             //     c: TNumber
                             //   })
                             // })

This type would line up with future goals of the project, as well as act as a general backing type for property evaluation / normalization (which TB currently uses the fairly awkward and not very powerful EvaluatedUnion and EvaluatedIntersect mapping functions). Replacing these with a more general implementation has been on the roadmap for a while.


So, this type likely won't be supported for quite some time (as it's going to take a lot of effort to implement correctly) but should make it in at some point in the future (of which ill ping this thread when that time comes). In the interim, the recommendation will be to use either the CompositeUnion or SimpliedIntersect workarounds above.

@sinclairzx81
Copy link
Owner

@aleclarson Heya,

I've gone ahead and published the fix for this on 0.32.16 (non-distributive union). This probably isn't the expected output type you were hoping for (so there's the SimpliedIntersect and CompositeUnion to use in the meantime).

I've actually spent a few days looking at distributive union transforms, and I am going to implement this. I think realistically, once implemented it would be applicable to Composite. The actual implementation fits under the umbrella of "Evaluated Types", which applies both to Union and Intersect. But to achieve type evaluation correctly, there needs to be quite a bit of type infrastructure written (and I'm still in the process of working through some of these details)

Will notify this thread once any progress has been made on this front, but for now, the non-distributive (or mapped) composite type is the best TB can offer .. for now.

Will keep you posted.

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

Successfully merging a pull request may close this issue.

2 participants