Conversation
🦋 Changeset detectedLatest commit: 66d6d4b 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 |
Contributor
|
dummdidumm
reviewed
Nov 4, 2025
Member
dummdidumm
left a comment
dummdidumm
left a comment
There was a problem hiding this comment.
This looks promising - what I don't quite understand at the moment is how it's safe that something counts as owned as long as it has any reactions
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
Member
Author
|
There is no concept of ownership any more — something is either connected (in which case it will be notified of changes to its dependencies, and will in turn notify its dependents) or it isn't. We don't need to care if a derived is owned or not |
* fix: delete from batch_values on updates This fixes a bug where a derived would still show its old value even after it was indirectly updated again within the same batch. This can for example happen by reading a derived on an effect, then writing to a source in that same effect that makes the derived update, and then read the derived value in a sibling effect - it still shows the old value without the fix. The fix is to _delete_ the value from batch_values, as it's now the newest value across all batches. In order to not prevent breakage on other batches we have to leave the status of deriveds as-is, i.e. within is_dirty and update_derived we cannot update its status. That might be a bit more inefficient as you now have to traverse the graph for each `get` of that derived (it's a bit like they are all disconnected) but we can always optimize that later if need be. Another advantage of this fix is that we can get rid of duplicate logic we had to add about unmarking and reconnecting deriveds, because that logic was only needed for the case where deriveds are read after they are updated, which now no longer hits that if-branch * keep derived cache, but clear it in mark_reactions (#17116) --------- Co-authored-by: Rich Harris <rich.harris@vercel.com>
Rich-Harris
commented
Nov 5, 2025
Merged
This was referenced Nov 6, 2025
Closed
6 tasks
dummdidumm
added a commit
that referenced
this pull request
Dec 2, 2025
In #17105 one line in `update_reaction` was changed that can cause reactivity loss. It checks if the reaction is updated inside of an effect and only then will push to the reactions. The prior version had an additional check to still add to the reactions if there is already at least one reaction on the derived, indicating it is connected. Removing this check fixes #17263 while keeping correctness: a connected derived by definition at least has one reaction and therefore can properly cleanup.
6 tasks
dummdidumm
added a commit
that referenced
this pull request
Dec 2, 2025
In #17105 one line in `update_reaction` was changed that can cause reactivity loss. It checks if the reaction is updated inside of an effect and only then will push to the reactions. The prior version had an additional check to still add to the reactions if there is already at least one reaction on the derived, indicating it is connected. Removing this check fixes #17263 while keeping correctness: a connected derived by definition at least has one reaction and therefore can properly cleanup.
6 tasks
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
2 participants
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.
This makes good on my threat in #17102 (comment) to sort out all the convoluted nonsense around
UNOWNEDandDISCONNECTEDderiveds. We've made this far more complicated than it needs to be. All we need to do is push the current signal to its dependencies'reactionswhen the signal is connected to the graph, and skip doing so when it isn't.Fixes #17024
Fixes #17049 (comment) (and therefore everything that was still buggy in that issue I think)
Most benchmarks are unaffected, but where there is a significant effect, this branch is much faster.
results of `pnpm bench:compare`
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 lint