Bubble up Component dom changes up the virtual tree (+51 B) #1700

andrewiggins · 2019-06-10T20:13:11Z

If a component changed it's DOM node and was wrapped in other Components, the parent Components _dom and .base pointer could become out of date. Since this.base is a public API feature on Components, I figured this need to be fixed.

This might slightly decrease our memory footprint as we won't have pointers to useless DOM nodes anymore. Also fixes some cases where we would needlessly append DOM nodes to parents that are no longer mounted (see comment in fragments.test.js)

~~Total change: +36 B 😐~~
~~Total change: +48 B 😕~~
Total change: +51 B 😕

In some situations an updating component could cause it's parent's _lastDomChild to become stale and point to a no longer mounted DOM node. This change eagerly cleans up _lastDomChild to prevent stale pointers since we don't need this pointer after its value is read in diffChildren.

andrewiggins · 2019-06-10T20:14:02Z

test/browser/fragments.test.js

@@ -2575,7 +2575,6 @@ describe('Fragment', () => {
 			div('B')
 		].join(''), 'updateB');
 		expectDomLogToBe([
-			// TODO: Extra appends happen here in actual. Why?


These spurious appends no longer happen!

andrewiggins · 2019-06-10T20:15:08Z

src/diff/children.js

+					// Eagerly cleanup _lastDomChild. We don't need to persist the value because
+					// it is only used by `diffChildren` to determine where to resume the diff after
+					// diffing Components and Fragments.
+					childVNode._lastDomChild = null;


Instead of trying to keep _lastDomChild up to date, I figured it was better to just clean up the pointer after we used it and no longer need it to reduce the likelihood of having out of date pointers to DOM nodes no longer in the DOM.

coveralls · 2019-06-10T20:17:31Z

Coverage increased (+0.9%) to 99.782% when pulling e12a477 on fix-cbase into a19aee9 on master.

JoviDeCroock

This is looking really good, I have to ask though was this issue occurring already in scenario's? Or is this something that is part of the previous PR?

Great work as always

andrewiggins · 2019-06-10T21:58:11Z

I believe this issue was already occurring. I think it has always been broken in Preact X actually. However, I don't think c.base is a well known or often used property (though we use it at work as a forwardRef cheat).

I checked out 699111d (the PR before my _children refactor) and 1566a31 (alpha.1) and the tests fail in both those commits so it's been with us for sometime.

marvinhagemeister · 2019-06-11T07:08:30Z

Can confirm that we always had this issue in X (maybe even in 8.x?). This PR is the missing piece for our new Fragment algorithm 👍 We may revisit it when we'll work on 2-phase render, but for now this is the best we can do for the current architecture 💯

marvinhagemeister · 2019-06-11T07:13:51Z

src/component.js

+		if (newDom != oldDom) {
+			// Update parent component's _dom and c.base pointers
+			while (
+				vnode._parent


Are there cases where the _parent pointer can be null? If that's the case for root nodes we could try using our EMPTY_OBJ ref for that. Maybe that could save a byte or two 🎉

Oh true! I'll investigate

test/browser/components.test.js

test/browser/fragments.test.js

This reverts commit 36326c2.

marvinhagemeister

So many new tests 😍 Let's do this 🎉💯🚀

marvinhagemeister · 2019-06-12T08:07:37Z

test/browser/components.test.js

+		expect(parentDom1.base).to.equalNode(scratch.firstChild);
+	});
+
+	it('should update parent c.base if first child becomes null', () => {


Had a look into this and it seems like sometimes the _dom pointer of components is not set properly. When I log the vnode in unmount() _dom is null whereas c.base points to the correct DOM node.

Yea, when looking at your comment about a missing test scenario, I realized a whole bunch of edge cases I didn't handle lol. Still working on it

…tests

andrewiggins · 2019-06-13T01:57:08Z

compat/src/suspense.js

-				removeDom(child._children);
-			}
-			if (child._dom) {
+			if (typeof child.type !== 'function' && child._dom) {


@sventschui FYI, this PR enables the removeDom function to now only remove the first DOM children parents under Suspense, instead of recursing all the way down the tree and removing all the nodes bottom up. Should be a bit faster now for deep trees!

Awesome 🎉

andrewiggins · 2019-06-13T02:02:46Z

src/component.js

+		if (newDom != oldDom) {
+			updateParentDomPointers(vnode);
+
+			// // 3496 B


TODO: Remove all the commented code before checking in. Wanted to first give people an idea of some of the golfing techniques I tried.

P.S. Surprised this iterative approach isn't smaller than the recursive function:

Minified:

// Iterative: 3496 B for (; null != (o = o.__p) && null != o.__c; ) for (o.__e = o.__c.base = null, r = 0; r < o.__k.length; r++) if (null != (i = o.__k[r]) && null != i.__e) { o.__e = o.__c.base = i.__e; break; }

// Recursive: 3484 B function m(n) { var l, u; if (null != (n = n.__p) && null != n.__c) { for (n.__e = n.__c.base = null, l = 0; l < n.__k.length; l++) if (null != (u = n.__k[l]) && null != u.__e) { n.__e = n.__c.base = u.__e; break; } return m(n); } }

Do we have some guidelines about bytes saving vs. function call overhead? Asking for a friend 😂

I've seen a bench (assuming it can be trusted 🤷‍♂) where in one situation recursion was faster than the iterative solution, so I typically go for the byte savings first on something like this, especially since this code is only called once per update.

I think it would be hard to measure the perf difference between an iterative solution and recursive in this case since it isn't called very often in a diff (as opposed to the children loop in diffChildren which is called for every VNode, for example).

But that's just me. I don't spend as much time doing perf tests as @developit or others, so I defer to them.

JoviDeCroock

I'd say let's remove the comments and get this into master since it looks awesome!

marvinhagemeister

Agree with @JoviDeCroock 👍 💯 🚀

andrewiggins · 2019-06-14T16:12:26Z

I'll clean it up and merge it today

With #1700 complete, we can now rely on the sibling._dom pointer to tell us if that subtree contains a rendered DOM node and what the first dom is.

andrewiggins added 11 commits June 10, 2019 13:06

Add failing test for nested c.base

37d6c30

Fix out of date c.base pointers (+28 B)

9df48dc

Update c.base test to verify sibling doesn't change

e0d05a7

Only update c.base if component is first child of parent (+12 B)

b0d3045

Add note about out of date _lastDomChild

357ca4c

Add test for child that is not first child

3f3de03

Update Fragment tests todos

f8f5e27

Add test for failing c.base update with null first children

b5c70e2

Simplify condition and fix children position check (-7 B)

6f97524

Fix c.base condition to update root Fragment (+3 B)

fe4f876

andrewiggins commented Jun 10, 2019

View reviewed changes

JoviDeCroock approved these changes Jun 10, 2019

View reviewed changes

marvinhagemeister reviewed Jun 11, 2019

View reviewed changes

test/browser/components.test.js Show resolved Hide resolved

marvinhagemeister reviewed Jun 11, 2019

View reviewed changes

test/browser/fragments.test.js Show resolved Hide resolved

andrewiggins added 3 commits June 11, 2019 14:42

Add more failing tests for c.base

e928a90

Experiment with findDomNode implementation of c.base

36326c2

Revert "Experiment with findDomNode implementation of c.base"

8568bd0

This reverts commit 36326c2.

marvinhagemeister approved these changes Jun 12, 2019

View reviewed changes

This was referenced Jun 12, 2019

[10.0.0-beta.2] Dom order incorrect after state update #1699

Closed

10.0.0-beta.1 - Incorrect DOM ordering of elements #1652

Closed

marvinhagemeister reviewed Jun 12, 2019

View reviewed changes

andrewiggins added 3 commits June 12, 2019 12:33

Reorganize c.base tests into common describe and improve last couple …

2eaf208

…tests

Add another c.base failing test

7af00d8

Recursively update DOM pointers handling null first children (+36 B)

6385ee0

andrewiggins added 4 commits June 12, 2019 15:58

Fix suspense catching and removal (+0 B)

c8ceb53

Improve toEqualNode null assertion

1649d76

Golf updateParentDomPointers (-20 B)

2d3a8a5

Merge branch 'master' into fix-cbase

3ccf2a5

andrewiggins requested review from marvinhagemeister and JoviDeCroock June 13, 2019 01:52

andrewiggins commented Jun 13, 2019

View reviewed changes

JoviDeCroock approved these changes Jun 14, 2019

View reviewed changes

marvinhagemeister approved these changes Jun 14, 2019

View reviewed changes

andrewiggins added 2 commits June 14, 2019 16:31

Remove failed golf attempts

2d08b3b

Merge branch 'master' into fix-cbase

e12a477

andrewiggins changed the title ~~Bubble up Component dom changes up the virtual tree (+36 B)~~ Bubble up Component dom changes up the virtual tree (+51 B) Jun 14, 2019

andrewiggins merged commit 3e3f3f9 into master Jun 14, 2019

andrewiggins deleted the fix-cbase branch June 14, 2019 23:41

marvinhagemeister mentioned this pull request Jun 16, 2019

10.0.0-beta.1 Incorrect DOM Ordering #1635

Closed

andrewiggins added a commit that referenced this pull request Jun 25, 2019

Simplify getDomSibling (-12 B)

acbebbc

With #1700 complete, we can now rely on the sibling._dom pointer to tell us if that subtree contains a rendered DOM node and what the first dom is.

andrewiggins mentioned this pull request Jun 26, 2019

Golf sibling DOM handling #1737

Merged

snyk-bot mentioned this pull request Oct 26, 2019

[Snyk] Upgrade preact from 8.5.2 to 10.0.1 ajesse11x/now-examples#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bubble up Component dom changes up the virtual tree (+51 B) #1700

Bubble up Component dom changes up the virtual tree (+51 B) #1700

andrewiggins commented Jun 10, 2019 •

edited

Loading

andrewiggins Jun 10, 2019

andrewiggins Jun 10, 2019 •

edited

Loading

coveralls commented Jun 10, 2019 •

edited

Loading

JoviDeCroock left a comment

andrewiggins commented Jun 10, 2019

marvinhagemeister commented Jun 11, 2019

marvinhagemeister Jun 11, 2019

andrewiggins Jun 11, 2019

marvinhagemeister left a comment

marvinhagemeister Jun 12, 2019

andrewiggins Jun 12, 2019 •

edited

Loading

andrewiggins Jun 13, 2019

sventschui Jun 13, 2019

andrewiggins Jun 13, 2019

sventschui Jun 13, 2019

andrewiggins Jun 14, 2019

JoviDeCroock left a comment

marvinhagemeister left a comment

andrewiggins commented Jun 14, 2019

Bubble up Component dom changes up the virtual tree (+51 B) #1700

Bubble up Component dom changes up the virtual tree (+51 B) #1700

Conversation

andrewiggins commented Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

andrewiggins Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jun 10, 2019 • edited Loading

JoviDeCroock left a comment

Choose a reason for hiding this comment

andrewiggins commented Jun 10, 2019

marvinhagemeister commented Jun 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marvinhagemeister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewiggins Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoviDeCroock left a comment

Choose a reason for hiding this comment

marvinhagemeister left a comment

Choose a reason for hiding this comment

andrewiggins commented Jun 14, 2019

andrewiggins commented Jun 10, 2019 •

edited

Loading

andrewiggins Jun 10, 2019 •

edited

Loading

coveralls commented Jun 10, 2019 •

edited

Loading

andrewiggins Jun 12, 2019 •

edited

Loading