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

using union as key of record adds undefined to the value type #492

Closed
bug-brain opened this issue Mar 19, 2024 · 14 comments
Closed

using union as key of record adds undefined to the value type #492

bug-brain opened this issue Mar 19, 2024 · 14 comments
Assignees
Labels
enhancement New feature or request intended The behavior is expected priority This has priority

Comments

@bug-brain
Copy link

import * as v from 'valibot';

const record = v.record(v.union([v.string()]), v.string());

const t: v.Output<typeof record> = { a: undefined }
//    ^ { [x: string]: string | undefined; }
//                              ^ should not be here
const result = v.safeParse(record, t);
console.log(result); // errors correctly
@fabian-hiller fabian-hiller self-assigned this Mar 19, 2024
@fabian-hiller fabian-hiller added the intended The behavior is expected label Mar 19, 2024
@fabian-hiller
Copy link
Owner

This is the intended behavior when string is not used directly as the key schema. In your case, I recommend removing union and using only string. This should solve your problem. union can be used with literal to describe certain record keys, but record does not force you to specify them and therefore we mark it as optional in this case. Feel free to contact me if you have any further questions.

@fabian-hiller
Copy link
Owner

But please leave this issue open. I agree that it would be better to specify it as undefined only when literal is used inside of union. I will see if I can improve the types.

@fabian-hiller fabian-hiller added enhancement New feature or request priority This has priority labels Mar 19, 2024
@bug-brain
Copy link
Author

How would I model the type Record<"a" | "b", number>? I would have expected v.record(v.union([v.literal("a"), v.literal("b")]), v.number()) but that makes the keys optional and adds undefined as you mentioned. The function required only works on ObjectSchema.

@fabian-hiller
Copy link
Owner

I would use object and not record:

import * as v from 'valibot';

const Schema = v.object({ a: v.number(), b: v.number() });

But maybe you are right and our current typing and implementation is wrong and we should only mark it as optional when optional is used for the value of the record.

@fabian-hiller
Copy link
Owner

@bug-brain I investigated this issue further. It is true that the behavior of Valibot is different from TypeScript and it would be nice to unify it. But there is a tradeoff. To support this behavior, record has to "understand" the schemas used and apply additional logic to ensure that every required entry is defined. This would increase the raw bundle size of the record function from 408 bytes to about 460 bytes (13%). Furthermore, this change would require us to limit the record key schema to string and picklist, as it would require much more code to handle other more "unpredictable" schemas such as union. That's probably also why Zod implemented it this way.

@fabian-hiller
Copy link
Owner

Since many Zod users are also complaining about this problem in #2623, I will investigate this further to find an appropriate solution.

@fabian-hiller
Copy link
Owner

I think I fixed it! Will test and investigate my code tomorrow. Thanks for bringing this up!

@fabian-hiller
Copy link
Owner

fabian-hiller commented Apr 17, 2024

Even if I found a solution, I am not sure if we should go this way as a library focused on bundle size. Supporting this behavior increases the bundle size of record, while the same behavior can already be achieved with object. Maybe we should limit the key argument of record to just string (or stick to the current behaviour with undefined) and provide a util function to convert a list of keys into the entries argument of object.

// `record` does only allow `string` as first argument
const Schema1 = v.record(v.string(), v.number());

// For explicit keys, `object` with `entriesFrom` is used instead
const Schema2 = v.object(v.entriesFrom(['foo', 'bar'], v.number()));

// Or without the util function
const Schema2 = v.object({ foo: v.number(), bar: v.number() });

@macmillen
Copy link

@fabian-hiller may I ask how much record would increase in bundle size?

@macmillen
Copy link

I am using a graphql-codegen-typescript-validation-schema to generate zod enums like this:

export const PosTypeEnumSchema = z.enum(['external', 'online', 'standard', 'telephone']);

That way I don't need to define all of my enum schemas manually.

Now I need a record with these enums as a key type and I don't want to re-define these keys by hand but instead use the generated z.enum. (the alternatives you showed unfortunately don't solve that issue)

@fabian-hiller
Copy link
Owner

The next version of Valibot will allow you to write the following code to solve your problem. Feel free to give me feedback on this in the long run.

import * as v from 'valibot';

const ListSchema = v.picklist(['external', 'online', 'standard', 'telephone']);

const ObjectSchema = v.object(v.entriesFromList(ListSchema.options, v.number()));

@fabian-hiller
Copy link
Owner

I am not a maintainer of Zod, so I cannot speak for it. I recommend asking the Zod team directly.

@macmillen
Copy link

Thank you, I'll do that :)

@fabian-hiller
Copy link
Owner

The next version of Valibot will allow you to write the following code to solve your problem

This is now implemented and 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 intended The behavior is expected priority This has priority
Projects
None yet
Development

No branches or pull requests

3 participants