-
-
Notifications
You must be signed in to change notification settings - Fork 12
fix: Correctly infer optional enum arrays #243
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: Correctly infer optional enum arrays #243
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request fixes the WrapType TypeScript utility to correctly handle optional enum arrays. The change ensures that when both Optional and Array flags are true, the type properly returns T[] | null instead of the previous incorrect behavior.
- Restructured the conditional logic in
WrapTypeto prioritize array handling before optional handling - Fixed the type inference for optional arrays to correctly produce
T[] | null
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/runtime/src/utils/type-utils.ts (2)
19-25: Prefer composition with NullableIf; constrain and rename type params.Cuts duplication, tightens the contract to booleans, and avoids shadowing exported
Optionaland globalArraynames.Apply:
- export type WrapType<T, Optional = false, Array = false> = Array extends true - ? Optional extends true - ? T[] | null - : T[] - : Optional extends true - ? T | null - : T; +/** Wraps T into an array and/or makes it nullable. + * - IsArray true => T[] + * - IsOptional true => ... | null + * Combine both => T[] | null + * Note: property optionality (| undefined) is handled separately via `?` or `OrUndefinedIf`. + */ +export type WrapType< + T, + IsOptional extends boolean = false, + IsArray extends boolean = false +> = NullableIf<IsArray extends true ? T[] : T, IsOptional>;
19-25: Add a minimal type-level test to lock this behavior.Prevents regressions for enum arrays and optional arrays.
Example (place under tests/types, compiled-only):
// tests/types/wraptype.test-d.ts import type { WrapType } from '../../packages/runtime/src/utils/type-utils'; type Equal<A, B> = (<T>() => T extends A ? 1 : 2) extends (<T>() => T extends B ? 1 : 2) ? true : false; type Assert<T extends true> = T; // Array + Optional => T[] | null type A1 = WrapType<'X', true, true>; type E1 = 'X'[] | null; type _t1 = Assert<Equal<A1, E1>>; // Array only => T[] type A2 = WrapType<'X', false, true>; type E2 = 'X'[]; type _t2 = Assert<Equal<A2, E2>>; // Optional only => T | null type A3 = WrapType<'X', true, false>; type E3 = 'X' | null; type _t3 = Assert<Equal<A3, E3>>;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime/src/utils/type-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/src/utils/type-utils.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test (20.x)
🔇 Additional comments (1)
packages/runtime/src/utils/type-utils.ts (1)
19-25: Verify codegen Optional+Array behavior and impact
- Ensure optional enum arrays still generate
| undefined(e.g.ShirtColor[] | null | undefined).- Confirm no existing call sites rely on the old
T | nullcollapse for arrays.
ymc9
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for making the fix. I'll add a test for it.
Fixes #240
@ymc9 I believe this is just a type change. Should there be a test associated with this?
Summary by CodeRabbit
Bug Fixes
Refactor