feat(query-db-collection): support runtime QueryClient binding#1663
Closed
KyleAMathews wants to merge 3 commits into
Closed
feat(query-db-collection): support runtime QueryClient binding#1663KyleAMathews wants to merge 3 commits into
KyleAMathews wants to merge 3 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds ChangesRuntime QueryClient Binding
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Application
participant defineQueryCollectionOptions
participant bind
participant queryCollectionOptions
participant QueryClient
Application->>defineQueryCollectionOptions: Define collection config
Application->>bind: bind({ queryClient })
bind->>queryCollectionOptions: Add the runtime QueryClient
queryCollectionOptions->>QueryClient: Create and fetch the collection
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
More templates
@tanstack/angular-db
@tanstack/browser-db-sqlite-persistence
@tanstack/capacitor-db-sqlite-persistence
@tanstack/cloudflare-durable-objects-db-sqlite-persistence
@tanstack/db
@tanstack/db-ivm
@tanstack/db-sqlite-persistence-core
@tanstack/electric-db-collection
@tanstack/electron-db-sqlite-persistence
@tanstack/expo-db-sqlite-persistence
@tanstack/node-db-sqlite-persistence
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/react-native-db-sqlite-persistence
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/tauri-db-sqlite-persistence
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
Contributor
|
Size Change: 0 B Total Size: 125 kB ℹ️ View Unchanged
|
Contributor
|
Size Change: 0 B Total Size: 4.22 kB ℹ️ View Unchanged
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/query-db-collection/src/query.ts (1)
362-365: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider guarding against mutation of the shared
configreference.
DefinedQueryCollectionOptions.configexposes the raw config object by reference, andbind()spreads it fresh on every call ({ ...config, queryClient }). If a consumer mutatesdefinition.config(or a nested object/array within it) betweenbind()calls — intentionally or by accident — every subsequent binding silently picks up the mutation, which can be surprising in multi-tenant/request-scoped usage (the exact scenario this API targets).Consider freezing the config before returning it, or exposing it via a getter/clone instead of a raw mutable reference, to keep the definition immutable once created.
🛡️ Proposed defensive fix
return { - config, + config: Object.freeze(config), bind: ({ queryClient }) => queryCollectionOptions({ ...config, queryClient, }), }As per coding guidelines, "Encapsulate implementation details within responsible classes; use delegation to maintain clean boundaries between components" and "Avoid exposing internal properties directly; instead add public methods that delegate to internal implementations."
Also applies to: 540-547
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query-db-collection/src/query.ts` around lines 362 - 365, Make DefinedQueryCollectionOptions.config immutable rather than exposing the shared mutable reference. Update the configuration creation and binding logic associated with DefinedQueryCollectionOptions and bind() to return or use a defensive deep clone/frozen snapshot, including nested objects and arrays, so mutations to definition.config cannot affect later bindings.Source: Coding guidelines
packages/query-db-collection/tests/query.test.ts (1)
191-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for repeated/concurrent
bind()calls on the same definition.Current tests cover a single bind with sync, and isolation across two distinct
QueryClients — good baseline. Missing: callingbind()twice on the sameQueryClient/definition (to confirm no shared mutable state leaks between the two resulting collections), and a case exercising bind happening while the first fetch is still in flight.As per coding guidelines, "/*.test.{ts,tsx,js}: Test corner cases including... async race conditions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query-db-collection/tests/query.test.ts` around lines 191 - 246, Add tests covering repeated and concurrent bind calls on the same definition: create two collections from one definition bound to the same QueryClient and verify their state does not leak or share mutable collection state, then invoke a second bind while the first query fetch remains pending and assert both collections initialize correctly after the fetch resolves. Place these cases alongside the existing binding tests and use deferred promises or equivalent synchronization to exercise the in-flight race.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/query-db-collection/src/query.ts`:
- Around line 362-365: Make DefinedQueryCollectionOptions.config immutable
rather than exposing the shared mutable reference. Update the configuration
creation and binding logic associated with DefinedQueryCollectionOptions and
bind() to return or use a defensive deep clone/frozen snapshot, including nested
objects and arrays, so mutations to definition.config cannot affect later
bindings.
In `@packages/query-db-collection/tests/query.test.ts`:
- Around line 191-246: Add tests covering repeated and concurrent bind calls on
the same definition: create two collections from one definition bound to the
same QueryClient and verify their state does not leak or share mutable
collection state, then invoke a second bind while the first query fetch remains
pending and assert both collections initialize correctly after the fetch
resolves. Place these cases alongside the existing binding tests and use
deferred promises or equivalent synchronization to exercise the in-flight race.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 484448fe-fbff-4bd8-834f-654b6ecab88e
📒 Files selected for processing (6)
.changeset/runtime-query-binding.mddocs/collections/query-collection.mdpackages/query-db-collection/src/index.tspackages/query-db-collection/src/query.tspackages/query-db-collection/tests/query.test-d.tspackages/query-db-collection/tests/query.test.ts
Collaborator
Author
|
Closing this implementation direction for now. The API shape feels like unnecessary sugar; we'll first document the request-local QueryClient factory pattern and only add API if the docs-only approach proves insufficient. |
Collaborator
Author
|
Follow-up: closed this implementation direction because the bind-style API feels like unnecessary sugar at this point. We'll first document the plain request-local QueryClient factory pattern and only add API if that proves insufficient. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
defineQueryCollectionOptionsso query collection definitions can be created without aQueryClientand bound later with a request-local/runtime client. This keeps the existingqueryCollectionOptions({ queryClient, ... })API working while supporting SSR/TanStack Start-style per-request clients.Approach
defineQueryCollectionOptions(config)as an additive helper for unbound query collection definitions.bind({ queryClient }), which delegates to the existingqueryCollectionOptions({ ...config, queryClient })lifecycle path.queryClientis rejected at definition time and must be supplied during binding.Key invariants
QueryClientis introduced.queryCollectionOptions({ queryClient, ... })usage remains supported.QueryClientpassed to.bind(...).queryFn,getKey, and schema usage is preserved.Non-goals
QueryClient.Trade-offs
The new helper is intentionally a thin wrapper over
queryCollectionOptionsrather than a separate runtime path. That keeps the API small and avoids duplicating query lifecycle behavior, while still separating definition-time configuration from runtimeQueryClientbinding.Verification
Files changed
packages/query-db-collection/src/query.ts— adds the runtime binding helper and types.packages/query-db-collection/src/index.ts— exports the new helper/types.packages/query-db-collection/tests/query.test.ts— covers runtime binding and separateQueryClientinstances.packages/query-db-collection/tests/query.test-d.ts— covers inference and rejectsqueryClientat definition time.docs/collections/query-collection.md— documents request-local/runtime binding usage..changeset/runtime-query-binding.md— records the package patch change.Related RFC: #1643
Addresses #436
Summary by CodeRabbit
defineQueryCollectionOptionsfor defining query collections without an immediately availableQueryClient.QueryClientbinding.