feat(sql-orm-client): expand and simplify output types#283
feat(sql-orm-client): expand and simplify output types#283
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
@prisma-next/runtime-executor
@prisma-next/mongo-core
@prisma-next/mongo-orm
@prisma-next/mongo-runtime
@prisma-next/sql-runtime
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract-authoring
@prisma-next/contract-ts
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/emitter
@prisma-next/migration-tools
@prisma-next/vite-plugin-contract-emit
@prisma-next/mongo-emitter
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/family-sql
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/adapter-postgres
@prisma-next/driver-postgres
@prisma-next/core-control-plane
@prisma-next/core-execution-plane
@prisma-next/config
@prisma-next/contract
@prisma-next/operations
@prisma-next/plan
@prisma-next/utils
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/3-extensions/sql-orm-client/test/simplify-deep.test-d.ts (1)
74-75: Use double cast for test mocks.Per coding guidelines, test mocks should use double casts (
as unknown as X) to make the unsafe boundary explicit.♻️ Suggested fix
const runtime = createMockRuntime(); - const context = {} as ExecutionContext<TestContract>; + const context = {} as unknown as ExecutionContext<TestContract>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/simplify-deep.test-d.ts` around lines 74 - 75, The test mock `context` is using a single cast which hides the unsafe conversion; change the declaration where `context` is assigned (`const context = {} as ExecutionContext<TestContract>`) to use a double cast (`as unknown as ExecutionContext<TestContract>`) so the unsafe boundary is explicit; leave `createMockRuntime()` as-is and update only the `context` variable's casting.packages/3-extensions/sql-orm-client/src/types.ts (1)
28-35: Readonly arrays lose theirreadonlymodifier.The array branch returns
SimplifyDeep<Element>[](mutable), which stripsreadonlyfrom input types likereadonly T[]. If any row fields are typed as readonly arrays, the simplified type will become mutable.If preserving readonly is desired:
♻️ Suggested fix to preserve readonly
export type SimplifyDeep<T> = T extends readonly (infer Element)[] - ? SimplifyDeep<Element>[] + ? readonly SimplifyDeep<Element>[] : T extends string | number | boolean | bigint | symbol | Date | Uint8Array ? T : T extends object ? { [K in keyof T]: SimplifyDeep<T[K]> } : T;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/types.ts` around lines 28 - 35, The SimplifyDeep utility is stripping readonly from array types because the array branch returns SimplifyDeep<Element>[] (mutable); update the array branch in SimplifyDeep so it preserves readonly (e.g., when T extends readonly (infer Element)[] return a readonly array of SimplifyDeep<Element>), ensuring readonly T[] inputs remain readonly in the output; modify the SimplifyDeep definition (the branch matching readonly (infer Element)[]) to produce a readonly SimplifyDeep<Element>[] rather than a mutable array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/3-extensions/sql-orm-client/src/types.ts`:
- Around line 28-35: The SimplifyDeep utility is stripping readonly from array
types because the array branch returns SimplifyDeep<Element>[] (mutable); update
the array branch in SimplifyDeep so it preserves readonly (e.g., when T extends
readonly (infer Element)[] return a readonly array of SimplifyDeep<Element>),
ensuring readonly T[] inputs remain readonly in the output; modify the
SimplifyDeep definition (the branch matching readonly (infer Element)[]) to
produce a readonly SimplifyDeep<Element>[] rather than a mutable array.
In `@packages/3-extensions/sql-orm-client/test/simplify-deep.test-d.ts`:
- Around line 74-75: The test mock `context` is using a single cast which hides
the unsafe conversion; change the declaration where `context` is assigned
(`const context = {} as ExecutionContext<TestContract>`) to use a double cast
(`as unknown as ExecutionContext<TestContract>`) so the unsafe boundary is
explicit; leave `createMockRuntime()` as-is and update only the `context`
variable's casting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5f49d154-0964-462d-a77d-f8515e353d75
📒 Files selected for processing (4)
packages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/grouped-collection.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/test/simplify-deep.test-d.ts
Closes: https://linear.app/prisma-company/issue/TML-2139/prettify-query-result-types-with-recursive-mapped-type Query result types in `sql-orm-client` are currently unexpanded intersections full of internal utility types, making them hard to read in IDE tooltips and error messages. This change applies a recursive mapped type at the top-level return positions so that IDE tooltips and hover types display fully-evaluated object literals.
aqrln
force-pushed
the
tml-2139-prettify-query-result-types-with-recursive-mapped-type
branch
from
131787e to
c25c1dd
Compare
wmadden
left a comment
wmadden
left a comment
There was a problem hiding this comment.
Preemptively approving — the approach is sound and the implementation is clean. Please address or respond to the inline comments before merging; no re-review needed.
Blocking (2): F01 (double-wrapping at each chaining step — apply at terminal methods instead), F02 (add test for deeply chained operations).
Non-blocking (3): F03 (readonly array erasure), F04 (raise to @prisma-next/utils), F05 (polymorphic model forward risk).
| export class Collection< | ||
| TContract extends SqlContract<SqlStorage>, | ||
| ModelName extends string, | ||
| Row = DefaultModelRow<TContract, ModelName>, | ||
| Row = SimplifyDeep<DefaultModelRow<TContract, ModelName>>, | ||
| State extends CollectionTypeState = DefaultCollectionTypeState, |
There was a problem hiding this comment.
F01 — Blocking: SimplifyDeep applied at every chaining step instead of terminal methods
The default Row is already SimplifyDeep<DefaultModelRow<...>>. When include() returns SimplifyDeep<Row & { posts: ... }>, this becomes SimplifyDeep<SimplifyDeep<...> & { ... }>. Each chaining step adds another wrapper. For users.select('name').include('posts'), that's three nested SimplifyDeep evaluations — O(N×K) work for N chained ops with K total keys.
Suggestion: Apply SimplifyDeep only at the terminal methods (first(), all(), toArray()) instead of at each builder step. Builder methods accumulate raw intersections internally; only the final result type gets simplified. This reduces overhead to exactly one pass regardless of chain length and is architecturally cleaner — simplification is a presentation concern, not a builder concern.
| describe('Collection result types are simplified', () => { | ||
| const runtime = createMockRuntime(); | ||
| const context = getTestContext(); | ||
|
|
||
| test('default Row is a plain object', () => { | ||
| const users = new Collection({ runtime, context }, 'User'); | ||
| type UserRow = Awaited<ReturnType<typeof users.first>>; | ||
| expectTypeOf<NonNullable<UserRow>>().toEqualTypeOf<{ | ||
| id: number; | ||
| name: string; | ||
| email: string; | ||
| invitedById: number | null; | ||
| }>(); | ||
| }); | ||
|
|
||
| test('select() produces a plain object', () => { | ||
| const users = new Collection({ runtime, context }, 'User'); | ||
| const selected = users.select('id', 'email'); | ||
| type SelectedRow = Awaited<ReturnType<typeof selected.first>>; | ||
| expectTypeOf<NonNullable<SelectedRow>>().toEqualTypeOf<{ | ||
| id: number; | ||
| email: string; | ||
| }>(); | ||
| }); | ||
|
|
||
| test('include() produces a plain object with nested relation', () => { | ||
| const users = new Collection({ runtime, context }, 'User'); | ||
| const withPosts = users.include('posts'); | ||
| type WithPostsRow = Awaited<ReturnType<typeof withPosts.first>>; | ||
| expectTypeOf<NonNullable<WithPostsRow>>().toEqualTypeOf<{ | ||
| id: number; | ||
| name: string; | ||
| email: string; | ||
| invitedById: number | null; | ||
| posts: { | ||
| id: number; | ||
| title: string; | ||
| userId: number; | ||
| views: number; | ||
| }[]; | ||
| }>(); | ||
| }); | ||
|
|
||
| test('select().include() produces a plain object', () => { | ||
| const users = new Collection({ runtime, context }, 'User'); | ||
| const selected = users.select('name').include('posts'); | ||
| type Row = Awaited<ReturnType<typeof selected.first>>; | ||
| expectTypeOf<NonNullable<Row>>().toEqualTypeOf<{ | ||
| name: string; | ||
| posts: { | ||
| id: number; | ||
| title: string; | ||
| userId: number; | ||
| views: number; | ||
| }[]; | ||
| }>(); | ||
| }); | ||
|
|
||
| test('include() with non-nullable to-one relation', () => { | ||
| const posts = new Collection({ runtime, context }, 'Post'); | ||
| const withAuthor = posts.include('author'); | ||
| type Row = Awaited<ReturnType<typeof withAuthor.first>>; | ||
| type AuthorField = NonNullable<Row>['author']; | ||
| expectTypeOf<AuthorField>().toEqualTypeOf<{ | ||
| id: number; | ||
| name: string; | ||
| email: string; | ||
| invitedById: number | null; | ||
| }>(); | ||
| }); | ||
|
|
||
| test('include() with count refinement', () => { | ||
| const users = new Collection({ runtime, context }, 'User'); | ||
| const withPostCount = users.include('posts', (posts) => posts.count()); | ||
| type Row = Awaited<ReturnType<typeof withPostCount.first>>; | ||
| expectTypeOf<NonNullable<Row>['posts']>().toEqualTypeOf<number>(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
F02 — Blocking: No test coverage for multi-step chaining (the primary use case)
Deeply nested chains are the primary use case that motivated this change — they produce the most unreadable intersections in IDE tooltips. The tests here verify individual operations and one two-method chain (select().include()), but don't test deeper chains like include('posts').include('invitedBy').
Since each chaining step currently wraps the previous Row in a new SimplifyDeep<Row & ...>, these deeper chains exercise the idempotency property that unit-level tests don't cover.
Suggestion: Add at least one multi-include chain test:
test('chained include() produces a plain object', () => {
const users = new Collection({ runtime, context }, 'User');
const withPostsAndInviter = users.include('posts').include('invitedBy');
type Row = Awaited<ReturnType<typeof withPostsAndInviter.first>>;
expectTypeOf<NonNullable<Row>>().toEqualTypeOf<{
id: number;
name: string;
email: string;
invitedById: number | null;
posts: { id: number; title: string; userId: number; views: number }[];
invitedBy: { id: number; name: string; email: string; invitedById: number | null } | null;
}>();
});| export type SimplifyDeep<T> = T extends readonly (infer Element)[] | ||
| ? SimplifyDeep<Element>[] |
There was a problem hiding this comment.
F03 — Non-blocking: readonly arrays are silently converted to mutable arrays
The array branch matches readonly (infer Element)[] but returns SimplifyDeep<Element>[] (mutable). A readonly T[] input becomes T[]. Acceptable for current query result types (which are always mutable arrays), but subtly changes the type contract — relevant if SimplifyDeep is applied more broadly (see F04).
Suggestion: Either preserve readonly with an additional branch, or add a test documenting this intentional behavior:
test('readonly arrays become mutable (intentional)', () => {
type Input = readonly ({ a: number } & { b: string })[];
type Result = SimplifyDeep<Input>;
expectTypeOf<Result>().toEqualTypeOf<{ a: number; b: string }[]>();
});| // --------------------------------------------------------------------------- | ||
| // SimplifyDeep — recursive type prettifier for IDE tooltips | ||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
F04 — Non-blocking: SimplifyDeep should be raised to @prisma-next/utils for cross-lane reuse
SimplifyDeep is a general-purpose type utility with no dependency on sql-orm-client internals. The @prisma-next/utils package (packages/1-framework/1-core/shared/utils) is the canonical home for shared utilities. Moving it there enables reuse by the Mongo family and other lanes.
Not blocking this PR, but should be done as a follow-up before other packages duplicate the type.
| export type SimplifyDeep<T> = T extends readonly (infer Element)[] | ||
| ? SimplifyDeep<Element>[] | ||
| : T extends string | number | boolean | bigint | symbol | Date | Uint8Array | ||
| ? T | ||
| : T extends object | ||
| ? { [K in keyof T]: SimplifyDeep<T[K]> } | ||
| : T; |
There was a problem hiding this comment.
F05 — Non-blocking: Forward risk — polymorphic model compatibility
Polymorphic model support (ADR 173 — discriminator + variants) will be ported to the SQL domain and will impact this ORM client. Experience implementing a similar helper in the Mongo domain surfaced issues with polymorphic model type shapes.
SimplifyDeep distributes over simple unions correctly (SimplifyDeep<A | B> = SimplifyDeep<A> | SimplifyDeep<B>), but more complex polymorphic type algebra (variant-specific computed properties, conditional types) may not be preserved correctly by the { [K in keyof T]: ... } mapping.
Not a blocker — just flagging that when polymorphic models land in SQL ORM, this helper will need verification and may need extension or removal.
2 participants
Closes: https://linear.app/prisma-company/issue/TML-2139/prettify-query-result-types-with-recursive-mapped-type
Query result types in
sql-orm-clientare currently unexpanded intersections full of internal utility types, making them hard to read in IDE tooltips and error messages.This change applies a recursive mapped type at the top-level return positions so that IDE tooltips and hover types display fully-evaluated object literals.
Before:
After:
Summary by CodeRabbit
Type System Improvements
Style
Tests