fix: exclude derived writes from effect abort and rescheduling

[paoloricciuti]

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

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.
• If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

🦋 Changeset detected

Latest commit: ab32c96

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

[github-actions]

Playground

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

[Rich-Harris]

Something I've been working on separately, that would fix this and #16413, is to clear out the current_batch after we've traversed the effect tree and before flushing queued effects, such that any changes inside those queued effects result in a new batch being created.

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 current_batch !== null translates to 'strap in, more work incoming'), but I just couldn't get it to work. @dummdidumm has been attacking the problem as well though and has some promising results.

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

[Rich-Harris]

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

[paoloricciuti]

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 😂