[fix] hydration improvements #6449

hbirler · 2021-06-25T16:48:50Z

This PR tries to address some issues that were not addressed by #6395 and issues pointed out by @benmccann and @ankitstarski

An img element's src value should only be set if it is not the expected value. This check existed, but failed to account for relative paths which were returned as absolute paths when checking img.src.
Existing nodes under <head> that are not claimed but also not detached should be ignored by init_hydrate. These nodes have node.claim_order === undefined
The function claim_html_tag should also set claim_order.

Edit:

Add a fast path to init_hydrate for when most of the nodes are on the longest increasing subsequence.
Improve claims for text nodes: There are many cases where we try to only claim a prefix of the text node. In these cases, we can use splitText to actually only claim the prefix. (Merging text siblings might help further Optimise parsed AST #3766)

Edit2:

Seems to partially address {@html} hydration in head duplicates elements #6463

Potential issues:

~~Is it possible that location is overwritten at a local scope by the compiler? This would cause URL normalization to break.~~

src/runtime/internal/dom.ts

hbirler · 2021-06-25T18:49:11Z

@benmccann Does this fix the image flickering issue that you encountered by any chance? This time, I tried to normalize both sides of the equality.

benmccann · 2021-06-25T20:56:11Z

It didn't help. I can't figure out what's causing it. It stops both if I turn off hydration and if I remove my global stylesheet. I can't find a line of code that's clearly causing it though, so I guess we should just ignore it for now. I'll let you know if I find anything else. I think you could probably revert the double URL normalization from 243c562 since it didn't help because it seems like extra code and CPU time without benefit

hbirler · 2021-06-25T21:00:59Z

I added the double URL normalization because I couldn't find a proper spec that clearly mentions that reading the .src attribute should always return the full absolute URL. (Maybe I missed it)

I saw a mention here https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement
but not here https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/src
and also not here https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-src

benmccann · 2021-06-25T21:03:22Z

Makes sense. I suppose I could see it working differently in different browsers in that case

dummdidumm · 2021-07-01T07:33:51Z

Is it possible to add tests that would fail without these fixes?

hbirler · 2021-07-01T12:13:53Z

@dummdidumm I believe hydration/head-meta-hydrate-duplicate as it is here fails in master and passes with this pull request. My modifying that test in #6395 was erroneous (I had assumed that the order of those nodes in head would not matter). This pull request reverts that test to how it was earlier.

benmccann · 2021-07-07T21:26:44Z

~~I tested that this fixes #6506, which reordered body content~~

hbirler · 2021-07-07T22:20:22Z

@benmccann Interesting, but I don't understand how this PR would solve a body reordering issue. Does it involve claim_html_tag? (I likely won't have time to fully investigate on my own for the next couple of days.)

benmccann · 2021-07-07T22:38:53Z

Ohh nevermind. I must have messed something up when testing earlier. That one appears to still be broken. Would you be able to take a look at it?

Normalize src values before checking equality

Set `claim_order` in claim_html_tag

Normalize both sides of the equality to be sure

Use imports

Do not create array from children if not needed

Split an existing text node if we match the prefix

benmccann · 2021-07-08T12:48:37Z

@hbirler it looks like this PR will need to be rebased

Conduitry · 2021-07-08T15:05:49Z

