Deprecate magic return APIs from collection handlers #843
Conversation
🦋 Changeset detectedLatest commit: 7b1aa37 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
More templates
@tanstack/angular-db
@tanstack/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
|
Size Change: +104 B (+0.11%) Total Size: 90.9 kB
ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 3.7 kB ℹ️ View Unchanged
|
|
I'm in favour of this change, it makes everything much more explicit. My only hesitation is on the query collection where it becomes a little more verbose with the need to refetch the majority of the time... but that explicitness does ensure that the user is aware of what is actually happening. on the whole, I think it's worth making this change. |
| // Users handle sync coordination in their own handlers | ||
| onInsert: config.onInsert, | ||
| onUpdate: config.onUpdate, | ||
| onDelete: config.onDelete |
There was a problem hiding this comment.
This is proof that this is a cleaner approach. Well done.
KyleAMathews
changed the title
kevin-dp
left a comment
kevin-dp
left a comment
There was a problem hiding this comment.
LGTM. I left a comment about improving the @deprecated annotation in the function's docstring.
packages/db/src/types.ts
Outdated
| * @param params Object containing transaction and collection information | ||
| * @returns Promise resolving to any value | ||
| * @returns Promise that should resolve to void | ||
| * @deprecated Returning values from this handler is deprecated. Use collection utilities to refetch/sync. |
There was a problem hiding this comment.
I believe that @deprecated is going to mark the onInsert function as deprecated which is not really the case. It's only a specific usage of it that will be deprecated.
To do it right i think we should have an overload for the old signature and an overload for the new signature. And then deprecate the old signature via docstring. Like in this artificial example:
// Deprecated overload (old usage)
/** @deprecated This function no longer returns a value. */
export function configure(): ConfigObject;
// New overload (current usage)
export function configure(): void;
// Implementation
export function configure() {
// ...
}
There was a problem hiding this comment.
Claude finished @kevin-dp's task in 3m 49s —— View job
Addressed deprecation annotation
I've improved the @deprecated annotations as suggested by using union types to create overloads.
What I changed
Instead of marking the entire onInsert, onUpdate, and onDelete handler functions as deprecated, I've restructured the type definitions to use union types that provide two alternatives:
- The current, non-deprecated signature:
InsertMutationFn<T, TKey, TUtils, void>(returnsPromise<void>) - The deprecated signature:
InsertMutationFn<T, TKey, TUtils, TReturn>(returnsPromise<TReturn>) with a@deprecatedJSDoc comment
This approach ensures that:
- Users who write handlers returning
void(the new, recommended way) won't see deprecation warnings - Users who write handlers returning other values will see the deprecation warning specifically on that usage
- The deprecation message clearly explains what's deprecated and how to migrate
Changes made
- Read current types.ts implementation
- Identify all handler types that need overload treatment
- Create union type overloads for deprecated signatures
- Update deprecation messages to be clearer
- Commit and push changes
The changes have been pushed to the branch in commit 2716d59.
| Branch
BREAKING CHANGE: Mutation handler return values are now deprecated
**Problem:**
Users were confused about the difference between collection handlers
(onInsert, onUpdate, onDelete) and manual actions/transactions. The
magic of returning values like `{ refetch: false }` from handlers was
specific to Query Collections but users expected it to work everywhere,
leading to incorrect mental models.
**Solution:**
1. Deprecate returning values from mutation handlers in TypeScript
- Changed `TReturn = any` to `TReturn = void` in base handler types
- Added @deprecated JSDoc tags with migration guidance
- Updated all handler examples to show manual refetch pattern
2. Update documentation to teach manual patterns:
- Query Collections: Use `await collection.utils.refetch()`
- Electric Collections: Return `{ txid }` or use `collection.utils.awaitMatch()`
3. Clarify Electric Collection's special pattern:
- Electric handlers REQUIRE returning txid or calling awaitMatch
- This is Electric-specific, not a general pattern
- Updated JSDoc to emphasize this distinction
**Migration Guide:**
Before (deprecated):
```typescript
onInsert: async ({ transaction }) => {
await api.create(data)
return { refetch: false } // ❌ Deprecated
}
```
After (recommended):
```typescript
onInsert: async ({ transaction, collection }) => {
await api.create(data)
await collection.utils.refetch() // ✅ Explicit and clear
}
```
For Electric Collections (unchanged):
```typescript
onInsert: async ({ transaction }) => {
const result = await api.create(data)
return { txid: result.txid } // ✅ Electric-specific pattern
}
```
**Files Changed:**
- packages/db/src/types.ts: Deprecated return types, updated JSDoc
- packages/electric-db-collection/src/electric.ts: Clarified Electric-specific pattern
- docs/collections/query-collection.md: Removed refetch control examples, added manual patterns
- docs/guides/mutations.md: Updated collection-specific handler patterns
This change reduces confusion and makes the API more explicit and easier to understand.
…alues Update the guide for creating custom collection options creators to reflect the deprecation of mutation handler return values. **Changes:** 1. **Pattern A (User-Provided Handlers):** - Clarified that handler return values are deprecated - Users should manually trigger refetch/sync within their handlers - Added note about Electric-specific exception (txid returns) - Simplified example to just pass through handlers 2. **Pattern B (Built-in Handlers):** - Updated examples to show handlers completing after sync coordination - Removed misleading return statements - Added key principle: coordinate sync internally via `await` 3. **Strategy 5 (Query Collection):** - Renamed from "Full Refetch" to "Manual Refetch" - Updated to show user explicitly calling `collection.utils.refetch()` - Clarified that users manage refetch in their own handlers 4. **WebSocket Example:** - Added explicit `Promise<void>` return types to handlers - Added comments about handlers completing after server confirmation 5. **Electric Example:** - Clarified that txid returns are Electric-specific - Updated wrapper example to not return the result 6. **Best Practices:** - Added new guideline about deprecated handler return values - Emphasized Electric is the only exception These changes align the guide with the new pattern where mutation handlers don't return values, and sync coordination happens explicitly within handlers via await or manual refetch calls.
…pattern
BREAKING CHANGE: Electric collection handlers no longer support returning { txid }
**Problem:**
The previous commit deprecated return values for most collections but kept
Electric's `return { txid }` pattern as an exception. This creates confusion
because it's still "magic" - users don't understand why Electric is different
and it maintains the same conceptual problem of implicit behavior.
**Solution:**
Deprecate ALL return value patterns, including Electric's. Electric handlers
should now use `await collection.utils.awaitTxId(txid)` explicitly.
**Changes:**
1. **Base Collection Types** (`packages/db/src/types.ts`):
- Added Electric example showing manual `await collection.utils.awaitTxId()`
- Removed mentions of Electric-specific return pattern
- All handlers consistently show void return with manual sync
2. **Electric Collection** (`packages/electric-db-collection/src/electric.ts`):
- Deprecated `return { txid }` pattern in all three handlers
- Updated JSDoc to show manual `await collection.utils.awaitTxId(txid)`
- Added `@deprecated` tags explaining the migration
- Updated all examples to use manual await pattern
3. **Electric Documentation** (`docs/collections/electric-collection.md`):
- Updated "Using Txid" section to show manual awaitTxId
- Changed "return txid to wait for sync" to "manually wait for txid"
- All code examples now use `await collection.utils.awaitTxId()`
4. **Collection Options Creator Guide** (`docs/guides/collection-options-creator.md`):
- Removed "Electric-Specific Exception" note
- Updated best practices to mention Electric should use awaitTxId
- No more special cases - all handlers work the same way
5. **Mutations Guide** (`docs/guides/mutations.md`):
- Updated Electric example to show manual awaitTxId pattern
- Changed from "return { txid }" to manual await pattern
**Migration Guide:**
Before (deprecated):
```typescript
// Electric Collection
onInsert: async ({ transaction }) => {
const result = await api.create(data)
return { txid: result.txid } // ❌ Deprecated
}
```
After (recommended):
```typescript
// Electric Collection
onInsert: async ({ transaction, collection }) => {
const result = await api.create(data)
await collection.utils.awaitTxId(result.txid) // ✅ Explicit and clear
}
```
**Benefits:**
1. **No more magic**: All collections follow the same explicit pattern
2. **Clearer mental model**: Users understand they're waiting for sync
3. **Consistent API**: No special cases to remember
4. **Better visibility**: `await` makes the async nature explicit
5. **Same control**: Users have same power, just more explicit
All collections now have a consistent pattern: handlers coordinate sync
explicitly via await, not implicitly via return values.
…entation Cleaned up language throughout mutation handler docs and JSDoc to be more direct. Changed phrases like "manually wait for" to "wait for" since there's only one way to do it.
Based on external code review feedback, this commit implements a soft deprecation
strategy for mutation handler return values:
Runtime changes:
- Add console warnings when QueryCollection handlers return { refetch }
- Add console warnings when Electric handlers return { txid }
- Keep runtime functionality intact for backward compatibility
- Runtime support will be removed in v1.0 RC
Type improvements:
- Mark TReturn generic as @internal and deprecated across all mutation types
- Clarify that it exists only for backward compatibility
Documentation improvements:
- Clarify Electric JSDoc: handlers return Promise<void> but must not RESOLVE
until synchronization is complete (avoiding "void but not void" confusion)
- Add timeout error handling example showing policy choices (rollback vs
eventual consistency)
- Update changeset to clearly communicate this is a soft deprecation
This aligns with the review recommendation for a gradual migration path with
clear runtime feedback to help users migrate to the new explicit patterns.
kevin-dp
force-pushed
the
claude/fix-mutation-callbacks-01HTYzMduGUSY1Dvt3B7YBEa
branch
from
58eb211 to
8d9f691
Compare
|
/review |
| @@ -0,0 +1,51 @@ | |||
| --- | |||
| '@tanstack/db': major | |||
There was a problem hiding this comment.
Claude finished @KyleAMathews's task in 1m 52s —— View job
Changeset updated to use minor versions
I've updated the changeset file locally to change version bumps from major to minor for all three packages since the project is < 1.0:
@tanstack/db: major → minor@tanstack/electric-db-collection: major → minor@tanstack/query-db-collection: minor → minor
Status: The change is committed locally but cannot be pushed automatically due to concurrent changes on the remote branch (likely from another Claude instance addressing Kevin's deprecation comment).
Next steps: You'll need to either:
- Grant me fetch/pull permissions so I can rebase and push, or
- Manually apply this simple change to
.changeset/deprecate-handler-return-values.md(lines 2-4: changemajortominor)
Local commit: ad9f319a - chore: change version bumps from major to minor (< 1.0)
|
@claude throughly review all docs for any other docs that are using this now deprecated pattern & rewrite as needed to the new pattern |
Use union types to create overloads that deprecate only the non-void return signatures, not the entire handler functions. This provides more precise deprecation warnings to users. Co-authored-by: Kevin <kevin-dp@users.noreply.github.com>
|
Claude finished @KyleAMathews's task in 3m 24s —— View job Documentation review complete ✅I've thoroughly reviewed all documentation files and updated any usage of the deprecated magic return API pattern from collection handlers. Tasks
Pattern Change Summary:
Files Updated:
Files Already Updated (in this PR):
Note: CHANGELOG files contain historical information and were intentionally not modified. All documentation now consistently uses the new explicit sync pattern instead of deprecated return values! |
🔍 Comprehensive PR ReviewPR #843: Deprecate magic return APIs from collection handlers This PR deprecates the "magic return value" pattern from mutation handlers ( Summary
🟠 Important Issues (Should Fix)1. JSDoc/Type Declaration Mismatch in Electric CollectionFiles: The JSDoc comments state "This handler returns Recommendation: Update the JSDoc to accurately reflect: "This handler returns 2. Changeset Inaccurately Claims TypeScript Will ErrorFile: The changeset states "TypeScript will error on Recommendation: Reword to: "Handlers now default to 3. Poor Error Sequence for
|
| Aspect | Rating | Notes |
|---|---|---|
| Encapsulation | 7/10 | Good, but TReturn remains overridable |
| Invariant Expression | 7/10 | Clear intent, minor JSDoc/type mismatches |
| Invariant Usefulness | 9/10 | Excellent improvement for API clarity |
| Invariant Enforcement | 6/10 | Runtime enforcement only for deprecated pattern |
Recommended Action
- Fix the 4 important issues before merge (particularly the JSDoc/type mismatch and timeout documentation)
- Consider suggestions for code quality improvements
- The PR is fundamentally sound and ready for merge after addressing important issues
🤖 Generated with Claude Code
…recated return values - Update ElectricCollectionConfig examples to use await collection.utils.awaitTxId() - Update overview.md example to use explicit sync awaiting - Update mutations.md to show cleaner pattern for awaiting txids - Update examples/react/projects/README.md to use collection.utils.refetch() Co-authored-by: Kyle Mathews <KyleAMathews@users.noreply.github.com>
- Fix changeset to remove incorrect claim that TypeScript would error on old return patterns (types are generic/overridable, only runtime warns) - Update awaitTxId default timeout from 5s to 15s for more reliable sync - Fix documentation that incorrectly stated 30s default Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add warnOnce utility to prevent console spam (logs each warning once)
- QueryCollection: warn about deprecated auto-refetch behavior
- QueryCollection: clarify { refetch: false } is correct pattern for now
- ElectricCollection: use warnOnce for { txid } return deprecation
- Update docs to accurately describe current vs v1.0 behavior
- Update changeset with clearer migration guidance
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
KyleAMathews
force-pushed
the
claude/fix-mutation-callbacks-01HTYzMduGUSY1Dvt3B7YBEa
branch
from
3e497ff to
5d564f7
Compare
🎯 Changes
✅ Checklist
pnpm test:pr.🚀 Release Impact