zod
zod copied to clipboard
Fix branded Record keys in ZodRecord
Because of a quirk of the extends syntax of typescript conditional types, the previous version of this check for a branded key didn't work as expected.
Thank you @akomm for the explanation of why the extends sides need to be swapped at https://github.com/colinhacks/zod/pull/2287#issuecomment-1689922672
Originally I intended to remove the RecordType type entirely, since it
would seem initially that it is only there to work around earlier
versions of typescript not having the noUncheckedIndexedAccess option.
However, it is really meant to work around a problem with using ZodEnum
as the key, see this comment: https://github.com/colinhacks/zod/pull/2287#issuecomment-1676624932
~The RecordType conditional type seems to exist for handling the partialness of indexed objects. However that is handled by the tsconfig option noUncheckedIndexedAccess in [email protected] and later.~
Deploy Preview for guileless-rolypoly-866f8a ready!
Built without sensitive environment variables
| Name | Link |
|---|---|
| Latest commit | 28dc5c6c51dbb4fac58121f85e2818f602cc902b |
| Latest deploy log | https://app.netlify.com/sites/guileless-rolypoly-866f8a/deploys/64e9d40bd6632700088e4fbc |
| Deploy Preview | https://deploy-preview-2287--guileless-rolypoly-866f8a.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Many thanks for approving this! Super happy to get this annoyance fixed :)
Just to clarify for myself, when can I expect this to get merged and published?
@sangxxh Hey! Could we get this merged soon?
Unfortunately this is currently intentional. We need the Partial here because Zod doesn't do exhaustiveness checking across the values when you pass a ZodEnum as the key schema. in as the keySchema for a ZodRecord, Zod
const schema = z.record(z.enum(['a', 'b']), z.number());
type schema = z.infer<typeof schema>;
schema.parse({ a: 5 }); // passes
var x: schema = { a: 5 } // missing key 'b'
I agree this isn't ideal, and this will likely change in Zod v4.
Ahh I see, I guess I never figured out what the original reason for this weird type is. The problem I'm having with it is that it's messing up records that are keyed by a branded string. These cases are not meant to have partial applied to them, right?
This line probably was an earlier attempt at fixing this, however it doesn't work for me. Do you have an idea on how to fix this?
const schema = z.record(z.string().brand('some-branded-string'), z.number())
type schema = z.infer<typeof schema> // expected Record<string & BRAND<'some-branded-string'>, number>, got Partial<Record<string & BRAND<'some-branded-string'>, number>>
It is also breaking usage of exactOptionalProperties. This breakage was introduced and blocking my older project from upgrading zod at all. Also all branded keys partial.
The following does not work (extracted from zod code, conditionally mapping record type):
type Test<K> = [z.BRAND<string | number | symbol>] extends [K] ? true : false
type Key = string & z.BRAND<'Key'>
type R = Test<Key> // = false
Here's why:
For A extends B, A must be equal or a subset of B. Extends is a bit confusing, because we expect equal or less and not more (in the sense of distinctive items, ... set theory).
In example above, Key's inner type string intersects with the brand, which is a superset.
Hence, the relation must be rotated:
type Test<K> = [K] extends [z.BRAND<string | number | symbol>] ? true : false
type Key = string & z.BRAND<'Key'>
type R = Test<Key> // = true
Funny thing is, that non-finite keys, for example string, are inferred as non-partial: TS Playground
Which is absurd.
Can we at least get the fix through for brand:
export declare type RecordType<K extends string | number | symbol, V> = [
string
] extends [K]
? Record<K, V>
: [number] extends [K]
? Record<K, V>
: [symbol] extends [K]
? Record<K, V>
: [K] extends [BRAND<string | number | symbol>]
? Record<K, V>
: Partial<Record<K, V>>
Thanks for your comment @akomm! That makes a lot of sense as to why the earlier merged fix didn't really fix the problem.
I've included your suggested solution in a commit on this branch now, to see how it passes the test suite.
I'll do testing on my codebase tomorrow (UTC+3) with this idea, and let you all know
I've verified that the change suggested by @akomm does fix the problem we're having in our codebase, so I've amended the branch to fix that one check only, so that this won't affect the case with ZodEnum
@colinhacks would this be ok to merge now
What's the update here, @colinhacks?
Is there something besides "no time" blocking this? Some doubts or issues? Maybe we can help.
ping @colinhacks. is there anything I could do to help this get merged?
@googol do you have a sense of how a Record<someStringLiteralUnion, boolean> could be enforced via the current state of zod? ran into this issue just now and a bit discouraged that this has been stale for so long. asking both to anticipate potential objections from @colinhacks and to see if you've found a better short-term alternative to manually defining this schema as an object
Unfortunately I have nothing :/
Having partial records when the keys are a literal union is by design. It reflects the way zod parses the records : it does not make sure that every value of the union is represented in the record's keys. For the same reason, we get the same behaviour with branded keys because such keys could have a literal union type under the hood.
I have not given many thoughts to this, but maybe using maps instead if records in such use cases is the way to go.
@fwoelffel
ZOD does not seem to be very exact about doing what you say, as I've explained in this comment: https://github.com/colinhacks/zod/pull/2287#issuecomment-1689922672
Using an infinite set (=type string) as key results in a non-partial Record. Example from the above comment: TS Playground
For obvious reason, ZOD can not test that all infinite possible keys are actually present. Yet the type suggests otherwise.
This issue has a long history actually. There were multiple topics regarding the exactOptionalPropertyTypes option in TS. It was initially ignored, typically the issues closed. And then suddenly, out of nothing this partial record update was dropped. Of all possible solutions to this tricky problem, the worst was chosen, without taking any feedback from users, because the prior issues were ignored and closed mostly.
I make it short, after a lot of testing and trying, turns out exactOptionalPropertyTypes isn't quite good and not a good fit with noUncheckedIndexAccess. The best solution would be to mimic z.record to the Record TS behavior, which means literal key union are considered non partial. Anyone who want extra type safety with non-finite set keys can enable noUncheckedIndexAccess, here TS playground example with it enabled: TS PG**
**By the way, if you switch noUncheckedIndexAccess on and off in tsconfig on the playground, you need to make some change in the code for the type marks to update (add and remove a line)
My argument for this approach is that it would mimic 1:1 TS type safety level to runtime safety level in zod. If you disable noUncheckedIndexAccess in TS, you will miss the same sort of issues at runtime with zod as with TS. Zod does not make it worse. But now the developer has the option to get more or less safety by simply toggling the TS setting. There is no perfect solution, but this one at least gives control of safety to the developer and its sync with TS behavior.
afterthought
IMO this issue turns out to be more broad than initially anticipated. That is why as an afterthought we received the Partial<Record<..,..>> change. It would be great if this could be solved in a clean and straightforward way in zod 3.0. Mimic the behavior as explained above would mean that z.record with z.literal or z.union of z.literal keys need to be mapped into z.object under the hood.
There's clearly a lot of confusion about why ZodRecord is the way it is. The comment by @fwoelffel is exactly right:
Having partial records when the keys are a literal union is by design. It reflects the way zod parses the records : it does not make sure that every value of the union is represented in the record's keys.
It's impossible (in the general case) for Zod to take an arbitrary schema and make a list of literals that will pass validation. As such, it isn't possible for Zod to do exhaustiveness checking on the keys of a given input object, to make sure, say, all the elements of a ZodUnion appear in the object. That's why Zod makes all fields optional for literal keys.
My argument for this approach is that it would mimic 1:1 TS type safety level to runtime safety level in zod
Exactly, this is the goal. For maximum type safety, Zod should add | undefined when there are non-finite keys in z.record(). In practice most people don't currently have noUncheckedIndexedAccess defined and found this behavior surprising before the RecordType change.
It's important to note that Zod's isn't able to alter its runtime behavior based on your tsconfig.json. Your tsconfig doesn't exist once your code is bundled. So Zod needs to make a decision about what the inferred type is, and its runtime behavior should agree with the inferred type. This is also why it's not easy for Zod and z.object() to magically support exactOptionalPropertyTypes.
Let's talk about improvements I'd like to make to ZodRecord.
noUncheckedIndexedAccess
As @akomm has suggested, Zod can check when noUncheckedIndexedAccess is enabled (in the static domain, not runtime) and add | undefined to all inferred ZodRecord types including non-finite keys. I'm in favor of that.
Practically speaking most people don't have noUncheckedIndexedAccess enabled currently, so I still stand behind the decision to introduce RecordType and remove optionality from non-finite keys. But per @akomm's suggestion the optionality should still remain for users with noUncheckedIndexedAccess: true.
After some tinkering I found this can be done statically with the type alias below. If someone finds a less hacky way let me know.
declare const arg: {[k:string]: string};
type noUncheckedIndexedAccess = (typeof arg.qwerty) extends string ? false : true;
This is a change I'd land it in Zod 4 because it's potentially breaking.
Improved exhaustiveness checking
I'm not very enthusiastic about this, and it's very nontrivial. It's the same difficulty that exists with ZodDiscriminatedUnion, which I described here: https://github.com/colinhacks/zod/issues/2106
The problem boils down to this: given an arbitrary Zod schema, determine the set of literal values that exist that will pass validation. You need to do this in both the runtime and static domains.
In the static domain, this requires a recursive type alias. See partialUtil.DeepPartial for an example of what it looks like. It's very annoying and error-prone to unwrap all the ZodOptional, ZodNullable, ZodEffects, ZodReadonly, ZodUnion, ZodDiscriminatedUnion, instances that may be wrapping your ZodLiteral, ZodEnum, and ZodNativeEnum instances.
Same problem at runtime, this requires a recursive visitor pattern. Out of the gate, there's no way to make this compatible with any user-defined ZodType subclasses, since the logic would necessarily be strongly-coupled to the set of first-party Zod-defined subclasses.
Plus there are schemas for which this problem is literally unsolvable like z.literal("asdf").transform(val => val.toUpperCase() as "ASDF"). There's no way for Zod to determine at runtime that "ASDF" is a potentially valid input, so Zod can't correctly due exhaustiveness checking.
I could add in some custom logic to special-case the major use cases (passing a ZodEnum in as the key to z.record) but a half-baked solution like that is just a recipe for maintenance headaches and user frustration. Look at all the issues surrounding ZodDiscriminatedUnion to see that up close.
I will experiment with this regardless and see if I can land on something that addresses major use cases without undue complexity. But I don't think it's likely.
Limiting keySchema
This is probably what I should have done initially: limit KeySchema to ZodString | ZodNumber | ZodSymbol | ZodEnum | ZodNativeEnum. Then there is a well-defined subset of schema types, which makes all the implementation much simpler. This would certainly lead to some issues when people try to use ZodEffects, etc, but at least the API would be clear.