fix: whitespace completely removed in each loop

[paoloricciuti]

Svelte 5 rewrite

Closes #11932

I don't know if this is the best fix but basically the problem is that the empty text node that get passed to set_text is never touched if the text to update is ' '...this is because the prev_node_value was dom.__nodeValue which by default is ' '. However the actual text of the text node is ''. So this is never touched and the result is that every node that has ' ' is basically cancelled.

My fix was to check if nodeValue and __nodeValue are equals so that if they are not nodeValue is given precedence as prev_node_value.

Other fixes i've thought of are:

1. add a __touched property yo TextNode prototype so that we know if the node has ben touched or not. I think messing with the prototype is worst than this.
2. we could literally have a special case for __nodeValue===' ' since the dissonance between the default value of __nodeValue and the "default value" of the empty text node is what's causing the issue.

let me know if you think one of those two solutions is better.

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: 170ef6f

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

[Rich-Harris]

I believe the purpose of using dom.__nodeValue was to avoid the cost of the dom.nodeValue getter — if that's true, then this change would negate that benefit and we may as well just do this:

export function set_text(dom, value) {
  const next = stringify(value);
  if (dom.nodeValue !== next) {
    dom.nodeValue = next;
  }
}

@trueadm might have more background. It was introduced here: https://github.com/sveltejs/svelte-octane/pull/413

[Rich-Harris]

Another thing that would work is changing the default __nodeValue to something that can't possibly match the initial value. Even just undefined would work. I'm not sure if there's some penalty to using a non-string value here.

I'd be curious to see some benchmarks to know whether we're gaining anything by using __nodeValue rather than just nodeValue.

[trueadm]

Maybe the __nodeValue should be ‘ ‘ instead? It definitely is faster than reading the he getter, as the getter has overhead that we want to avoid.

[paoloricciuti]

Another thing that would work is changing the default __nodeValue to something that can't possibly match the initial value. Even just undefined would work. I'm not sure if there's some penalty to using a non-string value here.

I'd be curious to see some benchmarks to know whether we're gaining anything by using __nodeValue rather than just nodeValue.

There's a comment about this specific issue too

https://github.com/sveltejs/svelte-octane/pull/413#discussion_r1347106724

[Rich-Harris]

Maybe the __nodeValue should be ‘ ‘ instead?

What do you mean? It's already ' ', and that's the problem. It needs to be something that couldn't possibly match the new value

[paoloricciuti]

We could do this check only when the value is ' ' so that generally the overhead of the getter would not be there. But yeah changing the default to something that can't be a nodeValue would actually be better...maybe even a Symbol?

[Rich-Harris]

I'm doing some microbenchmarks locally, and __nodeValue doesn't seem to give us any advantage — it's slower in addition to being more code. This is the code I'm using:

Text.prototype.__x = '';

const iterations = 1e6;

function a() {
  console.time('expando property on prototype');
  for (let i = 0; i < iterations; i += 1) {
    const text = document.createTextNode('');

    if (text.__x !== i) {
      text.__x = i;
      text.nodeValue = '' + i;
    }
  }
  console.timeEnd('expando property on prototype');
}

function b() {
  console.time('expando property not on prototype');
  for (let i = 0; i < iterations; i += 1) {
    const text = document.createTextNode('');

    if (text.__y !== i) {
      text.__y = i;
      text.nodeValue = '' + i;
    }
  }
  console.timeEnd('expando property not on prototype');
}

function c() {
  console.time('nodeValue with explicit coercion');
  for (let i = 0; i < iterations; i += 1) {
    const text = document.createTextNode('');
    const str = '' + i;

    if (text.nodeValue !== str) {
      text.nodeValue = str;
    }
  }
  console.timeEnd('nodeValue with explicit coercion');
}

function d() {
  console.time('nodeValue with implicit coercion');
  for (let i = 0; i < iterations; i += 1) {
    const text = document.createTextNode('');

    if (text.nodeValue !== i) {
      text.nodeValue = i;
    }
  }
  console.timeEnd('nodeValue with implicit coercion');
}

AFAICT reading text.nodeValue doesn't cause layout, so it seems unlikely that would change in a more realistic benchmark. Very much open to evidence to the contrary, but right now it seems to me that we're shooting ourselves in the foot

[Rich-Harris]

Actually, subsequent nodeValue reads do appear to be more expensive. Anyway, I opened #11944 so we can at least compare them

[trueadm]

I'll take a look into this properly tomorrow.