Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces type-safety improvements and test coverage for the mobx-tanstack-query library, including a bug fix for initial data handling in preset style ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Ticket-ID: 66
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/preset/create-query.ts (1)
262-284:⚠️ Potential issue | 🟡 MinorFix incorrect reference to onInit callback parameter
The
onInitproperty belongs to the options object (args[1]), not the query function (args[0]). Line 270 should beargs[1]?.onInit?.(query)instead ofargs[0]?.onInit?.(query).🐛 Fix
onInit: (query) => { queryClient.mount(); - args[0]?.onInit?.(query); + args[1]?.onInit?.(query); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/preset/create-query.ts` around lines 262 - 284, The onInit callback is being invoked on the wrong argument in createQuery: change the invocation from args[0]?.onInit?.(query) to args[1]?.onInit?.(query) so the options object's onInit is called; update the onInit handler inside the function branch that constructs new Query with queryFn to call args[1]?.onInit?.(query) (keep the existing queryClient.mount() call and other properties intact).
🧹 Nitpick comments (2)
src/infinite-query.test.ts (1)
487-493: Document theanyworkaround for refetchInterval.The explicit
query: anyannotation suggests a type inference limitation. Consider adding a brief comment explaining why this workaround is needed (e.g., upstream@tanstack/query-coretyping issue) to help future maintainers understand this is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infinite-query.test.ts` around lines 487 - 493, Add a brief inline comment above the refetchInterval handler explaining why the parameter is typed as any: note that this is an intentional workaround for a known upstream typing limitation in `@tanstack/query-core` (link or issue number optional), so future maintainers know the cast is deliberate; update the refetchInterval function signature comment near the query parameter to mention the typing gap and that it should be removed once upstream fixes their types.src/query.test.ts (1)
3954-3956: Consider addingqueryKeyfor completeness.While valid for type-checking purposes, this test case omits
queryKey. If the test file is ever executed at runtime (not just type-checked), this query would lack an essential property.Suggested enhancement
createQuery(() => 1, { initialData: 2, + queryKey: ['type-test-66'], });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/query.test.ts` around lines 3954 - 3956, The test call to createQuery(() => 1, { initialData: 2 }) omits the required runtime property queryKey; update the createQuery invocation to include a unique queryKey (e.g., queryKey: ['test', 'simple'] or similar) alongside initialData so the query has the essential identifier at runtime, ensuring createQuery and its options (queryKey, initialData) are provided consistently in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/preset/create-query.test.ts`:
- Around line 8-93: The test "preserves typings for overloads" creates live
queries that run at runtime; wrap the whole body of that it(...) callback in a
compile-only block (e.g. if (false) { ... }) so TypeScript still verifies types
but nothing executes, and apply the same change to the similar block at lines
128-139; locate usages of createQuery and new QueryClient() inside that test and
leave the existing `@ts-expect-error` comments intact so the type assertions
remain checked but no queries are created or run at runtime.
---
Outside diff comments:
In `@src/preset/create-query.ts`:
- Around line 262-284: The onInit callback is being invoked on the wrong
argument in createQuery: change the invocation from args[0]?.onInit?.(query) to
args[1]?.onInit?.(query) so the options object's onInit is called; update the
onInit handler inside the function branch that constructs new Query with queryFn
to call args[1]?.onInit?.(query) (keep the existing queryClient.mount() call and
other properties intact).
---
Nitpick comments:
In `@src/infinite-query.test.ts`:
- Around line 487-493: Add a brief inline comment above the refetchInterval
handler explaining why the parameter is typed as any: note that this is an
intentional workaround for a known upstream typing limitation in
`@tanstack/query-core` (link or issue number optional), so future maintainers know
the cast is deliberate; update the refetchInterval function signature comment
near the query parameter to mention the typing gap and that it should be removed
once upstream fixes their types.
In `@src/query.test.ts`:
- Around line 3954-3956: The test call to createQuery(() => 1, { initialData: 2
}) omits the required runtime property queryKey; update the createQuery
invocation to include a unique queryKey (e.g., queryKey: ['test', 'simple'] or
similar) alongside initialData so the query has the essential identifier at
runtime, ensuring createQuery and its options (queryKey, initialData) are
provided consistently in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a08a22f7-ef36-4b6f-8b88-9b4daf3cf9cb
📒 Files selected for processing (9)
.changeset/tame-stars-exist.mddocs/introduction/getting-started.mdpackage.jsonsrc/infinite-query.test.tssrc/mutation.test.tssrc/preset/create-query.test.tssrc/preset/create-query.tssrc/query.test.tstsconfig.test.json
Comment on lines
+8
to
+93
| it('preserves typings for overloads', () => { | ||
| type User = { | ||
| id: string; | ||
| email: string; | ||
| }; | ||
|
|
||
| const queryFnReturnsNumber: () => number = () => 1; | ||
| const getUser = async () => ({ id: '1', email: 'alice@example.com' }); | ||
|
|
||
| const queryFromOptions = createQuery({ | ||
| enabled: false, | ||
| queryKey: ['user'] as const, | ||
| queryFn: async () => ({ id: '1', email: 'alice@example.com' }), | ||
| }); | ||
|
|
||
| expectTypeOf(queryFromOptions.data).toEqualTypeOf<User | undefined>(); | ||
|
|
||
| const queryFromFnOnly = createQuery(async () => 1); | ||
|
|
||
| expectTypeOf(queryFromFnOnly.data).toEqualTypeOf<number | undefined>(); | ||
|
|
||
| const queryFromFn = createQuery( | ||
| async () => ({ id: '1', email: 'alice@example.com' }), | ||
| { | ||
| enabled: false, | ||
| queryKey: ['user'] as const, | ||
| initialData: { id: '0', email: 'init@example.com' }, | ||
| }, | ||
| ); | ||
|
|
||
| expectTypeOf(queryFromFn.data).toEqualTypeOf<User | undefined>(); | ||
|
|
||
| const queryFromFnWithSelect = createQuery(getUser, { | ||
| enabled: false, | ||
| queryKey: ['user'] as const, | ||
| initialData: { id: '0', email: 'init@example.com' }, | ||
| select: ((user) => user.email) satisfies ( | ||
| user: Awaited<ReturnType<typeof getUser>>, | ||
| ) => string, | ||
| }); | ||
|
|
||
| expectTypeOf(queryFromFnWithSelect.data).toEqualTypeOf< | ||
| string | undefined | ||
| >(); | ||
|
|
||
| const queryFromOptionsWithExplicitGenerics = createQuery< | ||
| number, | ||
| Error, | ||
| string | ||
| >({ | ||
| enabled: false, | ||
| queryKey: ['count'] as const, | ||
| queryFn: async () => 1, | ||
| select: ((count) => count.toString()) satisfies (count: number) => string, | ||
| }); | ||
|
|
||
| expectTypeOf(queryFromOptionsWithExplicitGenerics.data).toEqualTypeOf< | ||
| string | undefined | ||
| >(); | ||
|
|
||
| const queryFromClientAndOptions = createQuery(new QueryClient(), () => ({ | ||
| enabled: false, | ||
| queryKey: ['dynamic'] as const, | ||
| queryFn: async () => 1, | ||
| select: ((count) => count.toString()) satisfies (count: number) => string, | ||
| })); | ||
|
|
||
| expectTypeOf(queryFromClientAndOptions.data).toEqualTypeOf< | ||
| string | undefined | ||
| >(); | ||
|
|
||
| createQuery(queryFnReturnsNumber, { | ||
| initialData: 2, | ||
| }); | ||
|
|
||
| // @ts-expect-error initialData must match queryFn return type | ||
| createQuery(queryFnReturnsNumber, { | ||
| initialData: '2', | ||
| }); | ||
|
|
||
| // @ts-expect-error initialData still uses queryFn data, not selected data | ||
| createQuery<User, Error, string, User, any[]>(getUser, { | ||
| initialData: 'alice@example.com', | ||
| select: (user) => user.email, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Don’t execute these type-coverage query creations as normal runtime tests.
These blocks instantiate several real queries, including intentionally mismatched initialData cases, but never dispose them. A few calls also leave enabled at its default, so they can fetch/update state and bleed into later tests. Please move this coverage to a non-executing type test, or skip these blocks; otherwise every query created here should be disabled and cleaned up.
💡 Minimal fix
- it('preserves typings for overloads', () => {
+ it.skip('preserves typings for overloads', () => {
// ...
});
- it('rejects mismatched initialData for queryFn overload', () => {
+ it.skip('rejects mismatched initialData for queryFn overload', () => {
// ...
});Also applies to: 128-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/preset/create-query.test.ts` around lines 8 - 93, The test "preserves
typings for overloads" creates live queries that run at runtime; wrap the whole
body of that it(...) callback in a compile-only block (e.g. if (false) { ... })
so TypeScript still verifies types but nothing executes, and apply the same
change to the similar block at lines 128-139; locate usages of createQuery and
new QueryClient() inside that test and leave the existing `@ts-expect-error`
comments intact so the type assertions remain checked but no queries are created
or run at runtime.
Merged
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.
Summary by CodeRabbit
Bug Fixes
initialData's type doesn't match the return type ofqueryFn#66).New Features
Documentation