Feat: Preact Adapter #9935
Feat: Preact Adapter #9935
Conversation
|
WalkthroughAdds a new Changes
Estimated code review effortπ― 5 (Critical) | β±οΈ ~120 minutes Areas to focus review on:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touchesβ Failed checks (1 warning)
β Passed checks (2 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: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
β»οΈ Duplicate comments (1)
packages/preact-query/src/__tests__/useInfiniteQuery.test.tsx (1)
2-3: Same React import inconsistency as other test files.This test file also uses React imports instead of Preact. See the comment on
usePrefetchQuery.test.tsxregarding consistency.
π‘ Minor comments (8)
packages/preact-query/CHANGELOG.md-1-1 (1)
1-1: Incorrect package name in changelog header.The header says
@tanstack/react-querybut this is thepreact-querypackage.-# @tanstack/react-query +# @tanstack/preact-querypackages/preact-query/src/__tests__/useQuery.test-d.tsx-1-5 (1)
1-5: MissingUseQueryResultimport causes type error.Line 283 references
UseQueryResultbut it's not imported. This will cause a TypeScript compilation error.import { describe, expectTypeOf, it } from 'vitest' import { queryKey } from '@tanstack/query-test-utils' import { useQuery } from '../useQuery' import { queryOptions } from '../queryOptions' -import type { OmitKeyof, QueryFunction, UseQueryOptions } from '..' +import type { OmitKeyof, QueryFunction, UseQueryOptions, UseQueryResult } from '..'examples/preact/simple/src/index.tsx-9-9 (1)
9-9: Addrel="noopener noreferrer"to external links withtarget="_blank".Using
target="_blank"withoutrel="noopener"exposes the page to potential security risks where the opened page can accesswindow.opener.- <a href="https://preactjs.com" target="_blank"> + <a href="https://preactjs.com" target="_blank" rel="noopener noreferrer">packages/preact-query/src/__tests__/QueryResetErrorBoundary.test.tsx-26-27 (1)
26-27: SharedqueryCacheandqueryClientmay cause cross-test pollution.Same issue as in the persister test file. Consider clearing the client in
afterEachor creating fresh instances per test.afterEach(() => { vi.useRealTimers() + queryClient.clear() })Committable suggestion skipped: line range outside the PR's diff.
packages/preact-query/src/__tests__/fine-grained-persister.test.tsx-20-21 (1)
20-21: SharedqueryCacheandqueryClientacross tests may cause test pollution.These instances are created once at module scope and reused across all tests. Without cleanup between tests (e.g.,
queryClient.clear()inafterEach), cached query state from one test can leak into another, causing flaky or false-positive results.+ afterEach(() => { + vi.useRealTimers() + queryClient.clear() + }) - afterEach(() => { - vi.useRealTimers() - })Alternatively, create fresh instances per test in
beforeEach.Committable suggestion skipped: line range outside the PR's diff.
examples/preact/simple/package.json-1-22 (1)
1-22: Update Vite to the latest 7.x patch version.Vite 7.0.4 (July 2025) is not the latest 7.x patchβnewer versions (7.1/7.2 series) are available. Consider upgrading. TypeScript 5.9.3 (October 2025) and ESLint 9.36.0 (September 2025) are current valid releases.
packages/preact-query/src/__tests__/ssr.test.tsx-14-28 (1)
14-28:setIsServer(true)should be managed withinbeforeEach/afterEachlifecycle.
setIsServer(true)is called at the describe level (line 15), but its cleanup function is never invoked. This could leak the server state to other test suites.describe('Server Side Rendering', () => { - setIsServer(true) - let queryCache: QueryCache let queryClient: QueryClient + let restoreIsServer: () => void beforeEach(() => { + restoreIsServer = setIsServer(true) vi.useFakeTimers() queryCache = new QueryCache() queryClient = new QueryClient({ queryCache }) }) afterEach(() => { + restoreIsServer() vi.useRealTimers() })packages/preact-query/src/__tests__/useSuspenseQueries.test.tsx-108-116 (1)
108-116: Bug:setQueryDatais passed the queryFn function instead of the query result.Line 111 sets
query.queryFn(the function itself) as the query data, rather than the resolved value. This test passes because the check isonSuspendnot being called, but the data shape is incorrect.Apply this diff to fix the test setup:
it('should not suspend on mount if query has been already fetched', () => { const query = createQuery(1) - queryClient.setQueryData(query.queryKey, query.queryFn) + queryClient.setQueryData(query.queryKey, 1) render(<TestComponent queries={[query]} />) expect(onSuspend).not.toHaveBeenCalled() })
π§Ή Nitpick comments (39)
packages/preact-query/root.eslint.config.js (1)
20-40: Remove framework-specific terms not applicable to preact-query.The cspell word list contains terms specific to other TanStack Query adapters that don't apply to the Preact adapter:
solidjs(line 30) - SolidJS adaptervue-demi(line 37) - Vue adapterΙ΅kind,Ι΅providers(lines 38-39) - Angular adapterThis appears to be copied from another package's config. Consider removing these irrelevant entries and adding any Preact-specific terms if needed (e.g.,
preact).'tanstack', // Our package scope 'todos', // Too general word to be caught as error 'tsqd', // Our public interface (TanStack Query Devtools shorthand) 'tsup', // We use tsup as builder 'typecheck', // Field of vite.config.ts - 'vue-demi', // dependency of @tanstack/vue-query - 'Ι΅kind', // Angular specific - 'Ι΅providers', // Angular specific + 'preact', // Our target framework ],packages/preact-query/src/HydrationBoundary.tsx (2)
21-21: Consider using Preact-native types instead of React types.
React.ReactNodeis used here, but this is a Preact package. While Preact's compatibility layer often allows React types to work, using Preact's nativeComponentChildrentype frompreactwould be more consistent.+import type { ComponentChildren } from 'preact' + export interface HydrationBoundaryProps { state: DehydratedState | null | undefined options?: OmitKeyof<HydrateOptions, 'defaultOptions'> & { defaultOptions?: OmitKeyof< Exclude<HydrateOptions['defaultOptions'], undefined>, 'mutations' > } - children?: React.ReactNode + children?: ComponentChildren queryClient?: QueryClient }
110-110: Consider using Preact's VNode type for the return cast.For consistency with the Preact ecosystem, consider using
VNodefrom Preact instead ofReact.ReactElement.+import type { VNode } from 'preact' + - return children as React.ReactElement + return children as VNodepackages/preact-query/src/__tests__/useQuery.promise.test.tsx (1)
20-37: Consider isolating QueryClient per test to prevent state leakage.The
queryClientandqueryCacheare shared across all tests in this file. WhilebeforeAll/afterAllhandle the experimental option, test pollution can occur if one test leaves stale cache entries. Consider either:
- Clearing the cache in an
afterEachhook, or- Creating a fresh
QueryClientper test+ afterEach(() => { + queryCache.clear() + })packages/preact-query/src/queryOptions.ts (1)
40-50: Consider usingOmitKeyoffor consistency.Line 45 uses plain
OmitwhileUnusedSkipTokenOptions(line 30) usesOmitKeyof. For consistency with the rest of the codebase and to leverage the additional type safety ofOmitKeyof, consider updating this.export type DefinedInitialDataOptions< TQueryFnData = unknown, TError = DefaultError, TData = TQueryFnData, TQueryKey extends QueryKey = QueryKey, -> = Omit<UseQueryOptions<TQueryFnData, TError, TData, TQueryKey>, 'queryFn'> & { +> = OmitKeyof<UseQueryOptions<TQueryFnData, TError, TData, TQueryKey>, 'queryFn'> & { initialData: | NonUndefinedGuard<TQueryFnData> | (() => NonUndefinedGuard<TQueryFnData>) queryFn?: QueryFunction<TQueryFnData, TQueryKey> }packages/preact-query/src/types.ts (1)
160-189: Inconsistent omit utilities between suspense result types.
UseSuspenseQueryResultusesDistributiveOmit(line 163) whileUseSuspenseInfiniteQueryResultusesOmitKeyof(line 186). Since both are applied to union types (DefinedQueryObserverResultandDefinedInfiniteQueryObserverResultare unions per core types), they should use the same utility for consistent behavior.
DistributiveOmitdistributes over unions, which is typically desired for result types.export type UseSuspenseInfiniteQueryResult< TData = unknown, TError = DefaultError, -> = OmitKeyof< +> = DistributiveOmit< DefinedInfiniteQueryObserverResult<TData, TError>, 'isPlaceholderData' | 'promise' >packages/preact-query/README.md (2)
5-29: Clarify that this package targets Preact, not React, in the READMELine 5 still advertises βHooks for fetching, caching and updating asynchronous data in Reactβ, which is potentially confusing for
@tanstack/preact-query. Consider updating the wording (and, if desired, the bundle-size badge and related links) to explicitly mention Preact so users landing on this README understand theyβre in the Preact adapter package.
1-27: Optional: add meaningfulalttext for inline imagesA few
<img>tags use empty or missingaltattributes (e.g., Lines 1 and 12), which trips markdownlint and slightly hurts accessibility; adding short descriptions (oralt=""only where decorative) would address that.examples/preact/simple/tsconfig.json (1)
2-19: Verify thatpathsmappings are actually used by TypeScriptThis config defines
pathsfor"react"and"react-dom"(Lines 14β17) but doesnβt set abaseUrlorextendsanother config that does. In vanilla TypeScript,pathsare only honored whenbaseUrlis configured, so these aliases might be ignored by the language service even if Vite resolves them at build time. Consider adding"baseUrl": "."here or confirming that a parent tsconfig already provides it.packages/preact-query/src/__tests__/queryOptions.test.tsx (1)
5-13: Optional: decide whether identity or just structural equality is the contractRight now the test (Lines 6β13) uses
toStrictEqual, which only guarantees thatqueryOptionsreturns an equivalent object. If you want to lock in the stronger contract that it returns the same instance (matching the current implementation), you could switch totoBe(object); otherwise the existing assertion is fine and leaves room for cloning in the future.examples/preact/simple/README.md (1)
3-15: Minor docs/a11y polish for the example READMEYou might want to (a) add an
altattribute to the logo<img>on Line 4 and (b) wrap the bare URLs in Lines 11 and 15 in markdown links to satisfy markdownlint and slightly improve readability, but these are purely cosmetic.packages/preact-query/src/__tests__/infiniteQueryOptions.test.tsx (1)
15-15: UsetoBefor identity assertion.Since
infiniteQueryOptionsreturns the exact same object reference (identity behavior perinfiniteQueryOptions.tsline 146-148), usingtoBewould more accurately verify reference equality rather thantoStrictEqualwhich only checks deep equality.- expect(infiniteQueryOptions(object)).toStrictEqual(object) + expect(infiniteQueryOptions(object)).toBe(object)examples/preact/simple/src/index.tsx (1)
34-41: Add type annotations forResourceprops.Consider adding a TypeScript interface for the
Resourcecomponent props to improve type safety and developer experience.+interface ResourceProps { + title: string; + description: string; + href: string; +} + -function Resource(props) { +function Resource(props: ResourceProps) { return ( - <a href={props.href} target="_blank" class="resource"> + <a href={props.href} target="_blank" rel="noopener noreferrer" class="resource"> <h2>{props.title}</h2> <p>{props.description}</p> </a> ); }packages/preact-query/src/__tests__/mutationOptions.test.tsx (1)
10-18: Consider isolating QueryClient per test to prevent state leakage.A shared
QueryClientacross tests (created outside test blocks) can lead to flaky tests if mutations from previous tests persist in the cache. Consider creating a freshQueryClientin abeforeEachblock or within each test.describe('mutationOptions', () => { + let queryClient: QueryClient + beforeEach(() => { vi.useFakeTimers() + queryClient = new QueryClient() }) afterEach(() => { vi.useRealTimers() + queryClient.clear() })Note: Tests at lines 36-80 and beyond already create their own
queryClientwithin the test body, so this primarily affects consistency and future test additions.packages/preact-query/src/__tests__/useSuspenseInfiniteQuery.test.tsx (2)
12-14: Consider cleaning up QueryClient between tests.The shared
queryCacheandqueryClientat describe scope may accumulate mutation/query state across tests. Consider adding cleanup:describe('useSuspenseInfiniteQuery', () => { const queryCache = new QueryCache() const queryClient = new QueryClient({ queryCache }) + afterEach(() => { + queryCache.clear() + })
51-84: NODE_ENV manipulation is fragile but functional.The pattern of saving, modifying, and restoring
process.env.NODE_ENVworks in Vitest's runtime context. Consider usingvi.stubEnvfor cleaner environment stubbing that auto-restores:it('should log an error when skipToken is used in development environment', () => { - const envCopy = process.env.NODE_ENV - process.env.NODE_ENV = 'development' + vi.stubEnv('NODE_ENV', 'development') // ... test body ... consoleErrorSpy.mockRestore() - process.env.NODE_ENV = envCopy + vi.unstubAllEnvs() })With
restoreMocks: truein your Vite config, you could also rely on automatic restoration if usingvi.stubEnv.packages/preact-query/src/__tests__/fine-grained-persister.test.tsx (2)
2-2: Import should usepreact/compatinstead ofreactfor consistency.This is a Preact package, so the import should align with the package's ecosystem. While
preact/compatprovides React compatibility, explicitly importing fromreactin a Preact package test file may cause confusion and potential issues if the test environment isn't properly aliased.-import * as React from 'react' +import * as React from 'preact/compat'
54-67: Unused ref state pattern appears in all three test components.The
[_, setRef] = React.useState<HTMLDivElement | null>()andref={(value) => setRef(value)}pattern creates state that's never read. If this is intended to trigger re-renders on mount, consider documenting the intent or using a more explicit approach.Also applies to: 111-123, 152-164
examples/preact/simple/package.json (1)
12-18: Consider adding@tanstack/preact-query-devtoolsif available.If devtools are being introduced as part of this PR or planned, they would enhance the example. Otherwise, the dev dependencies look appropriate for the Vite + Preact + TypeScript stack.
packages/preact-query/src/__tests__/QueryResetErrorBoundary.test.tsx (1)
4-4: Import should usepreact/compatfor consistency with the Preact package.-import * as React from 'react' +import * as React from 'preact/compat'packages/preact-query/src/useIsFetching.ts (2)
8-10: TODO acknowledged: Consider addressing the preact/compat overhead.The comment is valid. If bundle size is a concern, using
@preact/signalsor a custom implementation ofuseSyncExternalStorecould reduce the preact/compat dependency overhead. This could be tracked as a follow-up optimization.Would you like me to open an issue to track this optimization?
19-26: Consider addinggetServerSnapshotfor SSR compatibility.
useSyncExternalStoreaccepts a third argument (getServerSnapshot) for server-side rendering. While the file is marked'use client', Preact apps may still encounter hydration scenarios where this is needed. ForisFetching, returning0during SSR is typically safe.return useSyncExternalStore( useCallback( (onStoreChange) => queryCache.subscribe(notifyManager.batchCalls(onStoreChange)), [queryCache], ), () => client.isFetching(filters), + () => 0, // Server snapshot: no queries fetching during SSR )Please verify if Preact's
useSyncExternalStorefrompreact/compatrequires the third argument for SSR scenarios in the TanStack Query use case.packages/preact-query/src/__tests__/suspense.test.tsx (1)
2-3: Imports should usepreact/compatfor consistency.-import { act, render } from '@testing-library/react' -import { Suspense } from 'react' +import { act, render } from '@testing-library/preact' +import { Suspense } from 'preact/compat'Note: Verify that
@testing-library/preactis available in the project's dev dependencies and provides equivalent functionality.packages/preact-query/src/__tests__/usePrefetchQuery.test.tsx (1)
26-36: Consider adding queryClient cleanup between tests.The
queryClientis created at module level but never cleared between tests. This can lead to test pollution where cached data from one test affects another.Add cleanup in
afterEach:afterEach(() => { vi.useRealTimers() + queryClient.clear() })packages/preact-query/src/QueryClientProvider.tsx (1)
3-5: Separate type imports from value imports.
ComponentChildrenandVNodeare types and should be imported usingimport typefor better tree-shaking and clarity.import type { QueryClient } from '@tanstack/query-core' -import { ComponentChildren, createContext, VNode } from 'preact' +import { createContext } from 'preact' +import type { ComponentChildren, VNode } from 'preact' import { useContext, useEffect } from 'preact/hooks'packages/preact-query/src/useMutation.ts (1)
42-44: Options object reference stability.The
optionsdependency will causesetOptionsto be called on every render sinceoptionsis typically a new object each time. This is acceptable becauseMutationObserver.setOptionsinternally handles diffing, but documenting this behavior or using a stable options reference pattern could improve clarity.packages/preact-query/src/__tests__/useInfiniteQuery.test.tsx (1)
49-65: Add queryClient cleanup between tests.Similar to the other test file, the shared
queryClientshould be cleared inafterEachto prevent test pollution.afterEach(() => { vi.useRealTimers() + queryClient.clear() })packages/preact-query/src/__tests__/useSuspenseQuery.test.tsx (2)
31-32: SharedqueryCacheandqueryClientmay cause test pollution.The
queryCacheandqueryClientare instantiated once at the module level and reused across all tests. This can lead to state leakage between tests if queries with the same key are used or if the cache isn't cleared properly.Consider moving these into
beforeEachto ensure test isolation:- const queryCache = new QueryCache() - const queryClient = new QueryClient({ queryCache }) + let queryCache: QueryCache + let queryClient: QueryClient beforeEach(() => { + queryCache = new QueryCache() + queryClient = new QueryClient({ queryCache }) vi.useFakeTimers() }) afterEach(() => { vi.useRealTimers() + queryClient.clear() })
665-667: Redundant error re-throw after suspense query.This explicit error re-throw is redundant for
useSuspenseQuerysince it already throws errors by default whenthrowOnErroris enabled (which is the default for suspense queries). This code path would only be reached after successful data resolution.- if (result.error) { - throw result.error - }packages/preact-query/src/__tests__/useSuspenseQueries.test.tsx (2)
260-267: Timer setup duplication may cause conflicts.This nested
describeblock sets up fake timers withbeforeEach/afterEach, but the outerdescribeblock (lines 40-46) already usesbeforeAll/afterAllfor fake timers. This could lead to timer state conflicts between tests.Consider either:
- Removing the inner timer setup since the outer block already handles it, or
- Restructuring to have a single timer management strategy across all tests
670-677: Duplicate fake timer setup.This nested
describeblock duplicates the fake timer setup that already exists in the parentdescribeblock at lines 261-267. Since both usebeforeAll/afterAll, the inner setup is redundant.Remove the duplicate timer setup:
describe('gc (with fake timers)', () => { - beforeAll(() => { - vi.useFakeTimers() - }) - - afterAll(() => { - vi.useRealTimers() - }) - it('should gc when unmounted while fetching with low gcTime (#8159)', async () => {packages/preact-query/src/useBaseQuery.ts (1)
118-120: Effect runs on every render due todefaultedOptionsobject recreation.
defaultedOptionsis a new object on every render (created byclient.defaultQueryOptions(options)on line 59), causing thisuseEffectto run every render. Whileobserver.setOptionsmay internally short-circuit if options are equivalent, this is inefficient.Consider memoizing the options or using a ref-based comparison pattern similar to react-query's implementation to avoid unnecessary effect executions.
packages/preact-query/src/__tests__/utils.tsx (2)
9-23: Consider adding explicit return type.The
as anycast on line 22 bypasses type safety. Consider defining a proper return type that extends the render result with the custom rerender signature.+interface RenderWithClientResult extends Omit<ReturnType<typeof render>, 'rerender'> { + rerender: (rerenderUi: React.ReactElement) => void +} + export function renderWithClient( client: QueryClient, ui: React.ReactElement, -): ReturnType<typeof render> { +): RenderWithClientResult { const { rerender, ...result } = render( <QueryClientProvider client={client}>{ui}</QueryClientProvider>, ) return { ...result, rerender: (rerenderUi: React.ReactElement) => rerender( <QueryClientProvider client={client}>{rerenderUi}</QueryClientProvider>, ), - } as any + } }
59-72: Addconfigurable: trueto prevent errors on repeated calls.If
setIsServeris called multiple times without restoring first, the secondObject.definePropertycall will fail because the property is not configurable by default.export function setIsServer(isServer: boolean) { const original = utils.isServer Object.defineProperty(utils, 'isServer', { get: () => isServer, + configurable: true, }) return () => { Object.defineProperty(utils, 'isServer', { get: () => original, + configurable: true, }) } }packages/preact-query/src/__tests__/useQueries.test.tsx (1)
40-41: SharedqueryClientacross tests may cause test pollution.The
queryCacheandqueryClientare created once at module scope and shared across all tests. While some tests create their own client, others reuse this shared instance. Consider clearing the cache inafterEachto ensure test isolation:afterEach(() => { vi.useRealTimers() + queryCache.clear() })Alternatively, create a fresh client per test using a
beforeEachhook.packages/preact-query/src/useMutationState.ts (2)
14-16: Acknowledge the TODO for bundle optimization.The TODO comment correctly identifies that
preact/compatadds overhead. Consider tracking this as a follow-up issue to implement a lighter-weight subscription mechanism using Preact's native signals or a custom store abstraction.Would you like me to open an issue to track this optimization opportunity?
52-55: Consider initializingresultref directly to avoid non-null assertion.The ref is typed as potentially
nullbut immediately initialized. A cleaner pattern would avoid the assertion:- const result = useRef<Array<TResult>>(null) - if (result.current === null) { - result.current = getResult(mutationCache, options) - } + const result = useRef<Array<TResult>>(getResult(mutationCache, options))This eliminates the need for the
!assertion on line 77 and makes the initialization intent clearer.Also applies to: 76-77
packages/preact-query/src/useQueries.ts (1)
249-252: Side effects during render may cause issues with concurrent rendering.Calling
ensureSuspenseTimersandensurePreventErrorBoundaryRetrydirectly during render (outside of hooks) can trigger unexpected behavior in concurrent mode or strict mode, where the render phase may execute multiple times. These mutations should be applied within theuseMemocallback wheredefaultedQueriesis computed, or the options should be cloned before mutation.Consider moving these calls inside the
useMemo:const defaultedQueries = useMemo( () => queries.map((opts) => { const defaultedOptions = client.defaultQueryOptions( opts as QueryObserverOptions, ) // Make sure the results are already in fetching state before subscribing or updating options defaultedOptions._optimisticResults = isRestoring ? 'isRestoring' : 'optimistic' + ensureSuspenseTimers(defaultedOptions) + ensurePreventErrorBoundaryRetry(defaultedOptions, errorResetBoundary) + return defaultedOptions }), - [queries, client, isRestoring], + [queries, client, isRestoring, errorResetBoundary], ) -defaultedQueries.forEach((query) => { - ensureSuspenseTimers(query) - ensurePreventErrorBoundaryRetry(query, errorResetBoundary) -})packages/preact-query/src/useSuspenseQueries.ts (1)
165-187: Redundant function overloads.The two overload signatures (lines 165-176 and 178-187) appear to be identical in their
queriestype definition. Consider consolidating into a single overload to reduce complexity.export function useSuspenseQueries< T extends Array<any>, TCombinedResult = SuspenseQueriesResults<T>, >( options: { queries: | readonly [...SuspenseQueriesOptions<T>] | readonly [...{ [K in keyof T]: GetUseSuspenseQueryOptions<T[K]> }] combine?: (result: SuspenseQueriesResults<T>) => TCombinedResult }, queryClient?: QueryClient, ): TCombinedResult - -export function useSuspenseQueries< - T extends Array<any>, - TCombinedResult = SuspenseQueriesResults<T>, ->( - options: { - queries: readonly [...SuspenseQueriesOptions<T>] - combine?: (result: SuspenseQueriesResults<T>) => TCombinedResult - }, - queryClient?: QueryClient, -): TCombinedResult
| "dependencies": { | ||
| "preact": "^10.26.9" | ||
| }, |
There was a problem hiding this comment.
Missing @tanstack/preact-query dependency.
This example is meant to demonstrate the Preact Query adapter, but the package doesn't include @tanstack/preact-query in its dependencies. Based on similar examples in the TanStack Query monorepo, workspace dependencies should be added.
"dependencies": {
- "preact": "^10.26.9"
+ "preact": "^10.26.9",
+ "@tanstack/preact-query": "workspace:*"
},π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "dependencies": { | |
| "preact": "^10.26.9" | |
| }, | |
| "dependencies": { | |
| "preact": "^10.26.9", | |
| "@tanstack/preact-query": "workspace:*" | |
| }, |
π€ Prompt for AI Agents
In examples/preact/simple/package.json around lines 9 to 11, the package.json is
missing the @tanstack/preact-query dependency required for the Preact Query
adapter example; add @tanstack/preact-query (use a workspace or matching version
as other examples in the monorepo use) to the "dependencies" section so the
example installs and runs correctly, ensuring version aligns with the monorepo's
TanStack Query packages.
| // @ts-expect-error wtf | ||
| ...reactHooks.configs['recommended-latest'], |
There was a problem hiding this comment.
π οΈ Refactor suggestion | π Major
Replace unprofessional comment with a descriptive explanation.
The @ts-expect-error wtf comment should be replaced with a meaningful explanation of why the type error occurs and is expected.
- // @ts-expect-error wtf
+ // @ts-expect-error types are not correctly exported from eslint-plugin-react-hooks
...reactHooks.configs['recommended-latest'],π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // @ts-expect-error wtf | |
| ...reactHooks.configs['recommended-latest'], | |
| // @ts-expect-error types are not correctly exported from eslint-plugin-react-hooks | |
| ...reactHooks.configs['recommended-latest'], |
π€ Prompt for AI Agents
In packages/preact-query/eslint.config.js around lines 11 to 12, replace the
unprofessional comment "// @ts-expect-error wtf" with a concise, descriptive
explanation that documents why a TypeScript type error is expected (for example:
mismatch between ESLint config types and reactHooks.configs shape, or upstream
type definitions incompatible with this project's TS version), keep the
@ts-expect-error directive if needed, and include the ticket/issue reference or
link and the conditions under which it can be removed so future maintainers
understand the rationale.
| import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest' | ||
| import * as React from 'react' | ||
| import { render } from '@testing-library/react' | ||
| import * as coreModule from '@tanstack/query-core' | ||
| import { sleep } from '@tanstack/query-test-utils' | ||
| import { | ||
| HydrationBoundary, | ||
| QueryClient, | ||
| QueryClientProvider, | ||
| dehydrate, | ||
| useQuery, | ||
| } from '..' | ||
| import type { hydrate } from '@tanstack/query-core' |
There was a problem hiding this comment.
Imports should use Preact instead of React.
Same issue as other test files - this Preact adapter package imports from react and @testing-library/react.
import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'
-import * as React from 'react'
-import { render } from '@testing-library/react'
+import { render } from '@testing-library/preact'
import * as coreModule from '@tanstack/query-core'
import { sleep } from '@tanstack/query-test-utils'Committable suggestion skipped: line range outside the PR's diff.
| @@ -0,0 +1,165 @@ | |||
| import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest' | |||
| import { render } from '@testing-library/react' | |||
There was a problem hiding this comment.
Use Preact testing utilities instead of React.
This file imports render from '@testing-library/react', but this is a Preact package. Tests should use '@testing-library/preact' instead.
Apply this diff:
-import { render } from '@testing-library/react'
+import { render } from '@testing-library/preact'π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { render } from '@testing-library/react' | |
| import { render } from '@testing-library/preact' |
π€ Prompt for AI Agents
In packages/preact-query/src/__tests__/QueryClientProvider.test.tsx around line
2, the test imports render from '@testing-library/react' but this is a Preact
test file; replace that import to use '@testing-library/preact' instead. Update
the import statement to import render (and any other testing utilities used)
from '@testing-library/preact' so the tests run with Preact's testing utilities
and adjust any named imports if necessary.
| import * as React from 'react' | ||
| import { renderToString } from 'react-dom/server' | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | ||
| import { queryKey, sleep } from '@tanstack/query-test-utils' | ||
| import { | ||
| QueryCache, | ||
| QueryClient, | ||
| QueryClientProvider, | ||
| useInfiniteQuery, | ||
| useQuery, | ||
| } from '..' | ||
| import { setIsServer } from './utils' |
There was a problem hiding this comment.
Imports should use Preact instead of React for SSR testing.
This SSR test imports from react and react-dom/server, but this is a Preact adapter package. Use Preact equivalents.
-import * as React from 'react'
-import { renderToString } from 'react-dom/server'
+import { renderToString } from 'preact-render-to-string'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import * as React from 'react' | |
| import { renderToString } from 'react-dom/server' | |
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | |
| import { queryKey, sleep } from '@tanstack/query-test-utils' | |
| import { | |
| QueryCache, | |
| QueryClient, | |
| QueryClientProvider, | |
| useInfiniteQuery, | |
| useQuery, | |
| } from '..' | |
| import { setIsServer } from './utils' | |
| import { renderToString } from 'preact-render-to-string' | |
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | |
| import { queryKey, sleep } from '@tanstack/query-test-utils' | |
| import { | |
| QueryCache, | |
| QueryClient, | |
| QueryClientProvider, | |
| useInfiniteQuery, | |
| useQuery, | |
| } from '..' | |
| import { setIsServer } from './utils' |
π€ Prompt for AI Agents
In packages/preact-query/src/__tests__/ssr.test.tsx around lines 1 to 12, the
test imports React and react-dom/server for SSR; replace those with Preact
equivalents (import from 'preact' instead of 'react' and use a Preact server
renderer such as 'preact-render-to-string' instead of 'react-dom/server'),
update any React-specific imports/usages to the matching Preact APIs (e.g.,
default export/component types if needed), and run the tests to ensure the
Preact renderer is used throughout the file.
| import { fireEvent, render } from '@testing-library/react' | ||
| import * as React from 'react' |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
#!/bin/bash
# Check what testing libraries other preact-query test files use
echo "=== Testing libraries imported in preact-query test files ==="
fd -e 'test.tsx' -e 'test.ts' packages/preact-query/src/__tests__/ | head -10 | while read file; do
echo "File: $file"
rg "from ['\"@testing-library" "$file" | head -3
echo ""
done
# Check if @testing-library/preact is a dependency
echo "=== Testing-library related dependencies in packages/preact-query/package.json ==="
cat packages/preact-query/package.json | jq '.devDependencies | to_entries[] | select(.key | contains("testing-library")) | {key: .key, value: .value}'
# Check if preact is imported in tests
echo "=== Imports from 'preact' vs 'react' in preact-query test files ==="
rg "from ['\"]preact" packages/preact-query/src/__tests__/ | head -5
rg "from ['\"]react['\"]" packages/preact-query/src/__tests__/ | head -5Repository: TanStack/query
Length of output: 1324
π Script executed:
#!/bin/bash
# Check which testing-library is imported in preact-query test files
echo "=== @testing-library imports in preact-query test files ==="
rg "from ['\"@testing-library" packages/preact-query/src/__tests__/ | head -10
# Also check if there are any preact imports at all in test files
echo ""
echo "=== All 'from preact' imports in preact-query test files ==="
rg "from ['\"]preact" packages/preact-query/src/__tests__/ | wc -l
rg "from ['\"]preact" packages/preact-query/src/__tests__/
# Check the useQueries.test.tsx file specifically
echo ""
echo "=== Imports in useQueries.test.tsx (first 20 lines) ==="
head -20 packages/preact-query/src/__tests__/useQueries.test.tsxRepository: TanStack/query
Length of output: 745
π Script executed:
#!/bin/bash
# Check all dependencies and devDependencies
echo "=== preact-query dependencies ==="
cat packages/preact-query/package.json | jq '.dependencies, .devDependencies' | head -40
# Check if react-error-boundary is used elsewhere
echo ""
echo "=== react-error-boundary usage in preact-query tests ==="
rg "react-error-boundary" packages/preact-query/src/__tests__/Repository: TanStack/query
Length of output: 1517
Preact adapter tests import React instead of Preact.
The test file imports from @testing-library/react and react instead of @testing-library/preact and preact. This pattern affects multiple test files in the preact-query package (useQueries.test.tsx, useSuspenseQuery.test.tsx, useQuery.test.tsx, and others). Since @testing-library/preact is available as a devDependency, the tests should use Preact-native utilities to properly validate the adapter's behavior with Preact's actual runtime.
π€ Prompt for AI Agents
In packages/preact-query/src/__tests__/useQueries.test.tsx around lines 10-11,
the test imports from @testing-library/react and react; replace those with
@testing-library/preact and preact respectively (i.e., import render/fireEvent
from @testing-library/preact and import h/Component or relevant APIs from
'preact') so the tests run against the Preact runtime; apply the same import
fixes to the other affected files (useSuspenseQuery.test.tsx, useQuery.test.tsx,
etc.), update any test utilities that rely on React-specific APIs to their
Preact equivalents, and re-run the test suite to verify everything passes.
| export type DefinedInitialDataInfiniteOptions< | ||
| TQueryFnData, | ||
| TError = DefaultError, | ||
| TData = InfiniteData<TQueryFnData>, | ||
| TQueryKey extends QueryKey = QueryKey, | ||
| TPageParam = unknown, | ||
| > = UseInfiniteQueryOptions< | ||
| TQueryFnData, | ||
| TError, | ||
| TData, | ||
| TQueryKey, | ||
| TPageParam | ||
| > & { | ||
| initialData: | ||
| | NonUndefinedGuard<InfiniteData<TQueryFnData, TPageParam>> | ||
| | (() => NonUndefinedGuard<InfiniteData<TQueryFnData, TPageParam>>) | ||
| | undefined | ||
| } |
There was a problem hiding this comment.
DefinedInitialDataInfiniteOptions should not allow undefined in initialData.
The type name suggests that initialData is always defined, but line 72 includes | undefined in the union, which contradicts the intent. Compare with the non-infinite DefinedInitialDataOptions in queryOptions.ts (lines 46-48) which correctly omits undefined.
export type DefinedInitialDataInfiniteOptions<
TQueryFnData,
TError = DefaultError,
TData = InfiniteData<TQueryFnData>,
TQueryKey extends QueryKey = QueryKey,
TPageParam = unknown,
> = UseInfiniteQueryOptions<
TQueryFnData,
TError,
TData,
TQueryKey,
TPageParam
> & {
initialData:
| NonUndefinedGuard<InfiniteData<TQueryFnData, TPageParam>>
| (() => NonUndefinedGuard<InfiniteData<TQueryFnData, TPageParam>>)
- | undefined
}π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type DefinedInitialDataInfiniteOptions< | |
| TQueryFnData, | |
| TError = DefaultError, | |
| TData = InfiniteData<TQueryFnData>, | |
| TQueryKey extends QueryKey = QueryKey, | |
| TPageParam = unknown, | |
| > = UseInfiniteQueryOptions< | |
| TQueryFnData, | |
| TError, | |
| TData, | |
| TQueryKey, | |
| TPageParam | |
| > & { | |
| initialData: | |
| | NonUndefinedGuard<InfiniteData<TQueryFnData, TPageParam>> | |
| | (() => NonUndefinedGuard<InfiniteData<TQueryFnData, TPageParam>>) | |
| | undefined | |
| } | |
| export type DefinedInitialDataInfiniteOptions< | |
| TQueryFnData, | |
| TError = DefaultError, | |
| TData = InfiniteData<TQueryFnData>, | |
| TQueryKey extends QueryKey = QueryKey, | |
| TPageParam = unknown, | |
| > = UseInfiniteQueryOptions< | |
| TQueryFnData, | |
| TError, | |
| TData, | |
| TQueryKey, | |
| TPageParam | |
| > & { | |
| initialData: | |
| | NonUndefinedGuard<InfiniteData<TQueryFnData, TPageParam>> | |
| | (() => NonUndefinedGuard<InfiniteData<TQueryFnData, TPageParam>>) | |
| } |
π€ Prompt for AI Agents
In packages/preact-query/src/infiniteQueryOptions.ts around lines 56 to 73, the
DefinedInitialDataInfiniteOptions type currently allows initialData to be
undefined, which contradicts the type name; remove the trailing " | undefined"
from the initialData union so initialData is strictly
NonUndefinedGuard<InfiniteData<...>> or a function returning that type, matching
the non-infinite DefinedInitialDataOptions pattern.
| useSyncExternalStore( | ||
| useCallback( | ||
| (onStoreChange) => { | ||
| const unsubscribe = shouldSubscribe | ||
| ? observer.subscribe(notifyManager.batchCalls(onStoreChange)) | ||
| : noop | ||
|
|
||
| // Update result to make sure we did not miss any query updates | ||
| // between creating the observer and subscribing to it. | ||
| observer.updateResult() | ||
|
|
||
| return unsubscribe | ||
| }, | ||
| [observer, shouldSubscribe], | ||
| ), | ||
| () => observer.getCurrentResult(), | ||
| ) |
There was a problem hiding this comment.
Missing getServerSnapshot parameter may cause SSR hydration issues.
useSyncExternalStore requires a third argument (getServerSnapshot) for proper SSR support. Without it, server-side rendering will either fail or produce hydration mismatches when the server and client snapshots differ.
Apply this diff to add SSR support:
useSyncExternalStore(
useCallback(
(onStoreChange) => {
const unsubscribe = shouldSubscribe
? observer.subscribe(notifyManager.batchCalls(onStoreChange))
: noop
// Update result to make sure we did not miss any query updates
// between creating the observer and subscribing to it.
observer.updateResult()
return unsubscribe
},
[observer, shouldSubscribe],
),
() => observer.getCurrentResult(),
+ () => observer.getCurrentResult(),
)Alternatively, if SSR behavior should differ (e.g., return optimistic result), adjust accordingly.
π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useSyncExternalStore( | |
| useCallback( | |
| (onStoreChange) => { | |
| const unsubscribe = shouldSubscribe | |
| ? observer.subscribe(notifyManager.batchCalls(onStoreChange)) | |
| : noop | |
| // Update result to make sure we did not miss any query updates | |
| // between creating the observer and subscribing to it. | |
| observer.updateResult() | |
| return unsubscribe | |
| }, | |
| [observer, shouldSubscribe], | |
| ), | |
| () => observer.getCurrentResult(), | |
| ) | |
| useSyncExternalStore( | |
| useCallback( | |
| (onStoreChange) => { | |
| const unsubscribe = shouldSubscribe | |
| ? observer.subscribe(notifyManager.batchCalls(onStoreChange)) | |
| : noop | |
| // Update result to make sure we did not miss any query updates | |
| // between creating the observer and subscribing to it. | |
| observer.updateResult() | |
| return unsubscribe | |
| }, | |
| [observer, shouldSubscribe], | |
| ), | |
| () => observer.getCurrentResult(), | |
| () => observer.getCurrentResult(), | |
| ) |
π€ Prompt for AI Agents
In packages/preact-query/src/useBaseQuery.ts around lines 100β116,
useSyncExternalStore is called without the third getServerSnapshot argument
which can cause SSR/hydration mismatches; fix by passing a getServerSnapshot
function appropriate for SSR (for example, a function that returns
observer.getCurrentResult() or the observer's optimistic/server snapshot method
if available) as the third parameter so the server snapshot is stable during
hydration; ensure the getServerSnapshot is a no-arg function (not invoked) and
aligns with the client snapshot behavior used in the second argument.
| const result = useSyncExternalStore( | ||
| useCallback( | ||
| (onStoreChange) => | ||
| observer.subscribe(notifyManager.batchCalls(onStoreChange)), | ||
| [observer], | ||
| ), | ||
| () => observer.getCurrentResult(), | ||
| ) |
There was a problem hiding this comment.
Missing getServerSnapshot argument for SSR support.
useSyncExternalStore requires a third argument (getServerSnapshot) for SSR compatibility. Without it, server-side rendering will fail or produce inconsistent results.
const result = useSyncExternalStore(
useCallback(
(onStoreChange) =>
observer.subscribe(notifyManager.batchCalls(onStoreChange)),
[observer],
),
() => observer.getCurrentResult(),
+ () => observer.getCurrentResult(),
)π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = useSyncExternalStore( | |
| useCallback( | |
| (onStoreChange) => | |
| observer.subscribe(notifyManager.batchCalls(onStoreChange)), | |
| [observer], | |
| ), | |
| () => observer.getCurrentResult(), | |
| ) | |
| const result = useSyncExternalStore( | |
| useCallback( | |
| (onStoreChange) => | |
| observer.subscribe(notifyManager.batchCalls(onStoreChange)), | |
| [observer], | |
| ), | |
| () => observer.getCurrentResult(), | |
| () => observer.getCurrentResult(), | |
| ) |
π€ Prompt for AI Agents
In packages/preact-query/src/useMutation.ts around lines 46 to 53, the
useSyncExternalStore call is missing the third getServerSnapshot argument
required for SSR compatibility; add a third parameter that returns the current
observer result (for example, a function that calls observer.getCurrentResult())
so server renders use the same snapshot as the client β ensure the function
reads the observer synchronously (e.g., () => observer.getCurrentResult()) to
provide a stable server snapshot.
| useSyncExternalStore( | ||
| useCallback( | ||
| (onStoreChange) => | ||
| shouldSubscribe | ||
| ? observer.subscribe(notifyManager.batchCalls(onStoreChange)) | ||
| : noop, | ||
| [observer, shouldSubscribe], | ||
| ), | ||
| () => observer.getCurrentResult(), | ||
| ) |
There was a problem hiding this comment.
Missing getServerSnapshot for SSR compatibility.
The useSyncExternalStore hook is called without the third argument (getServerSnapshot), which is required for proper SSR/hydration support. Without it, server rendering may fail or produce hydration mismatches.
useSyncExternalStore(
useCallback(
(onStoreChange) =>
shouldSubscribe
? observer.subscribe(notifyManager.batchCalls(onStoreChange))
: noop,
[observer, shouldSubscribe],
),
() => observer.getCurrentResult(),
+ () => observer.getCurrentResult(),
)π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useSyncExternalStore( | |
| useCallback( | |
| (onStoreChange) => | |
| shouldSubscribe | |
| ? observer.subscribe(notifyManager.batchCalls(onStoreChange)) | |
| : noop, | |
| [observer, shouldSubscribe], | |
| ), | |
| () => observer.getCurrentResult(), | |
| ) | |
| useSyncExternalStore( | |
| useCallback( | |
| (onStoreChange) => | |
| shouldSubscribe | |
| ? observer.subscribe(notifyManager.batchCalls(onStoreChange)) | |
| : noop, | |
| [observer, shouldSubscribe], | |
| ), | |
| () => observer.getCurrentResult(), | |
| () => observer.getCurrentResult(), | |
| ) |
π€ Prompt for AI Agents
In packages/preact-query/src/useQueries.ts around lines 273 to 282,
useSyncExternalStore is being called without the required third argument for
server snapshots; add a getServerSnapshot function (e.g., a function that
returns observer.getCurrentResult() or an appropriate server-side result) as the
third parameter to ensure SSR/hydration compatibility, passing the same logic
used in the client snapshot but returning a stable server value or undefined
when server data isnβt available.
| @@ -0,0 +1,76 @@ | |||
| 'use client' | |||
There was a problem hiding this comment.
FWIW, no need for these as they do nothing in Preact. None of our tooling acknowledges these directives.
There was a problem hiding this comment.
It's certainly not a problem, no need for an apology. Just wanted to drop mention in case you weren't aware is all.
| 'mutations' | ||
| > | ||
| } | ||
| children?: React.ReactNode |
There was a problem hiding this comment.
You'd want ComponentChildren from preact
There was a problem hiding this comment.
Actionable comments posted: 0
π§Ή Nitpick comments (1)
packages/preact-query/src/HydrationBoundary.tsx (1)
95-96: Update the ESLint disable comment for Preact context.The comment references
react-hooks/refs, but this is a Preact project. Consider updating the comment to reference Preact's hooks rules or provide a clearer explanation of why the side effect inuseMemois intentional.- // eslint-disable-next-line react-hooks/refs + // Intentionally calling hydrate() during render for SSR and transition safety. + // This ensures new queries are available before children render. hydrate(client, { queries: newQueries }, optionsRef.current)
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
packages/preact-query/src/HydrationBoundary.tsx(1 hunks)
π§° Additional context used
π§ Learnings (1)
π Common learnings
Learnt from: TkDodo
Repo: TanStack/query PR: 9612
File: packages/query-async-storage-persister/src/asyncThrottle.ts:0-0
Timestamp: 2025-09-02T17:57:33.184Z
Learning: When importing from tanstack/query-core in other TanStack Query packages like query-async-storage-persister, a workspace dependency "tanstack/query-core": "workspace:*" needs to be added to the package.json.
𧬠Code graph analysis (1)
packages/preact-query/src/HydrationBoundary.tsx (3)
packages/preact-query/src/index.ts (3)
HydrationBoundaryProps(40-40)HydrationBoundary(39-39)useQueryClient(35-35)packages/query-core/src/types.ts (1)
OmitKeyof(19-29)packages/preact-query/src/QueryClientProvider.tsx (1)
useQueryClient(11-23)
π Additional comments (4)
packages/preact-query/src/HydrationBoundary.tsx (4)
14-24: LGTM!The interface is well-structured with appropriate type constraints. The exclusion of
'mutations'fromdefaultOptionsappears intentional for hydration-specific configuration.
26-37: LGTM!The ref-based options tracking correctly avoids triggering re-hydration when options change, while ensuring the latest options are available during hydration callbacks.
105-109: LGTM!The deferred hydration effect correctly handles existing queries post-render, preventing premature updates during transitions as explained in the component's documentation.
111-111: LGTM!The explicit type cast ensures type safety for the return value, handling the optional
childrenprop appropriately.
|
Just wanted to note my appreciation for this, as a member of a team using Tanstack Query with preact π«Ά |
thank you πβ€οΈ |
3 participants
π― Changes
Makes a Preact Adapter for Tanstack Query. The main motivations are bundle sizes and ensuring compatibility with the events architecture.
β Checklist
pnpm run test:pr.π Release Impact
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
βοΈ Tip: You can customize this high-level summary in your review settings.