-
Notifications
You must be signed in to change notification settings - Fork 257
fix: support nested optional property access in SingleRowRefProxy #1747
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@tanstack/db": patch | ||
| --- | ||
|
|
||
| Fix `SingleRowRefProxy` collapsing optional and nullable nested objects to opaque leaves. `createIndex()` and single-row `where` callbacks can now traverse them with optional chaining (`row.updatedAt?.seconds`), matching the query builder's `Ref` behavior; runtime behavior is unchanged. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| import { describe, expectTypeOf, it } from 'vitest' | ||
| import { createCollection } from '../src/collection/index.js' | ||
| import { eq } from '../src/query/builder/functions.js' | ||
| import type { RefLeaf } from '../src/query/builder/types.js' | ||
|
|
||
| describe(`SingleRowRefProxy nested optional property access`, () => { | ||
| type Doc = { | ||
| id: string | ||
| name: string | ||
| updatedAt?: { seconds: number; nanoseconds: number } | ||
| author: { name: string; contact?: { email: string } } | ||
| deletedAt: { seconds: number } | null | ||
| } | ||
|
|
||
| const collection = createCollection<Doc, string>({ | ||
| getKey: (doc) => doc.id, | ||
| sync: { sync: () => {} }, | ||
| }) | ||
|
|
||
| it(`allows optional chaining into an optional nested object`, () => { | ||
| collection.createIndex((row) => { | ||
| expectTypeOf(row.updatedAt?.seconds).toEqualTypeOf< | ||
| RefLeaf<number> | undefined | ||
| >() | ||
| return row.updatedAt?.seconds | ||
| }) | ||
| }) | ||
|
|
||
| it(`requires optional chaining for an optional nested object`, () => { | ||
| collection.createIndex((row) => { | ||
| // @ts-expect-error - updatedAt may be undefined, plain access must error | ||
| return row.updatedAt.seconds | ||
| }) | ||
| }) | ||
|
|
||
| it(`allows optional chaining into a nullable nested object`, () => { | ||
| collection.createIndex((row) => { | ||
| expectTypeOf(row.deletedAt?.seconds).toEqualTypeOf< | ||
| RefLeaf<number> | undefined | ||
| >() | ||
|
Comment on lines
+38
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Assert null preservation on the field itself. The assertion on 🤖 Prompt for AI Agents |
||
| return row.deletedAt?.seconds | ||
| }) | ||
| }) | ||
|
|
||
| it(`keeps required nested objects traversable without optional chaining`, () => { | ||
| collection.createIndex((row) => { | ||
| expectTypeOf(row.author.name).toEqualTypeOf<RefLeaf<string>>() | ||
| return row.author.name | ||
| }) | ||
| }) | ||
|
|
||
| it(`supports optional objects nested below a required object`, () => { | ||
| collection.createIndex((row) => { | ||
| expectTypeOf(row.author.contact?.email).toEqualTypeOf< | ||
| RefLeaf<string> | undefined | ||
| >() | ||
| return row.author.contact?.email | ||
| }) | ||
| }) | ||
|
|
||
| it(`keeps scalar fields as plain leaves`, () => { | ||
| collection.createIndex((row) => { | ||
| expectTypeOf(row.name).toEqualTypeOf<RefLeaf<string>>() | ||
| return row.name | ||
| }) | ||
| }) | ||
|
|
||
| it(`rejects properties that do not exist on the nested object`, () => { | ||
| collection.createIndex((row) => { | ||
| // @ts-expect-error - millis is not a property of updatedAt | ||
| return row.updatedAt?.millis | ||
| }) | ||
| }) | ||
|
|
||
| it(`accepts nested optional refs in the subscribeChanges where callback`, () => { | ||
| collection.subscribeChanges(() => {}, { | ||
| where: (row) => eq(row.updatedAt?.seconds, 5), | ||
| }) | ||
| }) | ||
| }) | ||
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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: TanStack/db
Length of output: 42125
🏁 Script executed:
Repository: TanStack/db
Length of output: 34743
🏁 Script executed:
Repository: TanStack/db
Length of output: 5136
🏁 Script executed:
Repository: TanStack/db
Length of output: 149
Replace
Record<string, any>with an object constraint inSingleRowField.Record<string, unknown>is not a safe replacement because named interface types do not extend it. UseNonNullable<V> extends objector an equivalent non-anystructural check to preserve nested interface property access.🤖 Prompt for AI Agents
Source: Coding guidelines