-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix: ensure derived signals maintain reactions after deps change outside effect context #17262
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 28c6171 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Co-authored-by: dummdidumm <5968653+dummdidumm@users.noreply.github.com>
…eriveds Co-authored-by: dummdidumm <5968653+dummdidumm@users.noreply.github.com>
Co-authored-by: dummdidumm <5968653+dummdidumm@users.noreply.github.com>
…sent cycling) Co-authored-by: dummdidumm <5968653+dummdidumm@users.noreply.github.com>
|
@copilot It could be multiple deriveds interacting. Also try a test where the derived is created in an effect context but then subsequently moved out of that effect context. Maybe the effect it was created in even is destroyed but the derived now lives on separately |
…ect contexts Co-authored-by: dummdidumm <5968653+dummdidumm@users.noreply.github.com>
Added tests in commit 153e91d:
All tests pass. The reconnection logic handles these scenarios correctly. |
|
@copilot This seems to be a reproduction: |
…t reconnection bug Co-authored-by: dummdidumm <5968653+dummdidumm@users.noreply.github.com>
Reproduced the bug in commit a51580b. Two tests now fail:
The key difference from passing tests: these use actual |
|
@copilot Now fix the bug |
…ide effect context Co-authored-by: dummdidumm <5968653+dummdidumm@users.noreply.github.com>
Fixed in commit 28c6171. Root cause: When a connected derived's deps change during Fix: After |
|
Have a feeling this is wrong. I wonder if the real fix is to not set deriveds to clean if they are interacted with outside |
|
Hey, thanks for working on this bug! Just wondering if there are any blockers merging this because it's breaking a lot of stuff for our app. One of the toggles gets stuck in a |
Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lintFixes #17263 - A
$derivedproperty depending on aSvelteSetdoesn't update when read immediately after modifying the set.Root Cause
When a connected derived's deps change during
update_derivedoutside of an effect update cycle (is_updating_effect === false), theupdate_reactionfunction doesn't add the derived to new deps' reactions (due to the check on line 281). This causes the derived to not be notified when those new deps change.This manifests when:
SvelteSetviaset.has(value)The Fix
After
update_derivedcompletes in theget()function, if the derived was already connected, iterate over its current deps and ensure each one has the derived in its reactions. This handles the case where deps changed butupdate_reactiondidn't add the reactions due tois_updating_effectbeing false.Test Coverage Added
derived passed as prop - item with expanded derived from SvelteSetderived with SvelteSet - disconnected then reconnectedAll 156 signals tests pass, and 7007 tests in the full suite pass.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.