Fast hydration & FIX iframes hydration #4309

avi · 2020-01-23T10:48:59Z

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

It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

Run the tests tests with npm test or yarn test)

Solution to #4308

Reuse dom nodes in hydration avoid insertBefore/append dom nodes
Better performance in hydration which is now slower than client side render and avoid reloading iframes...

…ation

…cretly failing

yotambarzilay · 2020-02-09T19:21:27Z

@Conduitry @Rich-Harris
any chance for this PR to get reviewed?

devinrhode2 · 2020-02-12T17:09:27Z

These commits should obviously be rebased. At least a vanilla git rebase should do away with all the merge commits. The "remove comment" and "removed console.log" probably should be rebased away. (btw, this is how I personally would perform that particular "rebase" https://gist.github.com/devinrhode2/1dea188a754029c363b85ee2b13acd1c

sandrina-p · 2020-02-13T11:06:32Z

This would also fix (probably) sveltejs/sapper#1088!

Edit: I just tested the fork and it fixed the hydratation problem! 😍

…cretly failing

…ation

antony · 2020-03-18T14:31:56Z

test/hydration/samples/iframe/_config.js

+		});
+
+		assert(iframeMutations.length === 0, 'iframe added/removed');
+		// assert(false, 'test')


Probably comments should be cleared up.

antony · 2020-03-18T14:34:24Z

test/hydration/samples/binding-input/_config.js

@@ -3,6 +3,8 @@ export default {
 		name: 'world'
 	},

+	skip: true,


Are these tests being skipped?

A couple of tests are skipped - the tests we're written as if their test harness was async but it wasn't and when I wanted to write a test that actually had to be async - the tests failed when I changed the test harness to be async... the change that broke the tests I skipped was to test/hydration/index.js

antony · 2020-03-18T14:35:56Z

@avi thanks for this - I've done an initial pre-review (as best I can) and I'll see if somebody who is more familiar with this part of the code can do a fuller review.

There were just a couple of bits that confused me so I've added comments.

tanhauhau

first of all, @avi thanks for the change.

i think this MR tries to change too many things:

skip removing attribute during hydration (which i think its a breaking change)
upgrading typescript (probably do it in a separate mr?)
skip appendChild if is hydrating (i think this is the one that fix the iframe hydration reloading bug)
- if claiming will insert the element into the DOM, i think we can skip mounting if hydrate right?
fix hydration of text nodes (i think can do it in separate PR too)

avi · 2020-03-19T13:22:33Z

I know skipping removing attributes is a breaking change, I wanted feedback whether to make my changes behind a compiler flag or not... But the current state of hydration is worse than resetting the innerHtml of the mount point by target.innerHTML = '' and doing client side rendering.
Resetting the attributes causes the src of the iframe to be removed and re-added which also causes the iframe content to reload, it also causes all the bugs with css animations replaying on hydration.
Regarding the skipping of appendChild the old code grabbed DOM nodes regardless of their order (only checking type) which kinda forces the appendChild to be used to reinsert them back in order.
Mounting is still required to attach event handlers etc
I can revert the typescript change
The rest is kinda the atomic unit of change that solves the bug and doesn't introduce new ones.

avi · 2020-03-25T13:38:15Z

Reverted the typescript bump and resolved a conflict

benmccann · 2020-06-01T22:46:29Z

src/runtime/internal/dom.ts

