-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Conversation
🦋 Changeset detectedLatest commit: 7f01dc3 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 |
|
(/** @type {Derived} */ (reaction).parent !== null && | ||
(/** @type {Derived} */ (reaction).parent.f & DERIVED) !== 0))); |
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.
Pretty sure you can reduce this to /** @type {Derived} */ (reaction).parent?.f & DERIVED) !== 0
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.
You can't use optional chaining with a bitwise flag?
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.
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
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! |
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.
looks good, though I would add my suggestion of using optional chaining to save some bytes
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? |
packages/svelte/tests/runtime-runes/samples/derived-unowned-7/_config.js
Outdated
Show resolved
Hide resolved
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. |
@trueadm was that intended? |
@Autumnlight02 You need to create a separate issue and provide a repro please. |
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.