I'm a little concerned about using the URL API. Are there other places in Svelte itself (not in SvelteKit or Sapper) where we're using things that would need to be polyfilled on IE11? (Note that I'm specifically wondering about things that need to be polyfilled, and not things that need to be transpiled.)

Is there another way we could do this, like with a shared anchor element outside of the DOM that we set and check the href of?

hbirler · 2021-07-08T15:52:57Z

@Conduitry do you mean something like the following:

const relative_to_absolute = (function() {
    var anchor = document.createElement('a');
    return function(url) {
        anchor.href = url;
        return anchor.href;
    }
})();

I used the URL API since I assumed it would be faster but I did not actually check whether it is.

hbirler · 2021-07-08T15:58:49Z

Additionally Array.from seems to not be supported in IE11 and it is used in many places in the code. So it would need to be polyfilled.

benmccann · 2021-07-08T16:33:21Z

I'm a bit concerned about the potential performance impact of mutating a DOM element everytime we want to do this check. It might be better to just manually construct the string using window.location in the case that we want to avoid using URL

hbirler · 2021-07-08T17:43:02Z

@benmccann It might not be simple to manually canonicalize a URL:
https://chromium.googlesource.com/chromium/src/+/refs/heads/main/url/url_util.cc#211
https://chromium.googlesource.com/chromium/src/+/refs/heads/main/url/url_util.cc#291

benmccann · 2021-07-08T17:56:01Z

But, as far as I'm aware, we don't need to fully canonicalize a URL. We just need to prepend the hostname when comparing to an a.href

hbirler · 2021-07-08T17:58:34Z

Is it not possible that the relative URL looks like ../../image.png?

benmccann · 2021-07-08T19:32:07Z

That still seems doable to handle. Another option might be to just remove that particular optimization if it's getting too complicated. I doubt that setting img.src is any more expensive than setting a.href to calculate if the URLs are the same. It didn't seem to be causing a second network request for the image. Or was it also causing the whole img tag and/or subsequent/previous tags to be reinserted?

hbirler · 2021-07-08T19:34:41Z

I believe it was causing a visual flicker.

hbirler · 2021-07-08T19:57:23Z

In a surprising turn of events, using the anchor seems to actually be faster https://jsbench.me/stkqvbw6qj/2

Use an anchor element's href attribute to convert relative paths to absolute paths

benmccann · 2021-07-08T20:13:26Z

I can't argue with that!

dominikg · 2021-07-09T15:18:50Z

@Conduitry do you mean something like the following:
const relative_to_absolute = (function() {
    var anchor = document.createElement('a');
    return function(url) {
        anchor.href = url;
        return anchor.href;
    }
})();
I used the URL API since I assumed it would be faster but I did not actually check whether it is.

Has this actually been tested in IE11 ? I remember some weird bug that assigning href with a relative path would not turn it into an absolute url. You had to explicitly assign it again like a.href = a.href before returning it.

benmccann · 2021-07-09T15:32:12Z

There's not a huge impact if this doesn't work in IE11. They simply won't get the optimization. I don't think anyone would even notice. So I probably wouldn't let it hold up this PR

hbirler · 2021-07-09T19:17:38Z

@dominikg The polyfill library seems to not need the trick you described for IE8+ https://github.com/Financial-Times/polyfill-library/blob/master/polyfills/URL/polyfill.js#L344
Maybe you would need that for IE6?

dominikg · 2021-07-09T19:47:21Z

Maybe you would need that for IE6?

maybe. I remember it because it was a very strange thing way back when . Can't find references to it now and as Ben said if the worst thing that happens is IE users missing out on an optimization...

Conduitry · 2021-07-21T20:52:07Z

These have been released in 3.40.0 - thank you!

benmccann reviewed Jun 25, 2021

View reviewed changes

src/runtime/internal/dom.ts Outdated Show resolved Hide resolved

hbirler force-pushed the better-hydration-src branch 3 times, most recently from 73c5145 to 4521989 Compare June 27, 2021 05:56

hbirler mentioned this pull request Jun 29, 2021

{@html} hydration in head duplicates elements #6463

Closed

benmccann changed the title ~~Fix & improve hydration~~ [fix] hydration improvements Jun 30, 2021

OG84 mentioned this pull request Jul 1, 2021

Flash of unstyled content in dev mode sveltejs/kit#915

Closed

benmccann approved these changes Jul 4, 2021

View reviewed changes

benmccann requested review from tanhauhau and Conduitry July 4, 2021 13:08

benmccann linked an issue Jul 7, 2021 that may be closed by this pull request

HMR reorders content #6506

Closed

benmccann removed a link to an issue Jul 7, 2021

HMR reorders content #6506

Closed

hbirler added 9 commits July 8, 2021 14:47

Improve src equality checking during hydration

42b9156

Normalize src values before checking equality

Skip reordering of unclaimed nodes in hydration

34f9ffe

Fix failing tests

d8aceba

Set `claim_order` in claim_html_tag

Normale src urls

be00e03

Normalize both sides of the equality to be sure

Improve comments

dae7599

Implement proper src equality checking

1f2460d

Use imports

Optimize init_hydrate

8e69246

Do not create array from children if not needed

Implement fast path for init_hydrate binary search

a16bb39

Improve claim_text

2fbc2f8

Split an existing text node if we match the prefix

hbirler force-pushed the better-hydration-src branch from 4521989 to 2fbc2f8 Compare July 8, 2021 12:53

Use anchor instead of URL for relative to absolute path convertion

2647bfb

Use an anchor element's href attribute to convert relative paths to absolute paths

hbirler force-pushed the better-hydration-src branch from 8efda3b to 2647bfb Compare July 8, 2021 20:07

This comment has been minimized.

Sign in to view

Fix tree-shakability

ef025d9

hbirler force-pushed the better-hydration-src branch from 2cddb12 to ef025d9 Compare July 8, 2021 20:27

tidy

a43fb64

Conduitry merged commit ecbd96a into sveltejs:master Jul 21, 2021

benmccann mentioned this pull request Jul 21, 2021

FOUC on production sveltejs/kit#1892

Closed

dominikg mentioned this pull request Jan 4, 2023

src_url_equal is slow when dealing with large base64 encoded images #8164

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] hydration improvements #6449

