-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix(svelte-query): state_unsafe_mutation
error with useIs...
#9493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(svelte-query): state_unsafe_mutation
error with useIs...
#9493
Conversation
dd04604
to
e22d61e
Compare
|
Command | Status | Duration | Result |
---|---|---|---|
nx affected --targets=test:sherif,test:knip,tes... |
❌ Failed | 1m 17s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 6s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-09-27 12:32:45
UTC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @hmnd , I don't seem to have permission to sync this with the changes in the svelte-5-adapter. Could you fix the merge conflicts?
4f57fd5
to
ab7e6b4
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests
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 |
ab7e6b4
to
4f57fd5
Compare
@lachlancollins should be fixed now :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it worked, it's still showing all the conflicts
Oh sorry, One sec |
…afe_mutation fixes useIsFetching and useIsMutating in svelte 5 adapter
4f57fd5
to
f8f22f7
Compare
a9c586a
to
83143f0
Compare
83143f0
to
ac2afcf
Compare
|
||
onDestroy(() => { | ||
unsubscribe() | ||
let result = $derived(observer.getCurrentResult()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lachlancollins do you think we need to keep the Svelte peer dep pinned to as low as 5.7.0
, or could we bump that to at least 5.25.0
so that assigning to $derived
like this is supported? This was part of eslint --fix
, but if we need to stay on the lower version, I'll have to revert this back to a manually managed $state
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to bump the peer dependency version!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hmnd I've updated this on the target branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this change(?) might have caused tests to start failing. It's difficult to know though, as the force pushes have erased the history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I found the diff in a branch I cloned earlier. Tests are all passing again, so I think I'll merge this! Thanks heaps for your time @hmnd !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for getting this merged @lachlancollins! Sorry about the force pushes 😅
Replace derived state with direct state and add watchChanges for result updates.
968d6a8
into
TanStack:svelte-5-adapter
* WIP: Svelte 5 adapter (#6981) * feat(svelte-query): Improve svelte runes API (#8852) * feat: Draft proposal * chore: Improve reactive containers * ci: apply automated fixes * oops * fix: Update API, add a bunch of tests * merge main * fix: use const * more tests * feat: More tests, back to thunks, fixed svelte-query-persist-client * feat: More tests and examples! * lockfile * fixes * Fix current CI errors * More small fixes/tweaks * Remove test.only * ci: apply automated fixes * Fix pnpm-lock, fix import order * update main docs * feat: More tests * ci: apply automated fixes * add back old tests * Cleanup * Fix persist client * Fix useMutationState --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Lachlan Collins <[email protected]> * Use sleep from query-test-utils * Simplify test reset logic * Fix some merge conflicts * More fixes * A few more fixes * Fix useMutationState * Add changeset * Add migration docs * Replace Set with SvelteSet * Update minimum svelte version * Bump svelte-eslint-parser * Unwrap createQuery test * fix(svelte-query): `state_unsafe_mutation` error with `useIs...` (#9493) * fix(svelte-query): don't wrap observers in derived to avoid state_unsafe_mutation fixes useIsFetching and useIsMutating in svelte 5 adapter * test(svelte-query): wrap (useIs...) tests in QueryClientProvider to test non colocated query * fix(svelte-query): update observers when passed in query client changes * fix(svelte-query): simplify creatMutation sub/unsub * Refactor result handling in createMutation.svelte.ts Replace derived state with direct state and add watchChanges for result updates. --------- Co-authored-by: Lachlan Collins <[email protected]> * chore(svelte-query): fix eslint config (#9699) * chore(svelte-query): fix eslint config * Use @typescript-eslint/parser directly * ci: apply automated fixes * Fix sherif * Update docs and changeset * Update keywords --------- Co-authored-by: Zhiheng Zhang <[email protected]> Co-authored-by: Elliott Johnson <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: David <[email protected]>
🎯 Changes
Fixes the bug reported by @frederikhors here.
I thoroughly dug into this issue until I found calls to
Observer
andMutataionOBserver
were wrapped in a$derived(...)
in order to make them reactive to the passed in client. This was resulting in unsafe mutation errors, and there was already an$effect.pre(...)
used for updating options when they changed, so aside from the client reactivity, the$derived
wasn't necessary.To support reactivity for
queryClient
, the observer is now a$state
and is reassigned whenever the client changes.The
useIsFetching
anduseIsMutating
tests were previously passing because they used aQueryClient
local to the component, rather than one from context. I've updated the tests to use a provider instead, in order to properly repro this issue.✅ Checklist
pnpm test:pr
.🚀 Release Impact