-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix: exclude derived writes from effect abort and rescheduling #16477
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
Conversation
🦋 Changeset detectedLatest commit: ab32c96 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 |
|
Something I've been working on separately, that would fix this and #16413, is to clear out the That would allow us to do this instead of tracking the number of derived writes: /**
* @param {Array<Effect>} effects
* @returns {void}
*/
function flush_queued_effects(effects) {
var length = effects.length;
if (length === 0) return;
var i = 0;
while (i < length) {
var effect = effects[i++];
if ((effect.f & (DESTROYED | INERT)) === 0 && is_dirty(effect)) {
- var wv = write_version;
+ // if state is written in a user effect, abort and re-schedule, lest we run
+ // effects that should be removed as a result of the state change
+ if (current_batch !== null) {
+ break;
+ }
update_effect(effect);
// Effects with no dependencies or teardown do not get added to the effect tree.
// Deferred effects (e.g. `$effect(...)`) _are_ added to the tree because we
// don't know if we need to keep them until they are executed. Doing the check
// here (rather than in `update_effect`) allows us to skip the work for
// immediate effects.
if (effect.deps === null && effect.first === null && effect.nodes_start === null) {
// if there's no teardown or abort controller we completely unlink
// the effect from the graph
if (effect.teardown === null && effect.ac === null) {
// remove this effect from the graph
unlink_effect(effect);
} else {
// keep the effect in the graph, but free up some memory
effect.fn = null;
}
}
- // if state is written in a user effect, abort and re-schedule, lest we run
- // effects that should be removed as a result of the state change
- if (write_version > wv && (effect.f & USER_EFFECT) !== 0) {
- break;
- }
}
}
while (i < length) {
schedule_effect(effects[i++]);
}
} I think this is probably a more 'correct' fix (insofar as So I think the move is to wait for that PR, and add this test. But if it falls through, then this is definitely a good interim fix |
export function get_derived_writes() { | ||
return derived_writes; | ||
} |
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.
FWIW we don't need to expose function wrappers to get values from different modules, only to set them. we can just export let derived_writes
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.
I'm always so skeptical of live exports...like under my skin I know they works but I just can't trust myself enough to do it 😂
Closes #16423
One possible strategy to fix this issue (maybe not the best ?). We keep track of how many derived_writes we have before updating the effect and we subtract that number from the
write_version
to have check if something else increased it (namely setting a derived or a source).One thing that I would've loved to confirm but failed to do so is check that the test in #16280, if rewritten with deriveds, still pass. But that's very convoluted and I was not able to find a way to rewrite it so that the derived was updated in the effect.
Let's discuss if this is a good strategy.
@dummdidumm also had the idea to use even and off numbers for
write_version
for deriveds and sources but that sounds a nightmare to debug.Another idea I had was literally just a flag on the effect when
internal_set
is called.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 test
and lint the project withpnpm lint