chore: memoize clsx()

[adiguba]

Just a little improvement on clsx().

Function call in the template are memoized in order to be executed only on state changes.
But this is not the case for $.clsx() which is added by the compiler.

Currently, a code like that :

<script>
	let { class: className } = $props();
</script>

<div class={className}>
	... stuff
</div>

is compiled into something like that :

	$.template_effect(() => {
		$.set_class(div, 1, $.clsx($$props.class));
		// ... stuff
	});

So, every time the template is updated, $.clsx($$props.class) is re-evaluated.

With this PR, the $.clsx($$props.class) will be memoized on the template_effect(), and executed only when necessary :

	$.template_effect(($0) => {
		$.set_class(div, 1, $0);
		// ... stuff
	}, [() => $.clsx($$props.class)]);

Note that this is only done when the value of $.clsx() is not already memoized (I think this is a rare case which would be more complex to treat).

I don't have a test for that (I don't known how to check that), but all the current test are OK.

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

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

[svelte-docs-bot]

Preview: https://svelte-dev-git-preview-svelte-15441-svelte.vercel.app/

[github-actions]

Playground

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

[Rich-Harris]

Good catch. I think we can get away without a test here (if we wanted to add one it would be in the snapshot folder but we try to use those sparingly as they constantly need updating).

I noticed that it doesn't memoize expressions that already involve memoization — this...

<div class={foo()}></div>

...becomes this...

$.template_effect(($0) => $.set_class(div, 1, $.clsx($0)), [() => $$props.foo()]);

...rather than this:

$.template_effect(($0) => $.set_class(div, 1, $0), [() => $.clsx($$props.foo())]);

No idea off the top of my head if that's avoidable?

[adiguba]

I noticed that it doesn't memoize expressions that already involve memoization

Yes that may come with more complex case to handle, so I skipped it for now.
Especially in case of multiple calls like this code

	$.template_effect(
		($0) => {
			$.set_class(div, 1, $.clsx($0));
			$.set_class(div_1, 1, $.clsx($0));
		},
		[() => $$props.foo()]
	);

Perhaps we can use memoize_expression() instead, like for "autofocus", so it can be used to memoize clsx() :

	const expression = $.derived(() => $$props.foo());

	$.template_effect(
		($0) => {
			$.set_class(div, 1, $0));
			$.set_class(div_1, 1, $0);
		},
		[() => $.clsx(expression)]
	);

But it may be a more complex change, as currently memoize_expression() do no handle duplicate...

[Rich-Harris]

To be honest we should probably remove the deduplication anyway, per #15096 — will open a PR for that

[adiguba]

Without the deduplication it would be easier to do !
I will update this PR...

[Rich-Harris]

Merged #15451 so should be good to go

[adiguba]

I got mixed up with git, so I made another branch/PR : #15456