fix: better handling for empty header and query types #221
Conversation
WalkthroughIntroduce Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/types.ts (1)
58-63: MaybeEmptyObject: coverany/neverand document intentEdge cases:
- TObj = any → property becomes required; likely unintended.
- TObj = never → required key of type never.
Add
IsAny/IsNeverguards and a brief doc to avoid footguns.type IsMatchingEmptyObject<T> = [T] extends [{}] ? [{}] extends [T] ? true : false : false +/** + * MaybeEmptyObject + * - undefined | unknown | {} (or unions containing {}) => optional key + * - unions including null | undefined => optional key + * - otherwise => required key + */ export type MaybeEmptyObject< TObj, TKey extends PropertyKey, TFallback = Record<string, unknown> > = IsUndefined<TObj> extends true ? { [K in TKey]?: TFallback } + : IsAny<TObj> extends true + ? { [K in TKey]?: TFallback } : IsExactlyUnknown<TObj> extends true ? { [K in TKey]?: TFallback } : IsMatchingEmptyObject<TObj> extends true ? { [K in TKey]?: TObj } + : IsNever<TObj> extends true + ? { [K in TKey]?: TFallback } : undefined extends TObj ? { [K in TKey]?: TObj } : null extends TObj ? { [K in TKey]?: TObj } : { [K in TKey]: TObj }Also applies to: 64-79
src/treaty2/types.ts (1)
70-73: WSsubscribeoptions should mirror Param-required semantics
options?: Paramis always optional. For consistency with HTTP methods, consider making it required when Param has required keys.- ? MaybeEmptyObject<Route['subscribe']['headers'], 'headers'> & - MaybeEmptyObject<Route['subscribe']['query'], 'query'> extends infer Param - ? (options?: Param) => EdenWS<Route['subscribe']> + ? MaybeEmptyObject<Route['subscribe']['headers'], 'headers'> & + MaybeEmptyObject<Route['subscribe']['query'], 'query'> extends infer Param + ? ({} extends Param + ? (options?: Param) + : (options: Param)) => EdenWS<Route['subscribe']> : nevertest/treaty2.test.ts (1)
395-409: Usetest.skip.eachinstead of commenting out casesKeeps skipped cases visible in output and easy to re-enable.
-test.each([ - 'with-empty-obj', - 'with-partial', - 'with-unknown', - 'with-empty-record', - 'with-union-empty-obj', - 'with-union-empty-record', - // 'with-maybe-empty', - // 'with-optional', - // 'with-union-undefined', -] as const)('type test for case: %s', async (caseName) => { +const supportedCases = [ + 'with-empty-obj', + 'with-partial', + 'with-unknown', + 'with-empty-record', + 'with-union-empty-obj', + 'with-union-empty-record', +] as const +const skippedCases = [ + 'with-maybe-empty', + 'with-optional', + 'with-union-undefined', +] as const + +test.each(supportedCases)('type test for case: %s', async (caseName) => { const { data, error } = await client['empty-test'][caseName].get() expect(error, JSON.stringify(error, null, 2)).toBeNull() expect(data).toEqual({ query: {}, headers: {} }) }) + +test.skip.each(skippedCases)('type test (pending core fix): %s', async (caseName) => { + const { data, error } = await client['empty-test'][caseName].get() + expect(error, JSON.stringify(error, null, 2)).toBeNull() + expect(data).toEqual({ query: {}, headers: {} }) +})test/types/treaty2.ts (1)
1175-1224: Type assertions look correct; add union-with-undefined type-only checkYou’ve validated maybe-empty/unknown/partial. Consider adding a type-only assertion for union-with-undefined (even if runtime test is skipped) to lock inference.
} // Handle partial and optional query and headers { @@ expectTypeOf<NonNullable<Headers>>().toEqualTypeOf<{ username?: string }>() } + +// Handle union with undefined (type-only) +{ + const app = new Elysia().get('/u', () => 'ok', { + query: t.Union([t.Object({ alias: t.String() }), t.Undefined()]), + headers: t.Union([t.Object({ username: t.String() }), t.Undefined()]), + }) + const api = treaty(app) + type Route = typeof api['u']['get'] + type RouteOptions = Parameters<Route>[0] + type Query = NonNullable<RouteOptions>['query'] + type Headers = NonNullable<RouteOptions>['headers'] + expectTypeOf<RouteOptions>().toBeNullable() + expectTypeOf<Query>().toBeNullable() + expectTypeOf<Headers>().toBeNullable() + expectTypeOf<NonNullable<Query>>().toEqualTypeOf<{ alias: string }>() + expectTypeOf<NonNullable<Headers>>().toEqualTypeOf<{ username: string }>() +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/treaty2/types.ts(3 hunks)src/types.ts(1 hunks)test/treaty2.test.ts(3 hunks)test/types/treaty2.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/treaty2/types.ts (1)
src/types.ts (4)
IsNever(42-42)Not(104-104)MaybeEmptyObject(64-78)Prettify(88-90)
🔇 Additional comments (5)
src/treaty2/types.ts (3)
81-134: Nice: Param optionality now derives from required keysUsing
{}extends Param to toggle optionality is clean and fixes past ergonomics. LGTM.
138-156: CreateParams: clarity and correctnessGood handling of optional path params; avoiding Prettify at the callable union preserves call signatures. LGTM.
21-41: Exact-void check improvement: verified, but manual testing recommendedThe file state matches the review comment. The distinction between
void extends B(contravariance/supertypability check) and[B] extends [void](tuple distribution/exact check) is semantically meaningful:
void extends Breturns true when B is a supertype likeunknownorany, causing incorrect classification[B] extends [void]correctly distinguishes exactvoidfromunknownoranyThe proposed change correctly addresses the issue. However, the sandbox environment lacks a TypeScript compiler to validate the three test cases directly. The fix is sound in principle, but manual verification of the mapped types—particularly the first test case with
Generator<never, unknown, any>—is recommended to confirm the behavioral difference.test/treaty2.test.ts (1)
95-132: Coverage for empty/unknown/partial cases looks goodRoute matrix is comprehensive and traces real-world schemas. LGTM.
test/types/treaty2.ts (1)
134-145: New routes align with objectivesRoutes for maybe-empty, unknown-or-obj, and partial-or-optional model the target scenarios well. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/treaty2/types.ts (1)
70-82: Consider adding inline comments to explain the MaybeEmptyObject pattern.The type logic is sophisticated and correctly handles multiple edge cases (undefined, empty objects, optional properties, etc.). Adding brief comments explaining how
MaybeEmptyObjectwraps headers and query parameters, and how this preserves type hints while maintaining proper optionality, would help future maintainers quickly understand the implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/treaty2/types.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/treaty2/types.ts (2)
src/types.ts (1)
MaybeEmptyObject(64-78)src/treaty2/ws.ts (1)
EdenWS(5-91)
🔇 Additional comments (3)
src/treaty2/types.ts (3)
5-5: LGTM! Import correctly adds MaybeEmptyObject utility.The import is necessary for the improved type handling of headers and query parameters introduced below.
70-72: Excellent fix for preserving optional parameter types in subscribe routes.The
MaybeEmptyObjectapproach correctly addresses issue #217 by preserving optional field hints for headers and query parameters rather than collapsing them toRecord<string, unknown> | undefined. The intersection of both wrapped types ensures proper optionality of theoptionsparameter.
75-82: Correct implementation of MaybeEmptyObject for regular routes.The type extraction and parameter shaping using
MaybeEmptyObjectis consistent with the subscribe route handling and correctly preserves optional field types. The resultingParamtype properly integrates with the{} extends Paramcheck on line 83, ensuring theoptionsparameter's optionality matches the actual requirements.
1 participant
I reported similar issue here and attempted to fix this in core here previously, but for now, it's safer to fix Eden first, as that will make issues in the core more apparent.
Added tests for various cases. Note that I disabled these three cases:
with-maybe-empty, with-optional, with-union-undefined. These cause runtime failures even if the types are what you would expect. They may need to be fixed in the core, or the schema that can be assigned to query and headers should be restricted.For example, it can be made not possible to assign
t.Undefined(), but it's possible to assignt.Object({})ort.Partial(...). This is a bit more nuanced, because Eden may addcontent-typeheader even if you don't expect any headers, and that causes schema validation to fail. So it's not a good idea to define headers to be a strict schema or undefined. I am open to suggestions.I believe this also fixes #217
Summary by CodeRabbit
New Features
Tests