fabian-hiller / valibot

The modular and type safe schema library for validating structural data 🤖
https://valibot.dev
MIT License
5.88k stars 181 forks source link

Fix lazy generics in docs #707

Closed dearlordylord closed 1 month ago

dearlordylord commented 1 month ago

Hey, I noticed that in docs BinaryTreeSchema is parametrized with only input T

that causes type erasure for some types, notable for branded strings, such as:


const NonEmptyStringSchema = pipe(
  string(),
  minLength(1),
  brand('NonEmptyString')
);

type NonEmptyString = InferOutput<typeof NonEmptyStringSchema>;

const FileSystemCommonSchema = object({
  name: NonEmptyStringSchema,
});

type FileSystem = (
  | {
  readonly type: 'directory';
  readonly children: readonly FileSystem[];
}
  | {
  readonly type: 'file';
}
  ) & {
  readonly name: NonEmptyString;
};

const FileSystemDirectorySchema: GenericSchema<FileSystem & { type: 'directory' }> = intersect([
  FileSystemCommonSchema,
  object({
    type: literal('directory'),
    children: array(lazy(() => FileSystemSchema)),
  }),
]);

const FileSystemFileSchema = intersect([
  FileSystemCommonSchema,
  object({
    type: literal('file'),
  }),
]);

const FileSystemSchema = union([
  FileSystemDirectorySchema,
  FileSystemFileSchema,
]);

would cause type error because (I assume) FileSystem's name NonEmptyString being downcasted to string as an input and then assigned to the output as a default <---- this is only an assumption, I just went with my intuition and put GenericSchema<unknown, FileSystem & { type: 'directory' }> there and it works

fabian-hiller commented 1 month ago

This looks wrong to me. You get the TS error because your schema does not match the input type of FileSystem & { type: 'directory' }. Assigning unknown is not a good solution as it simply removes type safety.

Also, have a look at this guide. Often it is better to merge objects instead of using intersect.

dearlordylord commented 1 month ago

Thank you for checking it!

It will work when I change NonEmptyString to string though:

const NonEmptyStringSchema = pipe(
  string(),
  minLength(1),
  brand('NonEmptyString')
);

type NonEmptyString = InferOutput<typeof NonEmptyStringSchema>;

const FileSystemCommonSchema = object({
  name: string(),
});

type FileSystem = (
  | {
      readonly type: 'directory';
      readonly children: readonly FileSystem[];
    }
  | {
      readonly type: 'file';
    }
) & {
  readonly name: string;
};

const FileSystemDirectorySchema: GenericSchema<
  FileSystem & { type: 'directory' }
> = intersect([
  FileSystemCommonSchema,
  pipe(
    object({
      type: literal('directory'),
      children: array(lazy(() => FileSystemSchema)),
    }),
    check(
      (v) => new Set(v.children.map((c) => c.name)).size === v.children.length,
      'Expected unique names in the children'
    )
  ),
]);

const FileSystemFileSchema = intersect([
  FileSystemCommonSchema,
  object({
    type: literal('file'),
  }),
]);

const FileSystemSchema = union([
  FileSystemDirectorySchema,
  FileSystemFileSchema,
]);

Notice that I removed unknown here and changed every NonEmptyString to string. So, the code above just equalizes input and output types.

It will also work if I simply change unknown to Omit<FileSystem, 'name'> & { type: 'directory', name: string },

To me, the current example works only for types with no transformations; if transformations are involved it could be misleading, assigning input type to output type.

I wonder if it can be improved to work with support for transformed types.

dearlordylord commented 1 month ago

Side note, I digressed to intersect instead of merge because merge loses partialChecks (and I assume checks), not warning me about it. When .entries are used, all partialChecks wrappers are silently lost; I don't want to track it manually and choose between merge and intersect and ideally would like the type checker to do it for me!

example

const TemporalConcernUnsortedSchema = object({
  createdAt: DatetimeSchema,
  updatedAt: DatetimeSchema,
});

const TemporalConcernSchema = pipe(
  TemporalConcernUnsortedSchema,
  forward(
    partialCheck(
      [['createdAt'], ['updatedAt']],
      (input) => input.createdAt <= input.updatedAt,
      'createdAt must be less or equal than updatedAt'
    ),
    ['updatedAt']
  )
);

const UserSchema = object({
...TemporalConcernSchema.entries // partialCheck code ignored since
});

that's not strictly "type checking" issue but connascence between runtime and compile time

fabian-hiller commented 1 month ago

I wonder if it can be improved to work with support for transformed types.

The first generic of GenericSchema defines the input type and the second defines the output type. If you do not define an output type, the input type is used as the default.

Side note, I digressed to intersect instead of merge because merge loses partialChecks...

Yes, that's true. In this case it is necessary to apply the pipeline afterwards. I understand if intersect is easier to work with in this case.