fix: support async Svelte #9810
Conversation
Svelte 5 now supports async await in components. One behavior there is that when a component is created as part of a boundary that has pending async work, its `$effect`s will not run until that async work is done. The way the code is written right now means you can end up in an infinite pending state: If you do `await someQuery.promise`, the `$effect` for the subscription will never run, and therefore the `await` will never resolve. This PR fixes it.
🦋 Changeset detectedLatest commit: 59a1acc The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
WalkthroughReplaces the previous $effect-based subscription in Changes
Sequence DiagramsequenceDiagram
participant Component as Svelte Component
participant WatchChanges as watchChanges
participant Observer
participant OnDestroy as onDestroy
rect rgb(245,245,255)
Note over Component,Observer: Previous ($effect) pattern
Component->>Observer: $effect establishes subscription
Observer-->>Component: returns unsubscribe
end
rect rgb(235,250,235)
Note over Component,WatchChanges: New pattern (eager init + watchChanges + onDestroy)
Component->>Observer: initialize unsubscribe (eager, SSR-aware)
Component->>WatchChanges: observe isRestoring / observer changes
alt Re-subscribe needed
WatchChanges->>Observer: call observer.updateResult()
WatchChanges->>Observer: subscribe -> return new unsubscribe
end
Component->>OnDestroy: register cleanup
OnDestroy->>Observer: call unsubscribe on destroy (safe try/catch)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.changeset/thin-bugs-change.md (1)
1-5: Changelog entry OK; consider clarifying scope.Looks good for a patch bump. Optionally expand the body to mention “Svelte 5 async boundaries/$effect deferral” and that it fixes a potential infinite pending state, to aid readers scanning releases.
packages/svelte-query/src/createBaseQuery.svelte.ts (2)
1-12: Fix import order to satisfy lint (external before local).Place
svelteimport before relative (“./…”) imports.-import { useIsRestoring } from './useIsRestoring.js' -import { useQueryClient } from './useQueryClient.js' -import { createRawRef } from './containers.svelte.js' -import { watchChanges } from './utils.svelte.js' -import type { QueryClient, QueryKey, QueryObserver } from '@tanstack/query-core' -// types... -import { onDestroy } from 'svelte' +import { onDestroy } from 'svelte' +import type { QueryClient, QueryKey, QueryObserver } from '@tanstack/query-core' +import { useIsRestoring } from './useIsRestoring.js' +import { useQueryClient } from './useQueryClient.js' +import { createRawRef } from './containers.svelte.js' +import { watchChanges } from './utils.svelte.js' +// types...
75-94: The web search doesn't specifically address QueryObserver idempotency. Let me search the query-core source to check the unsubscribe implementation.Good! I found evidence from the Andri Purnomo blog about how QueryObserver.subscribe() works. It shows the unsubscribe pattern: the subscribe method adds a listener and returns a function that removes it. Now let me verify the actual QueryObserver implementation by searching the repository code more specifically to understand unsubscribe idempotency.Based on the search output and the code patterns I've seen:
- QueryObserver.subscribe() returns a function that removes the listener from the
this.listenersset- The unsubscribe function appears to be created each time, making it callable multiple times without side effects
- The pattern
listeners.delete(listener)is idempotent - calling delete on a set is safe to call multiple times- Similar patterns exist across other framework integrations (Vue, Solid, React all show the same pattern)
The code review comment is asking for:
- Extract a shared
noopfunction (for readability and consistency)- Type
unsubscribeexplicitly as() => void- Confirm idempotency
Based on the evidence from the codebase searches and the widespread use of this pattern in other framework packages, the unsubscribe function IS idempotent. The review suggestions are valid improvements for code clarity and maintainability.
Final Response:
Extract noop constant and type unsubscribe explicitly for clearer intent and maintainability.
The unsubscribe function from
observer.subscribe()is idempotent—calling it multiple times is safe. The review suggestions improve readability by:
- Using a shared
noopconstant instead of inline() => undefined- Explicitly typing
unsubscribeas() => voidThese changes align with patterns already used throughout the framework integrations (Vue Query line 315, Solid Query line 294).
- let unsubscribe = - isRestoring.current && typeof window !== 'undefined' - ? () => undefined - : observer.subscribe(() => update(createResult())) + const noop: () => void = () => {} + let unsubscribe: () => void = + isRestoring.current && typeof window !== 'undefined' + ? noop + : observer.subscribe(() => update(createResult())) @@ watchChanges( () => [isRestoring.current, observer] as const, 'pre', () => { unsubscribe() - unsubscribe = isRestoring.current - ? () => undefined + unsubscribe = isRestoring.current + ? noop : observer.subscribe(() => update(createResult())) observer.updateResult() return unsubscribe }, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/thin-bugs-change.md(1 hunks)packages/svelte-query/src/createBaseQuery.svelte.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/svelte-query/src/createBaseQuery.svelte.ts (1)
packages/svelte-query/src/utils.svelte.ts (1)
watchChanges(17-44)
🪛 ESLint
packages/svelte-query/src/createBaseQuery.svelte.ts
[error] 11-11: svelte import should occur before import of ./useIsRestoring.js
(import/order)
🪛 LanguageTool
.changeset/thin-bugs-change.md
[grammar] ~1-~1: Hier könnte ein Fehler sein.
Context: --- '@tanstack/svelte-query': patch --- fix: support async Svelte
(QB_NEW_DE)
[grammar] ~5-~5: Hier könnte ein Fehler sein.
Context: ...y': patch --- fix: support async Svelte
(QB_NEW_DE)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/svelte-query/src/createBaseQuery.svelte.ts (2)
83-94: Avoid double-unsubscribe across effect cycles.watchChanges runs the previous cleanup before invoking the next effect; calling
unsubscribe()again at the start of the effect can double-call the prior unsubscribe. Often idempotent, but not guaranteed. Guard it and return a wrapped cleanup to mark when cleanup already ran.+ let cleanupRan = false watchChanges( () => [isRestoring.current, observer] as const, 'pre', () => { - unsubscribe() - unsubscribe = isRestoring.current - ? () => undefined - : observer.subscribe(() => update(createResult())) - observer.updateResult() - return unsubscribe + const prevUnsub = unsubscribe + if (!cleanupRan) prevUnsub() + unsubscribe = isRestoring.current + ? () => undefined + : observer.subscribe(() => update(createResult())) + observer.updateResult() + const currentUnsub = unsubscribe + cleanupRan = false + return () => { + cleanupRan = true + currentUnsub() + } }, )
96-98: Optional: make cleanup idempotent across effect cleanup and onDestroy.onDestroy may run after the effect’s cleanup, causing another unsubscribe call. Consider a small wrapper that ensures at-most-once cleanup, then use it both in the effect’s returned cleanup and here.
// place near the subscribe setup let didUnsub = false const safeUnsubscribe = () => { if (!didUnsub) { didUnsub = true unsubscribe() } }- onDestroy(() => { - unsubscribe() - }) + onDestroy(() => { + safeUnsubscribe() + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/svelte-query/src/createBaseQuery.svelte.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/svelte-query/src/createBaseQuery.svelte.ts (1)
packages/svelte-query/src/utils.svelte.ts (1)
watchChanges(17-44)
🔇 Additional comments (2)
packages/svelte-query/src/createBaseQuery.svelte.ts (2)
1-1: LGTM: onDestroy import enables SSR cleanup.Needed for server-side lifecycle cleanup.
75-81: Verify whether eager subscription logic correctly gates SSR vs. client hydration.The code conditionally skips subscription only when
isRestoring.current && typeof window !== 'undefined'are both true—this avoids re-subscription during client hydration but always subscribes during SSR. However,createQueries.svelte.tsuses simpler logic (only checkingisRestoring.currentwithout the window gate), suggesting possible inconsistency.Confirm: (1) whether
useIsRestoring().currentcan ever be true during server-side rendering, and (2) whether the window check is intentional or should be removed to match the pattern increateQueries.svelte.ts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/svelte-query/src/createBaseQuery.svelte.ts (2)
83-94: Avoid double-unsubscribe on re-subscriptionwatchChanges returns a cleanup that runs before the next effect. Because the effect also calls unsubscribe() at the start, the previous unsubscribe is invoked twice on subsequent runs. Likely harmless, but avoidable. Do a one-time unsubscribe to clear the initial eager subscription, then rely on cleanup thereafter.
Apply:
- watchChanges( - () => [isRestoring.current, observer] as const, - 'pre', - () => { - unsubscribe() - unsubscribe = isRestoring.current - ? () => undefined - : observer.subscribe(() => update(createResult())) - observer.updateResult() - return unsubscribe - }, - ) + let didInitialUnsubscribe = false + watchChanges( + () => [isRestoring.current, observer] as const, + 'pre', + () => { + if (!didInitialUnsubscribe) { + unsubscribe() + didInitialUnsubscribe = true + } + unsubscribe = isRestoring.current + ? () => undefined + : observer.subscribe(() => update(createResult())) + observer.updateResult() + return unsubscribe + }, + )Please confirm QueryObserver’s unsubscribe is idempotent. If not, this refactor becomes essential.
99-102: Optional: extract a safeOnDestroy helperThe try/catch pattern is fine. Consider a tiny util to reduce repetition and improve intent:
// utils export const safeOnDestroy = (fn: () => void) => { try { onDestroy(fn) } catch {} } // usage safeOnDestroy(() => unsubscribe())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/svelte-query/src/createBaseQuery.svelte.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/svelte-query/src/createBaseQuery.svelte.ts (1)
packages/svelte-query/src/utils.svelte.ts (1)
watchChanges(17-44)
🔇 Additional comments (2)
packages/svelte-query/src/createBaseQuery.svelte.ts (2)
1-1: Import of onDestroy looks goodNeeded for SSR-safe teardown below. No concerns.
77-81: Eager subscribe/SSR guard is correctSubscribes on SSR and suspended client branches; skips during client restoring. This addresses the deferred $effect in Svelte 5.
1 participant
🎯 Changes
Svelte 5 now supports async await in components. One behavior there is that when a component is created as part of a boundary that has pending async work, its
$effects will not run until that async work is done.The way the code is written right now means you can end up in an infinite pending state: If you do
await someQuery.promise, the$effectfor the subscription will never run, and therefore theawaitwill never resolve. This PR fixes it.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit