fix: separate template_effect for dynamic class/style directive with dynamic attributes

[paoloricciuti]

Closes #13167

Svelte 5 rewrite

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

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.

Tests and linting

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

[changeset-bot]

🦋 Changeset detected

Latest commit: 93d3381

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

[paoloricciuti]

Ops, noticed some errors with spread only after i pushed...gonna fix those.

[paoloricciuti]

Ok so tests are failing for something i actually thought it could happen but initially tests were passing.

The problem is that currently we have state.init or state.update to update attributes and basically if something needs isolation it goes into state.init otherwise all updates are crammed into one single $.template_effect. This is slightly problematic because it means that if i move something to the init everything else needs to be moved to the init phase.

For example:

<script>
	export let foo = "font-size: 20px; color: blue;";
	let baz = "red"; // static value
	let bar = "32"; // static value interpolated
	export let bg = "gre"; // dynamic value interpolated/cached
	export let borderColor = "green"; // dynamic value non-cached
</script>

<p
	style:font-size="{bar}px"
	style:color={baz}
	style="{foo}"
	style:background-color="{bg}en"
	style:border-color={borderColor}
></p>

attributes here are dynamic and the style directive also has_state currently this get's compiled to

$.template_effect(() => {
	$.set_attribute(p, "style", foo());
	$.set_style(p, "font-size", `${bar ?? ""}px`, undefined, true);
	$.set_style(p, "color", baz, undefined, true);
	$.set_style(p, "background-color", `${bg() ?? ""}en`, undefined, true);
	$.set_style(p, "border-color", borderColor(), undefined, true);
});

and the order of the statement is defined by the position in which they are pushed to state.update. However to fix the bug i need to move the directives to state.init but that will basically snowball to everything being in init.

I was thinking could be worth to split the updates in an array of arrays of statement and each array it's a separate $.template_effect?

I need to explore this a bit more because i think there are other things in place that i need to fix but i would love some input in the meantime @dummdidumm

[paoloricciuti]

Ok i can make tests pass with this change which is also the correct one imho...basically i was wrongly also check for has_state but i only needed to check for has_call to move it to it's own separate effect. Then the only thing left to do is move spreads to the init too if some style or class directive has_call.

It seems to be correct now but would love a double check to make sure i'm not missing anything.

[trueadm]

We don't want to create extra template effects here, instead we just need to cache the value in a derived, like we do for other types of things inside the template. Not sure how we missed that for this class use-case but if you see how text updates work then it should be the same as that.

[paoloricciuti]

We don't want to create extra template effects here, instead we just need to cache the value in a derived, like we do for other types of things inside the template. Not sure how we missed that for this class use-case but if you see how text updates work then it should be the same as that.

Maybe i'm setting something wrong but it seems that with text we are creating multiple template effects (if you change text() with count it will be a single template effect)...how can trigger the optimization you talked about?

[dummdidumm]

We did use deriveds a while ago, but as Paolo said we're now just splitting into template effects.

[trueadm]

We don't want to create extra template effects here, instead we just need to cache the value in a derived, like we do for other types of things inside the template. Not sure how we missed that for this class use-case but if you see how text updates work then it should be the same as that.

Maybe i'm setting something wrong but it seems that with text we are creating multiple template effects (if you change text() with count it will be a single template effect)...how can trigger the optimization you talked about?

You need to have more than a single call expression in the same element. Then you will see that we add deriveds. However, it also seems to output duplicate lines that never get used too, so there's clearly been a regression with that optimisation.

We did use deriveds a while ago, but as Paolo said we're now just splitting into template effects.

The optimisation is still there, it's just you need to apply it for the same element.

[paoloricciuti]

Oh, we are creating more deriveds that we need

import * as $ from 'svelte/internal/client';

function increment(_, count) {
    $.set(count, $.get(count) + 1);
}

var root = $.template(`<button>+1</button> <input> <p> </p>`, 1);

export default function App($$anchor) {
    let count = $.state(0);
    let val = $.state('');

    function text() {
        console.log('text');
        return $.get(count);
    }

    var fragment = root();
    var button = $.first_child(fragment);

    button.__click = [increment, count];

    var input = $.sibling(button, 2);

    $.remove_input_defaults(input);

    var p = $.sibling(input, 2);
    const stringified_text = $.derived(() => text() ?? ''); // <-- unused
    const stringified_text_1 = $.derived(() => text() ?? ''); // <-- unused
    const stringified_text_2 = $.derived(() => text() ?? '');
    const stringified_text_3 = $.derived(() => text() ?? '');
    var text_1 = $.child(p);

    $.template_effect(() =>
        $.set_text(
            text_1,
            `${$.get(stringified_text_2)}${$.get(stringified_text_3)}`
        )
    );
    $.reset(p);
    $.bind_value(
        input,
        () => $.get(val),
        ($$value) => $.set(val, $$value)
    );
    $.append($$anchor, fragment);
}

$.delegate(['click']);

will also look into this and i will see if there's anything that we can do like this for the class thing

[paoloricciuti]

@dummdidumm do you have more insight about why this deriveds optimization is only used when the nodes are in the same text node? Considering the class directive and the class attribute compile to two different things i would expect the same reasoning here (so two different template_effects just like we don't create a derived for the same call in two different nodes).

[dummdidumm]

It's done because we can't put them into separate effects, as they write to the same text node.

class and style directives are somewhat special because we need to make sure that they always take precedence over styles and classes applied through their attributes, so separating them into a different template might not work (because e.g. the class attribute could change in one effect, but the class directive effect doesn't run because it hasn't changed, and now suddenly the class attribute value takes precedence when it shouldn't) - so a derived could ultimately be the correct solution.

[paoloricciuti]

It's done because we can't put them into separate effects, as they write to the same text node.

class and style directives are somewhat special because we need to make sure that they always take precedence over styles and classes applied through their attributes, so separating them into a different template might not work (because e.g. the class attribute could change in one effect, but the class directive effect doesn't run because it hasn't changed, and now suddenly the class attribute value takes precedence when it shouldn't) - so a derived could ultimately be the correct solution.

Oh gotcha...will work on this then