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

Tuple option types can get annoying. #279

Closed
ivands opened this issue Dec 1, 2023 · 11 comments
Closed

Tuple option types can get annoying. #279

ivands opened this issue Dec 1, 2023 · 11 comments
Assignees
Labels
enhancement New feature or request priority This has priority

Comments

@ivands
Copy link

ivands commented Dec 1, 2023

Using the union, picklist, and other functions can get annoying when using dynamic arguments.
Example:

const values = [1, 2, 3] as const
const schema = union(values.map(i => literal(i)))

Examples like this fail because even though the values array is a tuple, mapping the array will make it a union type instead.
To be fair, I would say TS is failing us here.
But because of this problem, we need to make complex types to get this working.

Could we remove the tuple argument type & make it an array?

Original:

export type UnionOptions = [BaseSchema, BaseSchema, ...BaseSchema[]];

Proposal:

export type UnionOptions = BaseSchema[];
@fabian-hiller fabian-hiller self-assigned this Dec 1, 2023
@fabian-hiller fabian-hiller added enhancement New feature or request priority This has priority labels Dec 1, 2023
@fabian-hiller
Copy link
Owner

Thank you for creating this issue. I am aware of the problem. I will look into it and then make a decision.

@ivands
Copy link
Author

ivands commented Dec 2, 2023

Sadly, TS isn't smart enough, in that you can't transform a tuple and expect a tuple back.

Example:

const v1 = [1, 2, 3] as const // type [1, 2, 3]
const v2 = v1.map(v => v * 2) // type number[]

The TS compiler can't figure out the actual runtime output of the map function, and the tuple will collapse into a more generic array type.
This means we will always need to write the tuple argument types ourselves. (Oh help me god)

Because the tuple expects at least 2 arguments, the argument type becomes bigger than it has to be.
Example:

// This doesn't work because it's not a tuple.
const values = [1, 2, 3] as const
const literals = values.map(v => literal(v)) as LiteralSchema<typeof values[number]>[]
const schema = union(literals)

// This works.
const values = [1, 2, 3] as const
const literals = values.map(v => literal(v)) as [ LiteralSchema<typeof values[number]>, LiteralSchema<typeof values[number]> ]
const schema = union(literals)

Overall not a great developer experience.

@fabian-hiller
Copy link
Owner

Yes, I agree. I will look into this in the next few days. Thanks for your research!

@dawidurbanski
Copy link

dawidurbanski commented Dec 5, 2023

FYI this should work in plain TypeScript

const v1 = [1, 2, 3] as const // type [1, 2, 3]
const v2 = v1.map(v => v * 2) as unknown as typeof v1 // type [1, 2, 3]

therefore this also should be ok with Valibot

const values = [1, 2, 3] as const // type [1, 2, 3]
const literals = values.map((v) => literal(v)) as unknown as typeof values // type [1, 2, 3]
const schema = union(literals as unknown as UnionOptions) // works fine

still ugly and not best developer experience, but hey, no need to type tuple arguments ourselves

basically "Sadly, TS isn't smart enough", so let's be smarter than TS and tell it what's in there

@ivands
Copy link
Author

ivands commented Dec 5, 2023

@dawidurbanski You are completely losing type inference this way.

const schema = union(literals as unknown as UnionOptions)
type I = Input<typeof schema> // ???

Plus, you lost type safety by casting everything. (The reason for TS in the first place).
I think that if people are typecasting to use the library, we made a mistake with the API.

@dawidurbanski
Copy link

Oh, right. I didn't thing this through :D

@fabian-hiller
Copy link
Owner

Will have a closer look at this issue in the next days.

@fabian-hiller
Copy link
Owner

fabian-hiller commented Dec 8, 2023

I am still unsure if I want to change the typing of the argument from a tuple to an array. The disadvantage is less type safety and the advantage is a better DX when using .map(...). Until I make a decision, I recommend using a small helper function that ensures type safety. Currently there is still a type error with the following solution but with v0.23.0 I will fix this problem.

  1. Add literalsFrom util to your project
import * as v from 'valibot';

/**
 * Converts a data tuple to a tuple of literal schemas.
 *
 * @param tuple The data tuple.
 *
 * @returns A tuple of literal schemas.
 */
export function literalsFrom<
  TTuple extends Readonly<[v.Literal, v.Literal, ...v.Literal[]]>
>(
  tuple: TTuple
): {
  [TTupleKey in keyof TTuple]: v.LiteralSchema<TTuple[TTupleKey]>;
};

/**
 * Converts a data tuple to a tuple of literal schemas.
 *
 * @param tuple The data tuple.
 * @param key The data key.
 *
 * @returns A tuple of literal schemas.
 */
export function literalsFrom<
  TTuple extends Readonly<
    [
      Record<TDataKey, v.Literal>,
      Record<TDataKey, v.Literal>,
      ...Record<TDataKey, v.Literal>[]
    ]
  >,
  TDataKey extends keyof TTuple[number]
>(
  tuple: TTuple,
  key: TDataKey
): {
  [TTupleKey in keyof TTuple]: v.LiteralSchema<TTuple[TTupleKey][TDataKey]>;
};

export function literalsFrom<
  TTuple extends Readonly<[any, any, ...any[]]>,
  TDataKey extends keyof TTuple[number]
>(tuple: TTuple, key?: TDataKey) {
  return tuple.map((literal) => v.literal(key ? literal[key] : literal));
}
  1. Use literalsFrom in your schema
import * as v from 'valibot';
import { literalsFrom } from '~/utils';

// With direct values
const options1 = ['foo', 'bar'] as const;
const Schema1 = v.union(literalsFrom(options1));

// With simple object
const options2 = [{ value: 'foo', label: 'Foo' }, { value: 'bar', label: 'Bar' }] as const;
const Schema2 = v.union(literalsFrom(options2, 'value'));

@ivands
Copy link
Author

ivands commented Dec 8, 2023

To be honest, I wouldn't say I like this solution.

  1. We now need to learn about helper functions to work around TS.
  2. This helper function example only fixes the single-use case of providing a list of literals. Unions can have all kinds of types inside them.
  3. I would argue that the type-safety provided by the tuple isn't necessary.
    It might be weird to create a union type with a single entry or to use picklist with just a single string.
    But that's just what might happen when providing a dynamic list. (Sometimes the list might have 3 items & sometimes only 1).

I guess I just disagree with the safety provided by the tuple.
A minimum of 2 arguments feels wrong to me.

@fabian-hiller
Copy link
Owner

Thank you very much for your feedback. I'll think about it again.

@fabian-hiller
Copy link
Owner

fabian-hiller commented Dec 8, 2023

I changed it. v0.23.0 is now available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority This has priority
Projects
None yet
Development

No branches or pull requests

3 participants