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

Fix branded Record keys in ZodRecord #2287

Closed
wants to merge 1 commit into from

Conversation

googol
Copy link

@googol googol commented Apr 3, 2023

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 #2287 (comment)

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: #2287 (comment)

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.

@netlify
Copy link

netlify bot commented Apr 3, 2023

Deploy Preview for guileless-rolypoly-866f8a ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 28dc5c6
🔍 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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

@sangxxh sangxxh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome thank you! This will bring back the TypeScript compile time safety that we're desperately missing 🙂

@googol
Copy link
Author

googol commented Jun 26, 2023

Many thanks for approving this! Super happy to get this annoyance fixed :)

@googol
Copy link
Author

googol commented Jun 27, 2023

Just to clarify for myself, when can I expect this to get merged and published?

@googol
Copy link
Author

googol commented Aug 13, 2023

@sangxxh Hey! Could we get this merged soon?

@colinhacks
Copy link
Owner

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.

@googol
Copy link
Author

googol commented Aug 14, 2023

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>>

@akomm
Copy link

akomm commented Aug 23, 2023

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>>

@googol googol force-pushed the simpler-record-type branch from 9cdf371 to 6563e80 Compare August 23, 2023 21:36
@googol
Copy link
Author

googol commented Aug 23, 2023

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

@googol googol force-pushed the simpler-record-type branch from f1c6828 to cc9297f Compare August 26, 2023 10:06
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 colinhacks#2287 (comment)

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:
colinhacks#2287 (comment)
@googol googol force-pushed the simpler-record-type branch from cc9297f to 28dc5c6 Compare August 26, 2023 10:29
@googol googol changed the title Simplify the typing of ZodRecord Fix branded Record keys in ZodRecord Aug 26, 2023
@googol
Copy link
Author

googol commented Aug 26, 2023

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

@Stefandasbach
Copy link

What's the update here, @colinhacks?

@akomm
Copy link

akomm commented Oct 13, 2023

Is there something besides "no time" blocking this? Some doubts or issues? Maybe we can help.

@googol
Copy link
Author

googol commented Jan 17, 2024

ping @colinhacks. is there anything I could do to help this get merged?

@dkrieger
Copy link

@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

@googol
Copy link
Author

googol commented Jan 31, 2024

Unfortunately I have nothing :/

@fwoelffel
Copy link

fwoelffel commented Feb 22, 2024

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.

@akomm
Copy link

akomm commented Feb 26, 2024

@fwoelffel

ZOD does not seem to be very exact about doing what you say, as I've explained in this comment: #2287 (comment)

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.

@colinhacks
Copy link
Owner

colinhacks commented Mar 21, 2024

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: #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.

@colinhacks colinhacks closed this Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants