Issue #8469 - fix(solid-query): client() doesn't return undefined #3
Conversation
|
Thank you very much for taking a look into this @gopnik5! I cannot make a statement about the implementation of the fix, but a test I wrote locally for the issue indeed passes now. Maybe it is useful to you as template for a regression test (in // ...
// I only managed to reproduce the issue with @solidjs/router, so I had to add it to devDependencies
import { Route, MemoryRouter, createMemoryHistory } from '@solidjs/router'
describe('useQuery', () => {
// ...
it('should not reproduce issue #8469', async () => {
const queryCache = new QueryCache()
const queryClient = new QueryClient({ queryCache });
const history = createMemoryHistory();
const errorHandler = vi
.fn<(err: unknown) => void>()
function App() {
return (
<ErrorBoundary
fallback={(err) => {
errorHandler(err)
return err.message
}}
>
<Suspense>
<MemoryRouter history={history}>
<Route path="/" component={Index} />
<Route path="/sub" component={Sub} />
</MemoryRouter>
</Suspense>
</ErrorBoundary>
)
}
queryClient.setQueryData(['parent'], { id: 123})
function Index() {
return 'Index'
}
function Sub() {
const parent = useQuery(() => ({
queryKey: ['parent'],
async queryFn() {
await new Promise((r) => setTimeout(r, 100))
return {
id: 123,
}
},
// refetchOnMount: false, // this would remove the error
}))
const childQuery = useQuery(() => ({
queryKey: ['sub', parent.data?.id],
// enabled: parent.data?.id != null,
async queryFn() {
await new Promise((r) => setTimeout(r, 200))
return Promise.resolve('child' + parent.data?.id)
},
}))
return <pre>{childQuery.data}</pre>
}
const rendered = render(() => (
<QueryClientProvider client={queryClient}>
<App />
</QueryClientProvider>
))
await waitFor(() => rendered.getByText('Index'))
// Navigate to the sub page to trigger the error
history.set({
value: '/sub',
})
// Wait for the error to occur
await sleep(200)
expect(errorHandler).not.toHaveBeenCalled()
await waitFor(() => {
expect(rendered.getByText('child123')).toBeInTheDocument()
})
})
}) |
|
Thank you! I'll take a look. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe changes introduce a new development dependency, Changes
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/solid-query/package.json(1 hunks)packages/solid-query/src/__tests__/useQuery.test.tsx(3 hunks)packages/solid-query/src/useBaseQuery.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/solid-query/src/useBaseQuery.ts (2)
packages/solid-query/src/index.ts (1)
useQueryClient(99-99)packages/solid-query/src/QueryClientProvider.tsx (1)
useQueryClient(14-25)
🪛 GitHub Actions: pr
packages/solid-query/package.json
[error] 1-1: pnpm install failed with frozen-lockfile because pnpm-lock.yaml is outdated and does not match devDependencies in package.json. Update the lockfile or run 'pnpm install --no-frozen-lockfile'.
🪛 GitHub Actions: autofix.ci
packages/solid-query/package.json
[error] 1-1: pnpm install failed with exit code 1 due to outdated lockfile. The 'pnpm-lock.yaml' devDependencies do not match those in package.json. Run 'pnpm install' without --frozen-lockfile to update the lockfile.
🔇 Additional comments (3)
packages/solid-query/package.json (1)
72-72:❓ Verification inconclusive
New dependency added to support routing tests.
The
@solidjs/routerdependency has been added to support the new test case that verifies proper handling of dependent queries in a routing context.The pipeline is failing because the lockfile hasn't been updated. Please run:
🏁 Script executed:
#!/bin/bash # Check for missing lockfile update if grep -q "pnpm install failed with frozen-lockfile" <<< "$PIPELINE_FAILURES"; then echo "Please run 'pnpm install' to update the lockfile after adding @solidjs/router" fiLength of output: 52
Action Required: Verify Lockfile Update
The new dependency (
@solidjs/router) has been added to support the routing tests. However, while the original note indicates that the pipeline is failing due to an outdated lockfile, our automated check did not capture any error message (e.g., "pnpm install failed with frozen-lockfile"). This discrepancy suggests that the pipeline failure might not be strictly due to the lockfile being out-of-date, or the error message isn’t matching the expected output.
- Location:
packages/solid-query/package.jsonat line 72, where"@solidjs/router": "^0.15.3"is defined.- Recommendation: Please manually verify that the lockfile (typically
pnpm-lock.yaml) has been updated to reflect the new dependency. If it hasn’t, runpnpm installto update it.packages/solid-query/src/__tests__/useQuery.test.tsx (2)
5-5: Added imports for Suspense and router-related components.These new imports are necessary for the new dependent queries test case that tests behavior in a routing context.
Also applies to: 15-15
6051-6124: Added regression test for Issue TanStack#8469.This test verifies that dependent queries (where one query depends on data from another) work correctly in a routing context. It ensures that the query client is properly initialized and accessible when navigating between routes.
The test creates a routing setup with parent and child queries, where the child query depends on data from the parent query. This effectively tests the fix in
useBaseQuery.tswhere the client memo now receives an initial value.
| const client = createMemo( | ||
| () => useQueryClient(queryClient?.()), | ||
| useQueryClient(queryClient?.()), | ||
| ) |
There was a problem hiding this comment.
Fixed client initialization by providing initial value to createMemo.
This change ensures that the query client is properly initialized with an initial value, resolving the issue where client() doesn't return undefined as expected.
The second parameter to createMemo provides an immediate value that will be used before the first execution of the memo function, ensuring the client is available immediately.
Issue TanStack#8469 - fix(solid-query): client() doesn't return undefined
Summary by CodeRabbit
Tests
Chores
@solidjs/routerpackage as a development dependency.Refactor