Fix importing optional properties

[michaelcollabai]

Module types look like a great idea to simplify some of our recursive schemas.

I tried them out and noticed a problem with optional object properties:
When importing an object schema, the optional status is lost for all properties with the more complex types handled in TFromType.
This does not happen at run time as [OptionalKind] is carried over in the spread op in CreateType.
I added type and run time tests to verify my assumptions.

Hats off for building something so complex with such clean and readable code!

[michaelcollabai]

I think TReadonly works in a similar way so it probably has the same issue.
But I also see that the area is still under active development. Let me know if a fix is helpful.

[sinclairzx81]

@michaelcollabai Hi, thanks for the PR

I think Optional and Readonly inference should have been resolved on 0.34.1 (there's actually been a lot of extra work done to the Module implementation to support computed types, Readonly and Optional inference were added as part of the new TComputed implementation). Can you try the install @sinclair/typebox@latest and check if you still see the issue?

TypeScript Link Here

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

const Module = Type.Module({

  // Explicit
  A: Type.Object({
    x: Type.Optional(Type.Number()),
    y: Type.Optional(Type.Number()),
    z: Type.Optional(Type.Number()),
  }),

  // Partial
  B: Type.Partial(Type.Object({
    x: Type.Number(),
    y: Type.Number(),
    z: Type.Number(),
  })),

  // Ref Target
  T: Type.Object({
    x: Type.Number(),
    y: Type.Number(),
    z: Type.Number(),
  }),

  R: Type.Partial(Type.Ref('T'))
})

type A = Static<typeof A>
type B = Static<typeof B>
type T = Static<typeof T> // expect required
type R = Static<typeof R>

const A = Module.Import('A')
const B = Module.Import('B')
const T = Module.Import('T') // required
const R = Module.Import('R')

If you can provide a repro of the Optional types not coming through, will definitely flag that as an issue for fixing. But think things might be all good on the latest version.

Keep me posted
S

[michaelcollabai]

Hey @sinclairzx81

The bug only happens for properties like TArray or TObject. It does indeed work for basic types. This is still true for 0.34.7.
TypeScript link here

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

const Module = Type.Module({

  // Explicit
  A: Type.Object({
    x: Type.Optional(Type.Number()),
    a: Type.Optional(Type.Array(Type.Number())),
    o: Type.Optional(Type.Object({ foo: Type.Number() }))
  })
})

const A = Module.Import("A");
type A = Static<typeof A>
/*
type A = {
    x?: number | undefined;
    a: number[];
    o: {
        foo: number;
    };
}*/

The test case "Object 1" in the PR also fails at HEAD without my change.

I don't think this is how it should work, is it? Not ruling out that I don't understand the feature, but it seems off to me that the state in type diverges from run time.

[sinclairzx81]

@michaelcollabai Hi!

Ah! Yes, this is a totally a bug (thanks for the follow up + repro!). This fix will need to be applied to both Optional and Readonly. You will want to update both FromType and TFromType just to keep these mappings symmetric (even if the FromType is just a pass through of the type, ill be coming back to this at a later time)

// ...
Type extends TOptional<infer Type extends TSchema> ? TOptional<TFromType<ModuleProperties, Type>> :
Type extends TReadonly<infer Type extends TSchema> ? TReadonly<TFromType<ModuleProperties, Type>> :

Will need to test for multiple assigned modifiers (ReadonlyOptional). Can drop a test in there for the following structure

const Module = Type.Module({
  T: Type.Object({
    // will want to ensure the unwrap works for Readonly and Optional applied modifiers
    x: Type.ReadonlyOptional(Type.Array(Type.Literal(1))), 
    y: Type.Readonly(Type.Array(Type.Literal(1))),
    z: Type.Optional(Type.Array(Type.Literal(1))),
    w: Type.Array(Type.Literal(1))
  })
})

If you can get the Readonly mapping in + this test, this PR should be good to go.
Thanks again, this was good spotting.
S

[sinclairzx81]

@michaelcollabai Hi,

Have gone ahead and fixed up Readonly on this PR and merged through (this issue was a breaking bug and wanted to get it sorted pretty quickly). Will be publishing the fix out to NPM push within the hour. Thanks again for finding this issue and for the PR to resolve, this was really good catch (and happy to hear the code wasn't to bad to navigate!)

Will notify once published
Cheers!
S

[sinclairzx81]

@michaelcollabai Hiya,

Fix published on 0.34.8. Thanks again!

All the best
S