svelte

svelte copied to clipboard

Related #7341, #7226

1. We have optimsiation to use innerHTML to create static element nodes, however, this is de-optimised, when the hydratable flag is on. This PR allows the element nodes to be claimed using innerHTML. It will create a data-svelte hash on the element that can be optimised this way, and if the data-svelte hash does not match, it will fallback to use innerHTML to "fix" the elements.
2. when {@html xxx} is the only child of an element, we "optimised" by setting parent.innerHTML = xxx on mount, for both CSR and hydration. However, for hydration, this will destroy and recreate existing elements from the SSR. When we have {@html xxx} as the only child of an element, we add data-svelte hash and only do .innerHTML if the hash does not match.
3. while adding tests for above, found there's some differences in the HTML from CSR and SSR, namely, when there's space between sibling elements, \s\t\r\n, CSR collapse them into 1 space element ' ', however SSR maintains the original spaces. this PR fix the consistency by collapsing spaces in SSR into 1 space ' '. since however many space between the sibling elements, as long as they are not within <pre> or <textarea>, browser will treat them as only 1 space element. This behavior however can be disabled through preserveWhitespace compiler option.

Before submitting the PR, please make sure you do the following

• [x] Prefix your PR title with [feat], [fix], [chore], or [docs].
• [x] This message body should clearly illustrate what problems it solves.
• [x] Ideally, include a test that fails without this PR but passes with it.

Tests

• [x] Run the tests with npm test and lint the project with npm run lint

I thinks this fixes #7226

will wait for https://github.com/sveltejs/svelte/pull/7280

If we want to use data-svelte we might have to tweak SvelteKit to avoid any conflicts since it currently uses that

If we want to use data-svelte we might have to tweak SvelteKit to avoid any conflicts since it currently uses that

hmm.. but sveltekit targets style[data-svelte] right? probably unlikely that it conflicts?

currently data-svelte is also used in head tags for hydration too

<svelte:head>
   <meta name="xxx" content="yyy" />
</svelte:head>

will ssr render to

<meta name="xxx" content="yyy" data-svelte="hash" />

oh, yes, right you are. please forgive my sleep deprived comment! though it's probably not a bad idea for SvelteKit to use data-sveltekit rather than data-svelte anyway

This is awesome! Very eager for this to be merged :)

@tanhauhau just a small heads up before our PR bash that there's a merge conflict on this one

@tanhauhau is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@tanhauhau in case you haven't seen it I added some comments on the possibility of the @html optimisation being wrong in some cases. I also think data-svelte might be too generic of a name to use because we might want to use that namespace later for other things, too. What do you think about those comments?

in case you haven't seen it I added some comments on the possibility of the @html optimisation being wrong in some cases

i wonder should we introduce a way to de-optimise the @html back to how it was previously. im not quite sure how we would be determine automatiaclly

I also think data-svelte might be too generic ...

i dont have strong feelings for renaming this one.

Let's rename it to data-svelte-h (for hydrate) then.

Does #7469 contradict with this PR? Is this another hint that we can't confidently do the {@html ..} optimisation?

@tanhauhau I reverted the @html changes and changed it from data-svelte to data-svelte-h - could you have another look if the changes look good?

meaning right now for static nodes in hydration you would always do parent.innerHTML = '...' ?

this would destroy the SSR-ed html elements, and replace it with the .innerHTML-ed HTML string.

i see u removed some of the tests assertions over here https://github.com/sveltejs/svelte/pull/7426/commits/718a25b53cfc073b523d7521cb48d89a8ecfbd99 that tries to assert the same element instance in hydration test

i wonder would it be possible or worthwhile to check if the parent.innerHTML !== '...' before we decided to wipe it away with parent.innerHTML = '...', eg:

if (parent.innerHTML !== '...') {
  parent.innerHTML = '...';
}

meaning right now for static nodes in hydration you would always do parent.innerHTML = '...'?

No, only for {@html ..} blocks. The other truly static content is checked against data-svelte-h as you implemented it, I just removed that logic for {@html ..} blocks because content could've changed; we can't know.

i see u removed some of the tests assertions over here https://github.com/sveltejs/svelte/commit/718a25b53cfc073b523d7521cb48d89a8ecfbd99 that tries to assert the same element instance in hydration test

Yes, because the test _before.html is different than _after.html, so the whole thing is replaced with .innerHTML per this PR. It didn't occur before because comments for the DOM were only implemented recently, and this is an effect of that now being different.

In general, this got my thinking - shouldn't all the tests in hydrate have data-svelte-h comments? Rather, are most of these tests now obsolete? Many of them check static content before/after, which by this PR is now done by comparing the data-svelte-h and doing nothing if it matches, and replacing everything with .innerHTML = .. if it doesn't - and since none of the tests set it, all the static content is recreated, right? I think we need to update the tests?

i wonder would it be possible or worthwhile to check if the parent.innerHTML !== '...' before we decided to wipe it away with parent.innerHTML = '...', eg:

How would we write code that compares dynamic content before replacing it? I don't see how that's possible.

ok that make sense. the PR has been a while, i've totally forgotten what i've done over here. 🙈

anyway, i think the changes looks good to me

One thing I'm worried about is that with this change everyt little bit of HTML will have a data-svelte-h attribute associated with it. Even <div>hi</div> will have it. What does this mean for the HTML size - is it ok or do we need to special-case the "one element with text in it"-case to be less consuming? Idea would be to compare textContent with what should be there and set it if it doesn't match. Like

function hydate_single_node_with_text(node, text) {
  if (node.textContent !== text) {
   node.textContent = text;
  }
}

// .. inside hydration block:
hydate_single_node_with_text(div, 'the text that should appear');

One thing I'm worried about is that with this change every little bit of HTML will have a data-svelte-h attribute associated with it. Even <div>hi</div> will have it. What does this mean for the HTML size

I think we come out very far ahead. I ran it on a real site earlier up above (https://github.com/sveltejs/svelte/pull/7426#pullrequestreview-1132119458) and it created 29 instances of data-svelte-h. That's not going to add too much size. Meanwhile it saved 6.5k on the main JS.

It's possible this could be further optimized, but I'm not sure it's worth sinking time into honestly as I'd rather spend time thinking about how to optimize perf for Svelte 5. This PR is still nice as it gives us a good starting point for benchmarking. Anything we come up with for Svelte 5 that's a very different approach like should have to beat this method by a good margin if we want to pursue it.

I also think data-svelte might be too generic

Yeah, but data-svelte-h sounds quite unintuitive for me. Maybe data-svelte-static or something more appropriate and readable?