Feat/compt 40 create query factory#2
Conversation
- Add createQuery(keyFn, fetcher) returning QueryDefinition<TParams, TData> - TData and TParams fully inferred from fetcher signature, zero manual annotation - queryKey returns stable readonly tuple via keyFn - useQuery shorthand hook wraps useTanstackQuery with typed params - Export from src/index.ts - Add @tanstack/react-query as peerDependency (>=5) and devDependency - Add pnpm cssstyle override to fix Node.js v22 + jsdom@28 ESM compat issue - Fix duplicate import in vitest.config.ts - 9 tests, 100% coverage on createQuery.ts, 88.6% overall Closes COMPT-40
There was a problem hiding this comment.
Pull request overview
Adds a small “query definition” factory around TanStack React Query v5 and exposes it as part of the package’s public API.
Changes:
- Introduces
createQuery(queryKey/queryFn/useQuery) undersrc/queryand exports it fromsrc/index.ts. - Adds unit tests for
createQueryusing React Testing Library + TanStack Query’sQueryClientProvider. - Updates dependencies/lockfiles to include
@tanstack/react-queryand adds pnpm-specific configuration.
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Removes a duplicate defineConfig import. |
| src/query/index.ts | New barrel export for query utilities. |
| src/query/createQuery.ts | Implements the createQuery factory and related types. |
| src/query/createQuery.test.tsx | Adds tests covering the query definition shape and hook behavior. |
| src/index.ts | Exposes the new query module via the public API. |
| package.json | Adds React Query peer/dev deps; adds pnpm override + packageManager metadata. |
| package-lock.json | Updates lockfile to include React Query and package metadata changes. |
| type UseQueryShorthandOptions<TData> = Omit<UseQueryOptions<TData, Error>, 'queryKey' | 'queryFn'>; | ||
|
|
There was a problem hiding this comment.
UseQueryShorthandOptions binds the query function data type and the returned data type to the same TData (because UseQueryOptions<TData, Error> sets both TQueryFnData and TData). This prevents consumers from using options like select to transform the fetched data into a different output type. Consider introducing separate generics (e.g. TQueryFnData vs TData) or defining the options type as UseQueryOptions<TQueryFnData, Error, TData> so select can change the result type safely.
| it('useQuery hook shows loading state before data resolves', () => { | ||
| let resolve: (v: { value: number }) => void; | ||
| const fetcher = vi.fn( | ||
| () => | ||
| new Promise<{ value: number }>((res) => { | ||
| resolve = res; | ||
| }), | ||
| ); | ||
| const def = createQuery((n: number) => ['lazy', n] as const, fetcher); | ||
|
|
||
| const { result } = renderHook(() => def.useQuery(99), { | ||
| wrapper: makeWrapper(), | ||
| }); | ||
|
|
||
| expect(result.current.isLoading).toBe(true); | ||
| // Resolve to avoid open handles | ||
| resolve!({ value: 99 }); | ||
| }); |
There was a problem hiding this comment.
In the loading-state test, the promise is resolved but the test does not wait for React Query to process the resolution. This can lead to async updates happening after the test finishes (and potential act/open-handle warnings). After calling resolve, wait for the hook to reach a settled state (e.g. isSuccess) or explicitly unmount/clear the QueryClient.
| "access": "public" | ||
| }, | ||
| "peerDependencies": { | ||
| "@tanstack/react-query": ">=5", |
There was a problem hiding this comment.
The peer dependency range for @tanstack/react-query is >=5, which will also allow future major versions (e.g. v6) that may include breaking changes. Since this package targets TanStack Query v5, consider tightening the range (e.g. ^5 or >=5 <6) to avoid accidental incompatibilities for consumers.
| "@tanstack/react-query": ">=5", | |
| "@tanstack/react-query": "^5", |
package.json
Outdated
| "pnpm": { | ||
| "overrides": { | ||
| "cssstyle": "6.0.1" | ||
| } | ||
| }, |
There was a problem hiding this comment.
CI uses npm ci (see .github/workflows/pr-validation.yml), so the pnpm.overrides block will not affect dependency resolution in CI or for npm users. If cssstyle must be pinned, use npm's overrides field (or align CI/package manager to pnpm) so the lockfile and install behavior are consistent.
package.json
Outdated
| }, | ||
| "pnpm": { | ||
| "overrides": { | ||
| "cssstyle": "6.0.1" | ||
| } | ||
| }, | ||
| "packageManager": "pnpm@10.20.0+sha512.cf9998222162dd85864d0a8102e7892e7ba4ceadebbf5a31f9c2fce48dfce317a9c53b9f6464d1ef9042cba2e02ae02a9f7c143a2b438cd93c91840f0192b9dd" |
There was a problem hiding this comment.
packageManager is set to pnpm@..., but the repository’s CI pipeline installs with npm (npm ci). This mismatch can cause contributors to generate different lockfiles/resolutions locally vs CI. Consider switching workflows to pnpm (and relying on pnpm-lock.yaml), or removing packageManager/pnpm-specific config to keep npm as the source of truth.
| }, | |
| "pnpm": { | |
| "overrides": { | |
| "cssstyle": "6.0.1" | |
| } | |
| }, | |
| "packageManager": "pnpm@10.20.0+sha512.cf9998222162dd85864d0a8102e7892e7ba4ceadebbf5a31f9c2fce48dfce317a9c53b9f6464d1ef9042cba2e02ae02a9f7c143a2b438cd93c91840f0192b9dd" | |
| } |
| it('queryKey produces a stable reference for the same params', () => { | ||
| const def = createQuery( | ||
| (id: number) => ['item', id] as const, | ||
| async (id: number) => id, | ||
| ); | ||
|
|
||
| const first = def.queryKey(1); | ||
| const second = def.queryKey(1); | ||
| expect(first).toEqual(second); | ||
| }); |
There was a problem hiding this comment.
This test description says the key function returns a “stable reference”, but the assertion only uses toEqual (deep equality) and does not validate referential stability. Either change the test name/expectation to reflect deep equality, or change the assertion to check reference identity and update the implementation accordingly.
Summary
Why
Checklist
npm run lintpassesnpm run typecheckpassesnpm testpassesnpm run buildpassesnpx changeset) if this affects consumersNotes