-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[v4 beta] Properly support branded keys in records #4277
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
base: v4
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
8970427
to
65d2c92
Compare
65d2c92
to
6b40942
Compare
Hi! I agree that your screenshot shows the current behavior. I would define your screenshot as an example of exactly the problem I am trying to solve. I believe the correct type should be
As your screenshot shows, Zod 3 and Zod 4 are both making the record partial, and they weaken the value type into |
This should resolve the longstanding discussion:
I'm running into this problem:
Colin explains the rationale for sometimes returning
Partial<Record<K, V>>
in #992 (comment). But that logic doesn't apply to brands - this is just an oversight.@AlexErrant noticed the same problem, and submitted a PR into Zod v3, which this PR is based on:
recordWithBrandedKeys
should not bePartial
#3918I'm going with a different approach than that PR takes, though.
Today, the code says that the record is not partial if one of these is true:
string extends K
)number extends K
)symbol extends K
)This PR adds three more checks. The record is also not partial if one of these is true:
string & $brand<never> extends K
)number & $brand<never> extends K
)symbol & $brand<never> extends K
)When my change didn't work, I found that the problem was because
$brand<T>
usesT
in an index key signature. This meant thatbrand<never>
resolved to{}
, which is not really correct from a type theory perspective.So... I'm also changing the type implementation of
$brand
in this PR. I think this is an acceptable change to make while Zod v4 is still in beta.Let me know what you think!