-
Notifications
You must be signed in to change notification settings - Fork 243
fix(db): preserve discriminated union types through .select() (#1511) #1597
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 `.select()` collapsing discriminated-union fields to the intersection of common keys (#1511). `Ref<T>` now distributes over `T` so `keyof (A | B | C)` no longer reduces the union to its common keys, and `ExtractRef<T>` now distinguishes a real branded `Ref` (where the underlying user type `U` can be returned directly) from a spread-produced inline object (which still needs to be projected through `ResultTypeFromSelect`). This preserves discriminated unions both when the field is selected at the top level and when the field is nested inside another selected object. The real-`Ref` detection uses a strict structural equivalence against the canonical `Ref<U>` shape, so spread-derived objects that keep the same keys but change a field's type (e.g. `{ ...u, code: u.slug }`) or drop an optional key (e.g. `const { nickname, ...rest } = u`) are projected through `ResultTypeFromSelect` instead of being collapsed back to `U`. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,6 @@ | ||||||||||||||||||||||||||||
| import { describe, expectTypeOf, test } from 'vitest' | ||||||||||||||||||||||||||||
| import { createCollection } from '../../src/collection/index.js' | ||||||||||||||||||||||||||||
| import { createLiveQueryCollection } from '../../src/query/index.js' | ||||||||||||||||||||||||||||
| import { createLiveQueryCollection, eq } from '../../src/query/index.js' | ||||||||||||||||||||||||||||
| import { mockSyncCollectionOptions } from '../utils.js' | ||||||||||||||||||||||||||||
| import { upper } from '../../src/query/builder/functions.js' | ||||||||||||||||||||||||||||
| import type { OutputWithVirtual } from '../utils.js' | ||||||||||||||||||||||||||||
|
|
@@ -109,6 +109,126 @@ describe(`select types`, () => { | |||||||||||||||||||||||||||
| expectTypeOf(results).toMatchTypeOf<OutputWithVirtualKeyed<Expected>>() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| test(`select preserves union types and where works on common keys`, () => { | ||||||||||||||||||||||||||||
| type ItemDocument = | ||||||||||||||||||||||||||||
| | { type: 'pdf'; url: string; pages: number } | ||||||||||||||||||||||||||||
| | { type: 'image'; url: string; width: number; height: number } | ||||||||||||||||||||||||||||
| | { type: 'legacy'; path: string } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| type Item = { id: number; name: string; document: ItemDocument } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const items = createCollection( | ||||||||||||||||||||||||||||
| mockSyncCollectionOptions<Item>({ | ||||||||||||||||||||||||||||
| id: `union-field-items`, | ||||||||||||||||||||||||||||
| getKey: (i) => i.id, | ||||||||||||||||||||||||||||
| initialData: [], | ||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Filtering by a common key of the union should compile, | ||||||||||||||||||||||||||||
| // and the result should preserve the full discriminated union | ||||||||||||||||||||||||||||
| const col = createLiveQueryCollection((q) => | ||||||||||||||||||||||||||||
| q | ||||||||||||||||||||||||||||
| .from({ i: items }) | ||||||||||||||||||||||||||||
| .where(({ i }) => eq(i.document.type, `pdf`)) | ||||||||||||||||||||||||||||
| .select(({ i }) => ({ | ||||||||||||||||||||||||||||
| id: i.id, | ||||||||||||||||||||||||||||
| document: i.document, | ||||||||||||||||||||||||||||
| })), | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const result = col.toArray[0]! | ||||||||||||||||||||||||||||
| expectTypeOf(result.document).toEqualTypeOf<ItemDocument>() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| test(`select preserves union when nested under another field`, () => { | ||||||||||||||||||||||||||||
| type Payload = | ||||||||||||||||||||||||||||
| | { kind: 'text'; body: string } | ||||||||||||||||||||||||||||
| | { kind: 'binary'; bytes: number; mime: string } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| type Envelope = { id: number; payload: { inner: Payload } } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const envelopes = createCollection( | ||||||||||||||||||||||||||||
| mockSyncCollectionOptions<Envelope>({ | ||||||||||||||||||||||||||||
| id: `nested-union-envelopes`, | ||||||||||||||||||||||||||||
| getKey: (e) => e.id, | ||||||||||||||||||||||||||||
| initialData: [], | ||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Selecting a nested object whose field is a discriminated union | ||||||||||||||||||||||||||||
| // must preserve the union (not collapse to the intersection of keys). | ||||||||||||||||||||||||||||
| const col = createLiveQueryCollection((q) => | ||||||||||||||||||||||||||||
| q.from({ e: envelopes }).select(({ e }) => ({ | ||||||||||||||||||||||||||||
| id: e.id, | ||||||||||||||||||||||||||||
| payload: e.payload, | ||||||||||||||||||||||||||||
| })), | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| const r = col.toArray[0]! | ||||||||||||||||||||||||||||
| expectTypeOf(r.payload).toEqualTypeOf<{ inner: Payload }>() | ||||||||||||||||||||||||||||
| expectTypeOf(r.payload.inner).toEqualTypeOf<Payload>() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| test(`spread with a same-key narrower override projects the override type`, () => { | ||||||||||||||||||||||||||||
| type SpreadUser = { | ||||||||||||||||||||||||||||
| id: number | ||||||||||||||||||||||||||||
| code: string | number | ||||||||||||||||||||||||||||
| slug: string | ||||||||||||||||||||||||||||
| nickname?: string | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const spreadUsers = createCollection( | ||||||||||||||||||||||||||||
| mockSyncCollectionOptions<SpreadUser>({ | ||||||||||||||||||||||||||||
| id: `spread-override-users`, | ||||||||||||||||||||||||||||
| getKey: (u) => u.id, | ||||||||||||||||||||||||||||
| initialData: [], | ||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const col = createLiveQueryCollection((q) => | ||||||||||||||||||||||||||||
| q.from({ u: spreadUsers }).select(({ u }) => ({ | ||||||||||||||||||||||||||||
| narrowed: { ...u, code: u.slug }, | ||||||||||||||||||||||||||||
| })), | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const result = col.toArray[0]! | ||||||||||||||||||||||||||||
| // `code` was overridden with `u.slug` (string), so the projected | ||||||||||||||||||||||||||||
| // field must be `string`, not the original `string | number`. | ||||||||||||||||||||||||||||
| expectTypeOf(result.narrowed.code).toEqualTypeOf<string>() | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| test(`spread that omits an optional property drops the key`, () => { | ||||||||||||||||||||||||||||
| type SpreadUser = { | ||||||||||||||||||||||||||||
| id: number | ||||||||||||||||||||||||||||
| code: string | number | ||||||||||||||||||||||||||||
| slug: string | ||||||||||||||||||||||||||||
| nickname?: string | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const spreadUsers = createCollection( | ||||||||||||||||||||||||||||
| mockSyncCollectionOptions<SpreadUser>({ | ||||||||||||||||||||||||||||
| id: `spread-omit-users`, | ||||||||||||||||||||||||||||
| getKey: (u) => u.id, | ||||||||||||||||||||||||||||
| initialData: [], | ||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const col = createLiveQueryCollection((q) => | ||||||||||||||||||||||||||||
| q.from({ u: spreadUsers }).select(({ u }) => { | ||||||||||||||||||||||||||||
| const { nickname, ...withoutNickname } = u | ||||||||||||||||||||||||||||
| return { trimmed: withoutNickname } | ||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const result = col.toArray[0]! | ||||||||||||||||||||||||||||
| // `nickname` was destructured out, so the projected object must | ||||||||||||||||||||||||||||
| // not reintroduce the key. | ||||||||||||||||||||||||||||
| type HasNickname = `nickname` extends keyof typeof result.trimmed | ||||||||||||||||||||||||||||
| ? true | ||||||||||||||||||||||||||||
| : false | ||||||||||||||||||||||||||||
| expectTypeOf<HasNickname>().toEqualTypeOf<false>() | ||||||||||||||||||||||||||||
|
Comment on lines
+223
to
+229
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. 📐 Maintainability & Code Quality | 🟡 Minor Remove unused runtime binding to fix type-only lint warning The variable 🛠️ Proposed fix- const result = col.toArray[0]!
- // `nickname` was destructured out, so the projected object must
- // not reintroduce the key.
- type HasNickname = `nickname` extends keyof typeof result.trimmed
+ // `nickname` was destructured out, so the projected object must
+ // not reintroduce the key.
+ type HasNickname = `nickname` extends keyof (typeof col.toArray)[number][`trimmed`]
? true
: false
expectTypeOf<HasNickname>().toEqualTypeOf<false>()The expression 📝 Committable suggestion
Suggested change
🧰 Tools🪛 ESLint[error] 223-223: 'result' is assigned a value but only used as a type. Allowed unused vars must match /^_/u. ( 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| test(`nested spread preserves object structure types`, () => { | ||||||||||||||||||||||||||||
| const users = createUsers() | ||||||||||||||||||||||||||||
| const col = createLiveQueryCollection((q) => { | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
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
Lint failure: unused
nicknamebinding.ESLint flags
nicknameas an unused variable (must match/^_/). You can't just rename the binding to_nicknamebecause that would change which property is destructured-out and break the omission semantics this test relies on. Use a rename binding so the keynicknameis still removed from the rest while the local name satisfies the rule.🛠️ Proposed fix
📝 Committable suggestion
🧰 Tools
🪛 ESLint
[error] 218-218: 'nickname' is assigned a value but never used. Allowed unused vars must match /^_/u.
(
@typescript-eslint/no-unused-vars)🤖 Prompt for AI Agents
Source: Linters/SAST tools