Repository navigation
perf(types): use key remapping in MarkOptional - #1675
fabian-hiller wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
commit: |
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="library/src/types/utils.ts">
<violation number="1" location="library/src/types/utils.ts:64">
P3: This reimplementation intentionally changes the displayed key order of `Prettify`-wrapped output types (required keys now precede optional ones, e.g. `{ id: number; name: string; email?: string }`), but neither the code nor the CHANGELOG entry records the trade-off, and the comment explaining key-order preservation was deleted. Because `toEqualTypeOf` ignores key order, the test suite cannot catch this user-visible change to hover previews and docs. Add a comment above `MarkOptional` documenting the required-before-optional ordering, and if the order change is accepted, reflect it in the CHANGELOG entry.</violation>
<violation number="2" location="library/src/types/utils.ts:66">
P2: `MarkOptional` drops explicitly declared keys hidden by an index signature. Remap the optional half over `TKeys` directly so `MarkOptional<{ [key: string]: unknown; type: 'foo' }, 'type'>` still enforces `type?: 'foo'`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| export type MarkOptional<TObject, TKeys extends keyof TObject> = { | ||
| [TKey in keyof TObject as TKey extends TKeys ? never : TKey]: TObject[TKey]; | ||
| } & { | ||
| [TKey in keyof TObject as TKey extends TKeys ? TKey : never]?: TObject[TKey]; |
There was a problem hiding this comment.
P2: MarkOptional drops explicitly declared keys hidden by an index signature. Remap the optional half over TKeys directly so MarkOptional<{ [key: string]: unknown; type: 'foo' }, 'type'> still enforces type?: 'foo'.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At library/src/types/utils.ts, line 66:
<comment>`MarkOptional` drops explicitly declared keys hidden by an index signature. Remap the optional half over `TKeys` directly so `MarkOptional<{ [key: string]: unknown; type: 'foo' }, 'type'>` still enforces `type?: 'foo'`.</comment>
<file context>
@@ -60,10 +60,11 @@ export type Prettify<TObject> = { [TKey in keyof TObject]: TObject[TKey] } & {};
+export type MarkOptional<TObject, TKeys extends keyof TObject> = {
+ [TKey in keyof TObject as TKey extends TKeys ? never : TKey]: TObject[TKey];
+} & {
+ [TKey in keyof TObject as TKey extends TKeys ? TKey : never]?: TObject[TKey];
+};
</file context>
| [TKey in keyof TObject as TKey extends TKeys ? TKey : never]?: TObject[TKey]; | |
| [TKey in TKeys]?: TObject[TKey]; |
| { [TKey in keyof TObject]?: unknown } & Omit<TObject, TKeys> & | ||
| Partial<Pick<TObject, TKeys>>; | ||
| export type MarkOptional<TObject, TKeys extends keyof TObject> = { | ||
| [TKey in keyof TObject as TKey extends TKeys ? never : TKey]: TObject[TKey]; |
There was a problem hiding this comment.
P3: This reimplementation intentionally changes the displayed key order of Prettify-wrapped output types (required keys now precede optional ones, e.g. { id: number; name: string; email?: string }), but neither the code nor the CHANGELOG entry records the trade-off, and the comment explaining key-order preservation was deleted. Because toEqualTypeOf ignores key order, the test suite cannot catch this user-visible change to hover previews and docs. Add a comment above MarkOptional documenting the required-before-optional ordering, and if the order change is accepted, reflect it in the CHANGELOG entry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At library/src/types/utils.ts, line 64:
<comment>This reimplementation intentionally changes the displayed key order of `Prettify`-wrapped output types (required keys now precede optional ones, e.g. `{ id: number; name: string; email?: string }`), but neither the code nor the CHANGELOG entry records the trade-off, and the comment explaining key-order preservation was deleted. Because `toEqualTypeOf` ignores key order, the test suite cannot catch this user-visible change to hover previews and docs. Add a comment above `MarkOptional` documenting the required-before-optional ordering, and if the order change is accepted, reflect it in the CHANGELOG entry.</comment>
<file context>
@@ -60,10 +60,11 @@ export type Prettify<TObject> = { [TKey in keyof TObject]: TObject[TKey] } & {};
- { [TKey in keyof TObject]?: unknown } & Omit<TObject, TKeys> &
- Partial<Pick<TObject, TKeys>>;
+export type MarkOptional<TObject, TKeys extends keyof TObject> = {
+ [TKey in keyof TObject as TKey extends TKeys ? never : TKey]: TObject[TKey];
+} & {
+ [TKey in keyof TObject as TKey extends TKeys ? TKey : never]?: TObject[TKey];
</file context>
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="library/CHANGELOG.md">
<violation number="1" location="library/CHANGELOG.md:7">
P3: This PR changes how inferred object types are displayed for every schema with optional entries: required keys are now shown before optional keys (e.g. `{ id: number; name: string; email?: string }` vs the original declaration order), because `Prettify` resolves the two key-remapped members in intersection order and the old `{ [TKey in keyof TObject]?: unknown }` order-preserving member is gone. Since the old comment documented that member as existing precisely to preserve key order, record this user-visible display change in the CHANGELOG entry (or keep the order-preserving member) so the decision called for in the PR is explicit.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| ## vX.X.X (Month DD, YYYY) | ||
|
|
||
| - Change `MarkOptional` type to use key remapping instead of `Omit`, `Pick` and `Partial` to improve TypeScript performance (pull request #1675) |
There was a problem hiding this comment.
P3: This PR changes how inferred object types are displayed for every schema with optional entries: required keys are now shown before optional keys (e.g. { id: number; name: string; email?: string } vs the original declaration order), because Prettify resolves the two key-remapped members in intersection order and the old { [TKey in keyof TObject]?: unknown } order-preserving member is gone. Since the old comment documented that member as existing precisely to preserve key order, record this user-visible display change in the CHANGELOG entry (or keep the order-preserving member) so the decision called for in the PR is explicit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At library/CHANGELOG.md, line 7:
<comment>This PR changes how inferred object types are displayed for every schema with optional entries: required keys are now shown before optional keys (e.g. `{ id: number; name: string; email?: string }` vs the original declaration order), because `Prettify` resolves the two key-remapped members in intersection order and the old `{ [TKey in keyof TObject]?: unknown }` order-preserving member is gone. Since the old comment documented that member as existing precisely to preserve key order, record this user-visible display change in the CHANGELOG entry (or keep the order-preserving member) so the decision called for in the PR is explicit.</comment>
<file context>
@@ -4,6 +4,7 @@ All notable changes to the library will be documented in this file.
## vX.X.X (Month DD, YYYY)
+- Change `MarkOptional` type to use key remapping instead of `Omit`, `Pick` and `Partial` to improve TypeScript performance (pull request #1675)
- Change internal `VariantOption` and `VariantOptionAsync` types of `variant` schema to use a structural object option type to improve TypeScript performance (pull request #1670)
- Change `isbn`, `creditCard` and `imei` actions to check digits by character code instead of splitting and parsing the input to improve performance (pull request #1634)
</file context>
| - Change `MarkOptional` type to use key remapping instead of `Omit`, `Pick` and `Partial` to improve TypeScript performance (pull request #1675) | |
| - Change `MarkOptional` type to use key remapping instead of `Omit`, `Pick` and `Partial` to improve TypeScript performance. This also changes the order of keys in inferred types - required keys are now shown before optional keys (pull request #1675) |

Part of #1663 (finding 10). Stacked on #1670. On
mainalone, this change breaks thevarianttests for optional discriminators. The currentvariantconstraint relies on howOmittreats the known keys of index-signature types (it makes them optional), and #1670 replaces that constraint.This changes how types are displayed, so it needs a decision.
Change
MarkOptionalnow uses two key-remapped mapped types (the same approach as Zod v4's$InferObjectOutput):It used to be
{ [TKey in keyof TObject]?: unknown } & Omit<TObject, TKeys> & Partial<Pick<TObject, TKeys>>, andOmitin turn buildsPick+Exclude.Measurements
TypeScript 6.0.3, schema-benchmarks probe, compared with #1670 (schema / infer):
partial/pick/omit/requiredBehavior change
Assignability is unchanged: all runtime and type tests pass (
toEqualTypeOfignores key order). The displayed key order changes: required keys are listed before optional ones. For example,{ id: number; email?: string; name: string }is now shown as{ id: number; name: string; email?: string }. The current code comment says the{ [TKey in keyof TObject]?: unknown }member exists to preserve key order, so this is a deliberate trade-off. An order-preserving variant, which keeps that member and only swapsOmit/Pickfor key remapping, gave mixed results in my measurements.🤖 Generated with Claude Code