+			if (n.nodeType === 3) {
+				return claim_text(nodes, (n as Text).data);
+			 } else {
+				return claim_element(nodes, n.nodeName, n, n.namespaceURI !== 'http://www.w3.org/1999/xhtml');


'http://www.w3.org/1999/xhtml' could be replaced with namespaces.html:

svelte/src/compiler/utils/namespaces.ts

Line 1 in 767ce22

export const html = 'http://www.w3.org/1999/xhtml';

benmccann · 2020-06-01T22:48:16Z

src/runtime/internal/dom.ts

+		this.n = this.n.map(n => {
+			if (n.nodeType === 3) {
+				return claim_text(nodes, (n as Text).data);
+			 } else {


it looks like this line and the closing curly brace two lines later both have an extra space

kjmph · 2020-10-30T15:53:23Z

Is there anything that can be done to help move this PR along? Some of the small comments could be fixed fairly quickly.

benmccann · 2020-10-30T16:08:21Z

It's probably not that easy. There's more discussion on the linked issue: #4308

jonatansberg · 2020-10-30T16:12:22Z

Yeah, I tried to rebase this the other day, and a whole lot of tests are failing.

benmccann · 2020-10-30T16:18:13Z

When I said it wouldn't be that easy, I meant that just rebasing this PR probably wouldn't be enough to get it checked in since there's not agreement that this is the right approach. I wasn't commenting on how hard or easy it would be to rebase and didn't expect any particular difficulties there.

benmccann · 2020-10-30T16:22:46Z

I just saw your comment on the issue from yesterday - yeah if you can do it without breaking changes that seems more likely to be accepted to me - but it might be good to ask some other folks as well to get sign-off on the approach before sinking too much time

jonatansberg · 2020-10-30T17:44:41Z

@benmccann Yeah, sorry, my comment above was a bit inexplicit. Currently on my phone..

What I meant to say above was that even just getting this to a point where everything works (with or without rebasing) would still require more work. The whole debate of the suitability of this approach notwithstanding.

I’m trying to get a slight variation of the approach taken by @avi to work, but I keep running in to issues.

benmccann · 2021-04-21T16:44:08Z

Addressed in #6204. Thanks!!

avi and others added 6 commits January 22, 2020 20:36

fast hydration

abd5551

added test that fails on master

bd42d64

Merge branch 'master' into fast-hydration

f927fc7

removed console.log

f0ccaab

Merge branch 'fast-hydration' of github.com:avi/svelte into fast-hydr…

a0127ca

…ation

skipping two tests that relied on async test in hydration and were se…

017d0db

…cretly failing

avi requested a review from Rich-Harris January 23, 2020 15:38

avi changed the title ~~Fast hydration~~ Fast hydration & FIX iframes hydration Jan 26, 2020

avi requested a review from Conduitry January 26, 2020 10:46

avi added 5 commits January 29, 2020 11:20

Merge branch 'master' into fast-hydration

595262c

finished merge

4fd40ad

Merge remote-tracking branch 'upstream/master' into fast-hydration

7b4d049

updated typescript

0e3e55e

Merge remote-tracking branch 'upstream/master' into fast-hydration

5f1bbd6

remove comment

f92d463

sandrina-p mentioned this pull request Feb 13, 2020

[Bug] Hydratation issue causes CSS animations to restart sveltejs/sapper#1088

Closed

avi added 9 commits February 16, 2020 11:28

Merge remote-tracking branch 'upstream/master' into fast-hydration

2f75fe9

fast hydration

a4ac609

added test that fails on master

9dadb50

removed console.log

231c3e5

skipping two tests that relied on async test in hydration and were se…

2451774

…cretly failing

finished merge

5bb1876

updated typescript

ca46753

remove comment

a179d6f

Merge branch 'fast-hydration' of github.com:avi/svelte into fast-hydr…

5e0ac70

…ation

antony reviewed Mar 18, 2020

View reviewed changes

tanhauhau requested changes Mar 19, 2020

View reviewed changes

avi and others added 3 commits March 25, 2020 15:33

Merge branch 'master' into fast-hydration

124dcdb

revert typescript bump

d46e1b3

forgot to pull before reset package.json and lock file

9e2b070

tanhauhau added the hydration label May 17, 2020

benmccann reviewed Jun 1, 2020

View reviewed changes

benmccann mentioned this pull request Jun 11, 2020

SPA mode sveltejs/sapper#383

Closed

benmccann mentioned this pull request Jun 21, 2020

<script>'s in <svelte:head> are running twice (unless wrapped in {@html}) sveltejs/sapper#1124

Open

jonatansberg mentioned this pull request Oct 31, 2020

Improve ssr hydration performance #5623

Closed

1 task

benmccann mentioned this pull request Apr 21, 2021

Improve SSR hydration performance #6204

Merged

benmccann closed this Apr 21, 2021

ankitstarski mentioned this pull request Jun 26, 2021

Improve hydration by reordering optimally #6395

Merged

benmccann mentioned this pull request Mar 8, 2022

Partial hydration sveltejs/kit#1390

Open

benmccann mentioned this pull request Nov 27, 2023

Better hydration #4308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fast hydration & FIX iframes hydration #4309

Fast hydration & FIX iframes hydration #4309

avi commented Jan 23, 2020 •

edited

Loading

yotambarzilay commented Feb 9, 2020

devinrhode2 commented Feb 12, 2020

sandrina-p commented Feb 13, 2020 •

edited

Loading

antony Mar 18, 2020

antony Mar 18, 2020

avi Mar 18, 2020

antony commented Mar 18, 2020

tanhauhau left a comment •

edited

Loading

avi commented Mar 19, 2020

avi commented Mar 25, 2020

benmccann Jun 1, 2020

benmccann Jun 1, 2020

kjmph commented Oct 30, 2020

benmccann commented Oct 30, 2020

jonatansberg commented Oct 30, 2020

benmccann commented Oct 30, 2020 •

edited

Loading

benmccann commented Oct 30, 2020

jonatansberg commented Oct 30, 2020

benmccann commented Apr 21, 2021

Fast hydration & FIX iframes hydration #4309

Fast hydration & FIX iframes hydration #4309

Conversation

avi commented Jan 23, 2020 • edited Loading

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

Tests

yotambarzilay commented Feb 9, 2020

devinrhode2 commented Feb 12, 2020

sandrina-p commented Feb 13, 2020 • edited Loading

antony Mar 18, 2020

Choose a reason for hiding this comment

antony Mar 18, 2020

Choose a reason for hiding this comment

avi Mar 18, 2020

Choose a reason for hiding this comment

antony commented Mar 18, 2020

tanhauhau left a comment • edited Loading

Choose a reason for hiding this comment

avi commented Mar 19, 2020

avi commented Mar 25, 2020

benmccann Jun 1, 2020

Choose a reason for hiding this comment

benmccann Jun 1, 2020

Choose a reason for hiding this comment

kjmph commented Oct 30, 2020

benmccann commented Oct 30, 2020

jonatansberg commented Oct 30, 2020

benmccann commented Oct 30, 2020 • edited Loading

benmccann commented Oct 30, 2020

jonatansberg commented Oct 30, 2020

benmccann commented Apr 21, 2021

avi commented Jan 23, 2020 •

edited

Loading

sandrina-p commented Feb 13, 2020 •

edited

Loading

tanhauhau left a comment •

edited

Loading

benmccann commented Oct 30, 2020 •

edited

Loading