Uh oh!
There was an error while loading. Please reload this page.
feat(vue-query): Add mutationOptions - #10036
Conversation
🦋 Changeset detectedLatest commit: 4f516be The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds getter-aware typing utilities and a new type-level helper Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
View your CI Pipeline Execution ↗ for commit 6abcbfb
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/vue-query/src/__tests__/mutationOptions.test.ts`:
- Around line 1-11: The test imports onScopeDispose from vue-demi but never
mocks it, so casting it to MockedFunction and calling mockImplementation will
throw; add a vi.mock for 'vue-demi' that provides onScopeDispose as a
jest/vitest mock function (e.g., onScopeDispose: vi.fn()) and ensure the mock is
registered before the test uses the imported onScopeDispose (place the vi.mock
call at the top of the test file or adjust imports so the mock is applied prior
to importing onScopeDispose) so the test can call mockImplementation on
onScopeDispose safely.
🧹 Nitpick comments (2)
packages/vue-query/src/__tests__/mutationOptions.test.ts (2)
262-263: Test descriptions are misleading.These tests verify mutation state data (e.g.,
mutationStateArray[0]?.data), not "the number of fetching mutations." Consider updating the descriptions to accurately reflect what's being tested, e.g., "should return mutation states when used with useMutationState."
379-407: Test doesn't exercisemutationOptionsfunctionality.This test uses
useMutationdirectly without callingmutationOptions. It appears to test the disposal behavior ofuseMutation/useIsMutating, which may be better suited inuseMutation.test.tsoruseMutationState.test.ts.If the intent is to test
mutationOptionsintegration with disposal, consider usingmutationOptionsin the test setup:- const mutation = useMutation({+ const mutation = useMutation(mutationOptions({ mutationFn: (params: string) => sleep(0).then(() => params), - })+ }))
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## main #10036 +/- ##
===========================================
- Coverage 45.84% 33.42% -12.43%
===========================================
Files 200 28 -172 Lines 8526 1071 -7455 Branches 1972 177 -1795 ===========================================
- Hits 3909 358 -3551 + Misses 4157 657 -3500 + Partials 460 56 -404
🚀 New features to boost your workflow:
|
Why doesn't test coverage exclude the test files themselves? 🤔 query/packages/vue-query/vite.config.ts Line 25 in ed5e5fc |
2210327 to
f05df98CompareUh oh!
There was an error while loading. Please reload this page.
| mutationFn: () => sleep(10).then(() => 5), | ||
| } as const | ||
| expect(mutationOptions(object)).toStrictEqual(object) |
There was a problem hiding this comment.
| expect(mutationOptions(object)).toStrictEqual(object) | |
| expect(mutationOptions(object)).toBe(object) |
There was a problem hiding this comment.
I believe mutationOptions is merely a type-check helper function; therefore, tests should guarantee the referential identity of object instances and ensure that the internal data remains unchanged.
Perhaps we should use both toBe and toStrictEqual.
| mutationFn: () => sleep(10).then(() => 5), | ||
| } as const | ||
| expect(mutationOptions(object)).toStrictEqual(object) |
There was a problem hiding this comment.
| expect(mutationOptions(object)).toStrictEqual(object) | |
| expect(mutationOptions(object)).toBe(object) |
TkDodo
commented
Mar 1, 2026
@DamianOsipiuk could you have a look here please 🙏 |
4f516be to
da4f339Compareshincurry
commented
Apr 19, 2026
6 |
🎯 Changes
mutationOptionsDeepUnwrapRefOrGetter,MaybeRefDeepOrGettermutationOptions(Generated by Claude Opus 4.5, reviewed by @shincurry)mutationOptions(Copied from react-query)✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
New Features
Documentation
Tests
Chores