Skip to content
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: ensure reactions are correctly attached for unowned deriveds #15158

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Jan 30, 2025

Fixes #15149. We weren't setting the active_reaction when processing effects and checking if they were dirty, which mean the skip_reaction logic was not treating unowned deriveds as connected even though they should as we're flushing effects at this stage of reconciliation.

Copy link

changeset-bot bot commented Jan 30, 2025

🦋 Changeset detected

Latest commit: f04f22b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

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

Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@15158

@dummdidumm
Copy link
Member

could you explain what deriveds were unowned in this example? Looking at the generated code, shouldn't it be seen as owned (directly created in an effect; the root effect)?

@trueadm
Copy link
Contributor Author

trueadm commented Jan 30, 2025

could you explain what deriveds were unowned in this example? Looking at the generated code, shouldn't it be seen as owned (directly created in an effect; the root effect)?

Deriveds are no longer attached to effect. Effects and deriveds no longer own deriveds too – this was changed in a previous PR because it was leading to bugs where the derived was being destroyed when the effect or owning derived re-runs. If you create a derived in something like a class and then pass that out of the owning derived or effect then it makes no sense for it suddenly get destroyed by something else.

Deriveds are marked as unowned when they're read outside of a tracked context – in this case from the props.js logic that reads it for the $bindable() logic. We mark them as unowned as nothing owns them and we don't want them to possible leak.

@dummdidumm
Copy link
Member

Ok, so asking differently - in which cases are deriveds still owned? Because it sounds like they are almost never owned now?

@trueadm
Copy link
Contributor Author

trueadm commented Jan 30, 2025

@dummdidumm A derived being unowned doesn't mean it still can't be connected to the graph – they are mostly connected to the graph when a tracking effect references them. If a derived is only ever read inside an effect, it is never set as unowned too.

@trueadm trueadm merged commit 7bef596 into main Jan 30, 2025
10 checks passed
@trueadm trueadm deleted the unowned-fix-3 branch January 30, 2025 18:17
This was referenced Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Svelte 5.19.5 breaks reactivity, Svelte 5.19.4 works fine
2 participants