fix: always assign text.nodeValue

[Rich-Harris]

alternative to #11937
Closes #11932

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: 699caa6

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]

Are you removing skipping during hydration because reading nodeValue to eventually skip if eventually more expensive than just set the value?

[Rich-Harris]

there's just nothing to do differently in the hydration case any more

[paoloricciuti]

there's just nothing to do differently in the hydration case any more

Wouldn't be possible to skip the assignment since the value is already there from SSR?

[Rich-Harris]

it could have changed

[trueadm]

This results in DOM mutations even when the content is the same unfortunately

[Rich-Harris]

fixed with a if (text.nodeValue != value) wrapper

[trueadm]

This is slower than what we have on main:

This PR:

Main:

benchmark

I wrapped process_deferred in console.times locally to capture it with 6x slowdown.

[trueadm]

My proposed fix is:

export function set_text(dom, value) {
	// @ts-expect-error need to add __value to patched prototype
	const prev_node_value = dom.__nodeValue ??= dom.nodeValue;
	const next_node_value = stringify(value);
	if (prev_node_value !== next_node_value) {
		dom.nodeValue = next_node_value;
		// @ts-expect-error need to add __className to patched prototype
		dom.__nodeValue = next_node_value;
	}
}

Text.prototype.__nodeValue = undefined;

[Rich-Harris]

Tweaked the benchmark to use flushSync so that it's not dependent on local changes, and I'm actually seeing the opposite, strangely enough

[trueadm]

@Rich-Harris If you put in stringify(value) back into your PR (otherwise tests fail), it's definitely showing as slower for me.

[trueadm]

Here's another benchmark that shows it even clearer.

[dummdidumm]

Even if it's a tiny bit slower, does that really matter? It's roughly 1 milisecond judging from the benchmark, and that's for 4000 text nodes which is a lot.
Before coming to a conclusion we should also bench what happens if the value turns out to be the same. Like, is text.nodeValue substantially slower than text.__nodeValue?

[Rich-Harris]

I found implicit coercion to be faster than explicit. Just pushed a tweak

[trueadm]

@dummdidumm My last comment shows that actually, pretty well too. Update: Rich just patched it, so the overhead from reading nodeValue is now gone when the value doesn't change.