fix: legacy mode: use coarse reactivity in the block's expressions

[7nik]

Fixes #14351

The block's expressions {#if someFn()} are deeply tracked in legacy mode, which is not how Svelte 4 reactivity works.

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: efb3b6a

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

[7nik]

Looks like I fixed everything. I didn't recreate this behavior - I'm sure it was a non-fixable bug.
Also, maybe it would be nice to also bail out when the accessed/called variable is a global one.

[Rich-Harris]

I was wondering about whether we could use the existing ExpressionMetadata approach rather than adding more AST logic (we already have an is_pure function, for example), so I opened #16100.

I like that approach because it gives us granular information about expressions — we can distinguish between whether an expression has state, a call, an assignment, or (on the async branch) an await expression and so on; each of those characteristics can be relevant in different situations. Adding that metadata to more nodes is something we could potentially use for other things in future (for example skipping effect creation for known-to-be-static {@html ...} blocks), particularly when we eventually start looking into cross-module optimisations.

A couple of tests are failing on that branch, but they relate to something that's also buggy on this branch. If you have something like this...

<script>
  let n = 0;
</script>

<p>n: {n}</p>

{#key n = 0}
  <p>n = 0: {n = 0}</p>
{/key}

<p>n: {n}</p>

<button on:click={() => {
  n += 1;
  console.log({ n });
}}>increment</button>

...then you get a whole bunch of different behaviours:

• v4
• v5
• this PR
• the other PR

In 4, we execute everything top-down and clone context every time we enter a fragment, which means that the key block mutates a local copy of n that is then used for the rest of the fragment, but n itself increments.

In 5, we end up not tracking n in the key block, because $.set(...) doesn't read the signal.

In this PR and the other PR, we do track n, which means we reassign it on every update, but we're reassigning the original value and not a local copy.

If we get rid of the {#key ...} block...

<p>n: {n}</p>
<p>n = 0: {n = 0}</p>
<p>n: {n}</p>

...the behaviour of 4 stays the same, but this time the assignment expression gets caught up in a template effect where n is read, meaning the behaviour of 5 matches this and the other PR:

• v4
• v5
• this PR
• the other PR

Which brings me to the failing test on #16100, which is one of the ones added here. The reality is that it's broken in both cases, because in both cases we're reassigning the actual value rather than a local copy (if you add a line to the inc handler like console.log({ a, b, c, ... }) you'll see what I mean).

So the question I'm trying to figure out is whether this discrepancy matters, and how far we should strive to be bug-compatible with Svelte 4. Even though 4 didn't prevent assignments-in-template, it's always been an extremely silly thing to do, and so my inclination is to not worry too much about it.

[7nik]

we already have an is_pure function

Oh, I missed it. Though, seems it allows calls and member access while disallowing simple expressions like a && b.

About the examples: in Svelte 4, only #if, #each, and #await, but no #key (I think because #8404 was fixed only in Svelte 5) clone the ctx, moreover they bail out doing it in cases like the example. And ever further, looks like the block expression is executed on the parent's ctx but not the child's.

I believe we shouldn't replicate Svelte 4 behavior up to all nuances (there are a bunch of buggy edge cases), but rather replicate the behavior people would expect from Svelte 4, unless is stated in RFC. In particular, I'm sure people wouldn't expect the n to continue to increment in the example. After all, n existing in a separate variable and in ctx is an implementation detail, and it should leak or have impact on the code behavior, right?

I'm surprised that, despite this being quite a big discrepancy of reactivity behavior, only a few users are affected.

I was wondering about whether we could use the existing ExpressionMetadata approach [...], so I opened #16100.

Sounds great, I didn't like that here I had to filter unrelated references, or track_assigment is calculated multiple times.

[7nik]

Talking about the test block-expression-assign, in particular

{#key 1}
    {@const x = (h = 0)}
    {x, ''}
{/key}

{#if 1}
    {@const x = (i = 0)}
    {x, ''}
{/if}

As #8404 was fixed in Svelte 5, you cannot have @const inside #key in Svelte 4, so it's not obvious, whether the behavior should be the same for both these blocks. But I tend to think it should.