[fix] hydration improvements #6449

hbirler commented Jun 25, 2021 •

edited

Loading

hbirler commented Jun 25, 2021

benmccann commented Jun 25, 2021

hbirler commented Jun 25, 2021

benmccann commented Jun 25, 2021

dummdidumm commented Jul 1, 2021

hbirler commented Jul 1, 2021

benmccann commented Jul 7, 2021 •

edited

Loading

hbirler commented Jul 7, 2021

benmccann commented Jul 7, 2021 •

edited

Loading

benmccann commented Jul 8, 2021

Conduitry commented Jul 8, 2021

hbirler commented Jul 8, 2021

hbirler commented Jul 8, 2021 •

edited

Loading

benmccann commented Jul 8, 2021

hbirler commented Jul 8, 2021 •

edited

Loading

benmccann commented Jul 8, 2021

hbirler commented Jul 8, 2021 •

edited

Loading

benmccann commented Jul 8, 2021

hbirler commented Jul 8, 2021

hbirler commented Jul 8, 2021

benmccann commented Jul 8, 2021 •

edited

Loading

This comment has been minimized.

dominikg commented Jul 9, 2021 •

edited

Loading

benmccann commented Jul 9, 2021

hbirler commented Jul 9, 2021

dominikg commented Jul 9, 2021 •

edited

Loading

Conduitry commented Jul 21, 2021

[fix] hydration improvements #6449

[fix] hydration improvements #6449

Conversation

hbirler commented Jun 25, 2021 • edited Loading

Potential issues:

hbirler commented Jun 25, 2021

benmccann commented Jun 25, 2021

hbirler commented Jun 25, 2021

benmccann commented Jun 25, 2021

dummdidumm commented Jul 1, 2021

hbirler commented Jul 1, 2021

benmccann commented Jul 7, 2021 • edited Loading

hbirler commented Jul 7, 2021

benmccann commented Jul 7, 2021 • edited Loading

benmccann commented Jul 8, 2021

Conduitry commented Jul 8, 2021

hbirler commented Jul 8, 2021

hbirler commented Jul 8, 2021 • edited Loading

benmccann commented Jul 8, 2021

hbirler commented Jul 8, 2021 • edited Loading

benmccann commented Jul 8, 2021

hbirler commented Jul 8, 2021 • edited Loading

benmccann commented Jul 8, 2021

hbirler commented Jul 8, 2021

hbirler commented Jul 8, 2021

benmccann commented Jul 8, 2021 • edited Loading

This comment has been minimized.

dominikg commented Jul 9, 2021 • edited Loading

benmccann commented Jul 9, 2021

hbirler commented Jul 9, 2021

dominikg commented Jul 9, 2021 • edited Loading

Conduitry commented Jul 21, 2021

hbirler commented Jun 25, 2021 •

edited

Loading

benmccann commented Jul 7, 2021 •

edited

Loading

benmccann commented Jul 7, 2021 •

edited

Loading

hbirler commented Jul 8, 2021 •

edited

Loading

hbirler commented Jul 8, 2021 •

edited

Loading

hbirler commented Jul 8, 2021 •

edited

Loading

benmccann commented Jul 8, 2021 •

edited

Loading

dominikg commented Jul 9, 2021 •

edited

Loading

dominikg commented Jul 9, 2021 •

edited

Loading