fix(solid-query): resolve query client outside memos#10571
Open
KimHyeongRae0 wants to merge 3 commits intoTanStack:mainfrom
Open
fix(solid-query): resolve query client outside memos#10571KimHyeongRae0 wants to merge 3 commits intoTanStack:mainfrom
KimHyeongRae0 wants to merge 3 commits intoTanStack:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a top-level QueryClient resolver ( ChangesQuery Client Context Resolution Pattern
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
|
View your CI Pipeline Execution ↗ for commit 3970ffc
☁️ Nx Cloud last updated this comment at |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/solid-query/src/useIsMutating.ts (1)
11-23: ⚡ Quick winSame subscription-reactivity gap as
useIsFetching— wrap increateEffect.
mutationCache().subscribe(...)at line 19 is called once at initialization. Ifclientchanges, the subscription won't follow the new mutation cache. Apply the samecreateEffect-based pattern used inuseMutationState:♻️ Suggested fix
- const [mutations, setMutations] = createSignal( - client().isMutating(filters?.()), - ) - - const unsubscribe = mutationCache().subscribe((_result) => { - setMutations(client().isMutating(filters?.())) - }) - - onCleanup(unsubscribe) + const [mutations, setMutations] = createSignal(0) + + createEffect(() => { + setMutations(client().isMutating(filters?.())) + const unsubscribe = mutationCache().subscribe(() => { + setMutations(client().isMutating(filters?.())) + }) + onCleanup(unsubscribe) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-query/src/useIsMutating.ts` around lines 11 - 23, The subscription to mutationCache in useIsMutating is created once and won't track changes if the resolved client changes; wrap the subscription logic in a createEffect (like useMutationState) that reads resolveClient()/client() and mutationCache() so it re-subscribes when client changes, call setMutations(client().isMutating(filters?.())) inside the effect, and ensure you call onCleanup to unsubscribe the previous mutationCache.subscribe; update functions/variables involved: resolveClient, client, mutationCache, setMutations, createEffect, and onCleanup so the subscription follows client changes.packages/solid-query/src/useMutation.ts (1)
24-25: ⚖️ Poor tradeoff
clientmemo is only read at initialization; client changes won't be reflected in the observer.
client(line 25) is read once at line 32 to constructMutationObserver. There is no reactive listener (e.g.createComputed(on(client, ...))) that would recreate the observer if the resolvedQueryClientchanges later. This is pre-existing behavior, but unlikeuseBaseQuerywhich handles client-swap viaon(client, ...),useMutationsilently continues using the original client.Worth tracking in a follow-up if dynamic client swapping needs to be supported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-query/src/useMutation.ts` around lines 24 - 25, The MutationObserver in useMutation is constructed once using the memoized client (client = createMemo(() => resolveClient())) and never updated if the resolved QueryClient changes; modify useMutation so it watches the client memo and recreates/updates the MutationObserver when client changes (similar to useBaseQuery's on(client, ...) pattern): use a reactive listener (e.g., createComputed(on(client, ...))) inside useMutation to dispose the old MutationObserver and instantiate a new MutationObserver bound to the new resolveClient() result so the observer always uses the current QueryClient.packages/solid-query/src/useIsFetching.ts (1)
11-21: ⚡ Quick winSubscription doesn't react to
clientchanges — inconsistent withuseMutationState.The
queryCache().subscribe(...)call at line 17 is established once, outside any reactive scope. Ifclientchanges (e.g. the provider swaps itsQueryClient),queryCacherecomputes but the subscription stays bound to the old cache.useMutationStateavoids this correctly by wrapping its subscription increateEffect:createEffect(() => { const unsubscribe = mutationCache().subscribe(...) onCleanup(unsubscribe) })This PR makes
clientcorrectly reactive for the first time (fixing theuseContext-in-memo bug), so this latent subscription gap is now more likely to surface.♻️ Suggested fix
- const [fetches, setFetches] = createSignal(client().isFetching(filters?.())) - - const unsubscribe = queryCache().subscribe(() => { - setFetches(client().isFetching(filters?.())) - }) - - onCleanup(unsubscribe) + const [fetches, setFetches] = createSignal(0) + + createEffect(() => { + setFetches(client().isFetching(filters?.())) + const unsubscribe = queryCache().subscribe(() => { + setFetches(client().isFetching(filters?.())) + }) + onCleanup(unsubscribe) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-query/src/useIsFetching.ts` around lines 11 - 21, The subscription to queryCache is created once and won't update when the reactive client/queryCache changes; wrap the subscribe/unsubscribe logic in a reactive scope (e.g. createEffect) so it re-subscribes whenever client() or queryCache() changes and ensure onCleanup unsubscribes the previous listener; specifically, move the queryCache().subscribe(...) and its onCleanup into a createEffect that reads queryCache() (or client()) and also re-evaluates setFetches(client().isFetching(filters?.())) inside the effect so fetches updates immediately when client, queryCache or filters change (referencing resolveClient, client, queryCache, setFetches).packages/solid-query/src/__tests__/QueryClientProvider.test.tsx (1)
180-201: ⚡ Quick winAdd a test for
useQueryClientResolverwhen noQueryClientProvideris present.The PR's linked issue (
#10445) explicitly lists "ensure resolver avoids throws when context is missing/late" as a required test, but no such case is covered here. The existing suite only validates the happy path. It's unclear from the test file alone whetheruseQueryClientResolver()without a provider throws (likeuseQueryClient()does), silently returnsundefined, or behaves differently — and the expected contract should be locked down by a test.💡 Example test to add
+ describe('useQueryClientResolver', () => { + it('should throw when called outside a QueryClientProvider', () => { + const consoleMock = vi + .spyOn(console, 'error') + .mockImplementation(() => undefined) + + function Page() { + useQueryClientResolver() + return null + } + + // Adjust the expectation to match the actual contract (throw vs. safe no-op) + expect(() => render(() => <Page />)).toThrow( + 'No QueryClient set, use QueryClientProvider to set one', + ) + + consoleMock.mockRestore() + }) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-query/src/__tests__/QueryClientProvider.test.tsx` around lines 180 - 201, Add a test that verifies useQueryClientResolver is safe when no provider is present: render a component that captures resolveClient = useQueryClientResolver() but do not wrap it with <QueryClientProvider>, then inside a createRoot/createMemo call invoke resolveClient() and assert that invoking it does not throw and returns undefined (i.e., the resolver handles missing/late context); reference useQueryClientResolver, QueryClientProvider, createRoot and createMemo in the test to locate where to add the new case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/solid-query/src/__tests__/QueryClientProvider.test.tsx`:
- Around line 180-201: Add a test that verifies useQueryClientResolver is safe
when no provider is present: render a component that captures resolveClient =
useQueryClientResolver() but do not wrap it with <QueryClientProvider>, then
inside a createRoot/createMemo call invoke resolveClient() and assert that
invoking it does not throw and returns undefined (i.e., the resolver handles
missing/late context); reference useQueryClientResolver, QueryClientProvider,
createRoot and createMemo in the test to locate where to add the new case.
In `@packages/solid-query/src/useIsFetching.ts`:
- Around line 11-21: The subscription to queryCache is created once and won't
update when the reactive client/queryCache changes; wrap the
subscribe/unsubscribe logic in a reactive scope (e.g. createEffect) so it
re-subscribes whenever client() or queryCache() changes and ensure onCleanup
unsubscribes the previous listener; specifically, move the
queryCache().subscribe(...) and its onCleanup into a createEffect that reads
queryCache() (or client()) and also re-evaluates
setFetches(client().isFetching(filters?.())) inside the effect so fetches
updates immediately when client, queryCache or filters change (referencing
resolveClient, client, queryCache, setFetches).
In `@packages/solid-query/src/useIsMutating.ts`:
- Around line 11-23: The subscription to mutationCache in useIsMutating is
created once and won't track changes if the resolved client changes; wrap the
subscription logic in a createEffect (like useMutationState) that reads
resolveClient()/client() and mutationCache() so it re-subscribes when client
changes, call setMutations(client().isMutating(filters?.())) inside the effect,
and ensure you call onCleanup to unsubscribe the previous
mutationCache.subscribe; update functions/variables involved: resolveClient,
client, mutationCache, setMutations, createEffect, and onCleanup so the
subscription follows client changes.
In `@packages/solid-query/src/useMutation.ts`:
- Around line 24-25: The MutationObserver in useMutation is constructed once
using the memoized client (client = createMemo(() => resolveClient())) and never
updated if the resolved QueryClient changes; modify useMutation so it watches
the client memo and recreates/updates the MutationObserver when client changes
(similar to useBaseQuery's on(client, ...) pattern): use a reactive listener
(e.g., createComputed(on(client, ...))) inside useMutation to dispose the old
MutationObserver and instantiate a new MutationObserver bound to the new
resolveClient() result so the observer always uses the current QueryClient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80c0bf91-5702-4412-964a-c1bb97f3c67f
📒 Files selected for processing (9)
.changeset/solid-query-client-resolver.mdpackages/solid-query/src/QueryClientProvider.tsxpackages/solid-query/src/__tests__/QueryClientProvider.test.tsxpackages/solid-query/src/useBaseQuery.tspackages/solid-query/src/useIsFetching.tspackages/solid-query/src/useIsMutating.tspackages/solid-query/src/useMutation.tspackages/solid-query/src/useMutationState.tspackages/solid-query/src/useQueries.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/solid-query/src/__tests__/useIsFetching.test.tsx (1)
264-301: ⚡ Quick winMake this test verify unsubscribe, not just the displayed count.
The current assertions don’t prove the old
queryClient1query-cache listener was removed. A stale listener would still call back intoclient()and read the new client aftersetClient(queryClient2), so this can stay green with a leak. Please assert that the first cache subscription is cleaned up when the client accessor changes.Example: wrap the first cache subscription and assert its cleanup
it('should resubscribe when a custom queryClient changes', async () => { const queryClient1 = new QueryClient() const queryClient2 = new QueryClient() const key1 = queryKey() const key2 = queryKey() const [client, setClient] = createSignal(queryClient1) + const queryCache1 = queryClient1.getQueryCache() + const originalSubscribe1 = queryCache1.subscribe.bind(queryCache1) + const unsubscribe1 = vi.fn() + + vi.spyOn(queryCache1, 'subscribe').mockImplementation((listener) => { + const cleanup = originalSubscribe1(listener) + return () => { + unsubscribe1() + cleanup() + } + }) function Page() { const isFetching = useIsFetching(undefined, client) return <div>isFetching: {isFetching()}</div> @@ expect(rendered.getByText('isFetching: 1')).toBeInTheDocument() setClient(queryClient2) + expect(unsubscribe1).toHaveBeenCalledTimes(1) expect(rendered.getByText('isFetching: 0')).toBeInTheDocument()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-query/src/__tests__/useIsFetching.test.tsx` around lines 264 - 301, The test should assert that the old QueryClient's cache listener is removed when the client accessor changes; wrap or spy on queryClient1.getQueryCache().subscribe (or the subscribe return's unsubscribe function) before rendering Page (which uses useIsFetching with client accessor), capture the unsubscribe function or a spy, then after calling setClient(queryClient2) assert that the unsubscribe was called (or that the returned subscription is cleaned up) in addition to the displayed count assertions so you prove the subscription was removed from queryClient1 when Page switched clients.packages/solid-query/src/__tests__/useIsMutating.test.tsx (1)
210-245: ⚡ Quick winAssert old-cache cleanup explicitly in this resubscription test.
This sequence can still pass if the
queryClient1listener leaks, because the stale callback inuseIsMutatingreadsclient()when it fires and will seequeryClient2after the swap. Please make the test observe the first subscription’s cleanup directly.Example: verify the first unsubscribe runs on client switch
it('should resubscribe when a custom queryClient changes', async () => { const queryClient1 = new QueryClient() const queryClient2 = new QueryClient() const [client, setClient] = createSignal(queryClient1) + const mutationCache1 = queryClient1.getMutationCache() + const originalSubscribe1 = mutationCache1.subscribe.bind(mutationCache1) + const unsubscribe1 = vi.fn() + + vi.spyOn(mutationCache1, 'subscribe').mockImplementation((listener) => { + const cleanup = originalSubscribe1(listener) + return () => { + unsubscribe1() + cleanup() + } + }) function Page() { const isMutating = useIsMutating(undefined, client) return <div>mutating: {isMutating()}</div> @@ expect(rendered.getByText('mutating: 1')).toBeInTheDocument() setClient(queryClient2) + expect(unsubscribe1).toHaveBeenCalledTimes(1) expect(rendered.getByText('mutating: 0')).toBeInTheDocument()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-query/src/__tests__/useIsMutating.test.tsx` around lines 210 - 245, The test must explicitly assert that the old mutation cache subscription is cleaned up when swapping clients: wrap or spy on queryClient1.getMutationCache().subscribe (or the returned unsubscribe) before rendering Page, record when the unsubscribe is invoked, call setClient(queryClient2) and assert that the unsubscribe was called (showing the first subscription was removed) in addition to the existing mutating-count assertions; reference queryClient1, getMutationCache, subscribe/returned-unsubscribe, useIsMutating and setClient to locate where to instrument the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/solid-query/src/__tests__/useIsFetching.test.tsx`:
- Around line 264-301: The test should assert that the old QueryClient's cache
listener is removed when the client accessor changes; wrap or spy on
queryClient1.getQueryCache().subscribe (or the subscribe return's unsubscribe
function) before rendering Page (which uses useIsFetching with client accessor),
capture the unsubscribe function or a spy, then after calling
setClient(queryClient2) assert that the unsubscribe was called (or that the
returned subscription is cleaned up) in addition to the displayed count
assertions so you prove the subscription was removed from queryClient1 when Page
switched clients.
In `@packages/solid-query/src/__tests__/useIsMutating.test.tsx`:
- Around line 210-245: The test must explicitly assert that the old mutation
cache subscription is cleaned up when swapping clients: wrap or spy on
queryClient1.getMutationCache().subscribe (or the returned unsubscribe) before
rendering Page, record when the unsubscribe is invoked, call
setClient(queryClient2) and assert that the unsubscribe was called (showing the
first subscription was removed) in addition to the existing mutating-count
assertions; reference queryClient1, getMutationCache,
subscribe/returned-unsubscribe, useIsMutating and setClient to locate where to
instrument the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2916fe7e-3bfa-4ba1-adb1-64a361016163
📒 Files selected for processing (5)
packages/solid-query/src/__tests__/QueryClientProvider.test.tsxpackages/solid-query/src/__tests__/useIsFetching.test.tsxpackages/solid-query/src/__tests__/useIsMutating.test.tsxpackages/solid-query/src/useIsFetching.tspackages/solid-query/src/useIsMutating.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/solid-query/src/tests/QueryClientProvider.test.tsx
- packages/solid-query/src/useIsFetching.ts
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.
Changes
Closes #10445.
Solid hooks no longer call
useQueryClientfrom insidecreateMemo.useQueryClientResolvercapturesQueryClientContextat hook top level, then affected hooks call the returned accessor from memos.This keeps explicit
queryClientaccessors reactive and leaves the existinguseQueryClient(client)API unchanged.Checklist
pnpm run test:pr.Local verification:
pnpm --filter @tanstack/solid-query test:libpnpm --filter @tanstack/solid-query test:types:tscurrentpnpm --filter @tanstack/solid-query test:eslintRelease Impact
Summary by CodeRabbit
Bug Fixes
Tests
Chores