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: improve derived connection to ownership graph #15137

Merged
merged 10 commits into from
Jan 29, 2025
Merged

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Jan 29, 2025

Fixes #15111 fully.

This is a follow up to #15129. If a derived is disconnected from the reactivity graph, then it should destroy any child effects/deriveds that it has. Furthermore, instead of blindly attaching a derived to it's original parent derived – we mark it as unowned, as the derived value might have been passed around to other owners since and thus not be associated anymore. For example if you create a class instance (that has derived properties) inside a derived and then pass that elsewhere, it's not technically owner by the original derived anymore.

Copy link

changeset-bot bot commented Jan 29, 2025

🦋 Changeset detected

Latest commit: 7f01dc3

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@15137

Comment on lines 425 to 426
(/** @type {Derived} */ (reaction).parent !== null &&
(/** @type {Derived} */ (reaction).parent.f & DERIVED) !== 0)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure you can reduce this to /** @type {Derived} */ (reaction).parent?.f & DERIVED) !== 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't use optional chaining with a bitwise flag?

Copy link
Member

@dummdidumm dummdidumm Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course you can?

let foo = { x: { i: 2 } };
console.log((foo.x.i & 2) !== 0); // true
console.log((foo.y?.i & 2) !== 0); // false

it doesn't work in the case of === 0 because then regardless of undefined or a number it's always 0, but it works in the !== 0 case

@gyzerok
Copy link

gyzerok commented Jan 29, 2025

Yesterday once we updated to 5.19.4 we discovered that while some cases were fixed others we still broken. Looks like it was highly dependent on the runtime path user takes to arrive to particular screen, where the issue were present.

Now I've tried this new fix with dev/prod build and REPL repro I've managed to come up with today. Works in both cases, hopefully that's the last of it.

Thank you for looking into it further!

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, though I would add my suggestion of using optional chaining to save some bytes

@trueadm
Copy link
Contributor Author

trueadm commented Jan 29, 2025

I thought I could also remove a lot of code and simplify things in my latest commit, however it might be overly invasive. @gyzerok can you try it out locally please?

@Autumnlight02
Copy link

This change causes a wreid issue that sometimes new properties, or objects, which are added as children are not reactive anymore, This is the commit which broke our production codebase.

@Autumnlight02
Copy link

@trueadm was that intended?

@trueadm
Copy link
Contributor Author

trueadm commented Feb 6, 2025

@Autumnlight02 You need to create a separate issue and provide a repro please.

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.

5.19.2 (and 5.19.3) fixed untrack error, but reactivity graph is broken somewhere
4 participants