Consistently use `_children` & combine Fragment and Component diffing (-73 B) by andrewiggins · Pull Request #1658 · preactjs/preact

andrewiggins · 2019-05-29T18:55:42Z

Summary

~~NOTE: this might be a good candidate for merging after we release 10.0. Given the size of the change, it may not be worth de-stabilizing master with more changes. Thoughts?~~ I might need this change to improve #1605, so perhaps it would be worthwhile to get in soon. Still investigating.

Replace component._prevVNode with vnode._children
Combine Fragment and Component diffing

Details

Replace `component._prevVNode` with `vnode._children`

Previously, _children was a cache for a normalized props.children. With a DOM VNode and Fragment VNode, you could access the children of the VNodes on the this _children property, but on VNodes for components, you need to go through vnode._component._preVNode.

This PR formalizes the concept behind _children. It is now the container for the rendered children of all VNodes. If you think of props.children as input to render functions, _children is the output of the render. And DOM VNodes and Fragments are just render functions that return props.children. The implementation is a little different (DOM VNodes don't have a render function, per se), but this concept applies.

Combine Fragment and Component diffing

Previously, if diff was called with a Component, it would call c.render, coerce the result to a single VNode (e.g. convert arrays to a Fragment), and then call diff. However, building on the _children change (and the concept mentioned there), we now instead treat Components as always returning arrays, and coerce result of render into a children of VNodes to continue rendering. So, diff always calls diffChildren after diff'ing a VNode, regardless of its type. Another way to think about this is that everything is Fragment now 😄

Being able to depend on diffChildren -> diff -> diffChildren for all VNodes allows us to do operations that require knowing the parentVNode in diffChildren without having to special case the diff -> diff situation. For example, there is now only one place where oldVNode matching happens: diffChildren (we can remove some code from the top of diff). Further, mounting and unmounting DOM only happens in diffChildren as well (if unmounting wasn't recursive, we could inline it).

Future Considerations

With this PR, their are only 2 place diff is called: diffChildren and forceUpdate. If we can change forceUpdate to also enter the diff through diffChildren, then it is conceivable that we could inline diff into diffChildren and make our diff a single function! I think this could give us some good byte savings by sharing more local variables and reducing duplicate structures (e.g. have fewer than 5 try {} catch {} like we currently do). It may reduce the approachability of our code, but perhaps we can mitigate that with a well-defined sectional comment format to make up for the lost function names. Either way, I think it might be worth an exploration.

Breaking change

There are some breaking changes in this PR.

Component refs aren't re-invoked after each render. Preact v8 and v10 both re-invoke component refs on every render. This PR changes that behavior so they are only invoked on initial render, when the ref changes, or on unmount. This new behavior appears to match React.

Per change summary

91f043b Only copy _dom from oldVNode to newVNode if necessary (-1 B)
3159b71 Replace c._prevVNode with vnode._children (-2 B)
b73ab00 Call diffChildren after rendering components (-8 B)
b09318f Copy the oldVNode in forceUpdate to prevent side effect modification (+7 B)
dc86517 Set vnode._dom pointer in diffChildren (+2 B)
fe7f20d Only remove excessDomChildren after diff'ing DOM VNodes (+5 B)
6f06653 Remove redundant vnode type checks in diff (-17 B)
40212eb Apply refs after unmounting in diffChildren (+25 B)
910f98c Combine Fragment and Component diffing (-60 B)
27116bc Remove Array coercion from coerceToVNode (-8 B)
6b18f6f Remove more null checks from diff (-13 B)
c0c070e Normalize oldVNode to EMPTY_OBJ once (-2 B)
9d89d10 Fix Fragment _dom pointers (-10 B)
3470318 Flatten top-level Fragments returned from render (+21 B)
31036a3 Remove unused append in forceUpdate (-14 B)

~~Total change: -70 B 😁~~
~~Total change: -78 B 😁~~
~~Total change: -57 B 😃~~
Total change: -73 B 😁

NOTE: Individual commit size diffs may not be exact due to drifting caused by merges from master. They are intended to give an estimate of relative cost.

…n (+7 B)

To fix hydration tests

Since `diffChildren` is always called after `diff`, `diffChildren` is the only place where VNodes are matched against oldVNodes

Note: this introduces a breaking change where component refs aren't re-invoked every render

This code had 0 code coverage. It appears that toChildArray flattens all nested arrays before they go to coerceToVNode as arrays...

andrewiggins · 2019-05-29T18:56:08Z

 						options: {
-							comments: false,
-							compact: true,
+							// comments: false,


Note to self: Undo before checking in

I'm wondering if we should remove these options from our karma config. Last time I checked they were breaking sourcemaps and thus any breakpoints.

I accidentally left this in (whoops). If this causes problems, we can undo it.

andrewiggins · 2019-05-29T18:56:19Z

    "build:test-utils": "microbundle build --raw --cwd test-utils",
    "build:compat": "microbundle build --raw --cwd compat --globals 'preact/hooks=preactHooks'",
-    "dev": "microbundle watch --raw --format cjs,umd",
+    "dev": "microbundle watch --raw --format cjs",


Note to self: Undo before checking in

Whoops, forgot this one too 🤦‍♂. When comparing sizes I usually just look at cjs (for no real reason, it's just first in the list), so I remove the umd cuz it just slows down the build. If there is a reason to keep both, happy to undo and change my habits lol

oh no 👀

I think it was for direct usage with unpkg

andrewiggins · 2019-05-29T18:58:10Z

 		return createVNode(null, possibleVNode, null, null);
 	}

-	if (Array.isArray(possibleVNode)) {


This change should be okay since toChildArray is always flattens nested arrays before coerceToVNode is called, but it does make me a little nervous. We don't have any tests that used these lines so I think they are safe to remove.

andrewiggins · 2019-05-29T19:16:59Z

 		ref.resetHistory();
 		render(<Foo ref={ref} />, scratch);
-		expect(ref).to.have.been.calledOnce;
+		expect(ref).not.to.have.been.called;


These test modifications demonstrate the breaking change

The only place `diff` is called not inside `diffChildren` (which already does proper null checking) is `forceUpdate`. `forceUpdate` will never pass in a null newVNode or oldVNode cuz it always passes the VNode that represents the component. If the component is in the tree, it can't have a null VNode (or else it doesn't exist in the tree. It can render null, but that is handled by `diffChildren`).

JoviDeCroock · 2019-05-29T19:40:38Z

This is more a question but does this solve the issues added in: #1639

andrewiggins · 2019-05-29T19:44:02Z

 * Fragments that have siblings. In most cases, it starts out as `oldChildren[0]._dom`.
 */
 export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChildren, mounts, ancestorComponent, force, oldDom) {
-	// If the previous type doesn't match the new type we drop the whole subtree


These checks are not necessary when diff is always called by diffChildren (there is one exception, forceUpdate, discussed below). diffChildren already does proper null checking on newVNode and passes in EMPTY_OBJ if oldVNode is null. diffChildren will never pass two vnodes of non-equal types. It will instead always pass null if it can't find a matching oldVNode for newVNode.

The only place diff is called not inside diffChildren is forceUpdate. However, forceUpdate always calls diff with non-null vnodes of the same type. It will never pass in a null newVNode or oldVNode cuz it always passes the VNode that represents the component. If the component is in the tree, it can't have a null VNode or else it doesn't exist in the tree. It can render null, but that is handled by diffChildren.

In other words, all of this logic is duplicated in diffChildren. Since we now call always diffChildren after diff, we no longer need to duplicate that logic here.

marvinhagemeister · 2019-06-01T04:56:29Z

Finally had a chance to read through the changes in depth. Merging _prevVNode and _children is such a relief. It was causing me a lot of trouble with the devtools and always using _children makes it so much simpler 🎉

Removing the special diff -> diff case in particular is very exciting! Making every component a Fragment is a more sensible path forward and should help us normalize more things 👍 I'll need to have a closer look again after JSConf is over, but I'm hoping that it will help us find a better solution to the sibling issue #1605 .

The forceUpdate stuff can be postponed to another PR in my opinion. It may take a bit of thought to get it right too. Although I'm confident that this PR will make it a lot easier. I think you have a really good handle on that :)

Regarding refs: Calling them on every render seems unnecessary. I'm with you that just calling them when something changes is sufficient 💯

Overall this PR is (like your others) top notch and the creme de la creme. It's just that good 👍 👍

coveralls · 2019-06-01T07:18:34Z

Coverage decreased (-0.002%) to 99.775% when pulling a665861 on vnode-prop-explorations into 896e77d on master.

robertknight · 2019-06-01T08:52:54Z

Nice work! I'm not as deeply familiar with the diffing logic as other devs but conceptually it looks like a good simplification. Assuming this lands before the next beta, I will need to update the Enzyme adapter accordingly, as it accesses the rendered children of vnodes.

JoviDeCroock

This looks awesome Andre, it's a real pleasure to read through these changes! BIG BIG props to you 👍

andrewiggins · 2019-06-03T00:16:17Z

 		let mounts = [];
-		dom = diff(parentDom, vnode, vnode, this._context, parentDom.ownerSVGElement!==undefined, null, mounts, this._ancestorComponent, force, dom);
-		if (dom!=null && dom.parentNode!==parentDom) {
-			parentDom.appendChild(dom);


Removed the appendChild call here because code coverage was complaining (showing this line was never hit), which I take to mean that either it is no longer needed, or that we are missing a test case to justify it's purpose. However, since #1605 is going to completely rewrite this code anyway, I'll just remove this now to keep coveralls happy.

marvinhagemeister

This PR is so good!! 👍 👍

JoviDeCroock · 2019-06-04T14:28:18Z

The maxDuration line in compat/suspense seems to have lost its hit

marvinhagemeister · 2019-06-04T14:59:01Z

@JoviDeCroock I'm running into this even on master. Not sure how it got in :S

/cc @sventschui

JoviDeCroock · 2019-06-04T15:00:07Z

@marvinhagemeister maybe the coverall settings reset again and it's showing branch coverage or because of an update in istanbul maybe :/

sventschui · 2019-06-04T21:45:09Z

I guess maxDuration was never tested. I don‘t know how to test this in mocha. I know jest supports mocking timers :/

I‘m in a university seminar this week. Can look into this next week ✌️

andrewiggins · 2019-06-04T22:37:09Z

I think coveralls is failing cuz I changed the number of lines in suspense.js, so the one line that wasn't covered caused a tiny percentage change cuz it is now a greater percentage of lines (since the total number of lines is smaller).

I think we can proceed without coveralls approval since it doesn't actually represent a change in functional coverage.

Are people okay if I merge this?

- The `component._prevVNode` VNode property containing the rendered output of a component was replaced by lookups through `component._vnode._children`, which is always an array. - A `Fragment` is now simply a regular function component which returns `props.children`, so we need to test for whether the node is a fragment before testing whether it is a custom component - Handle the possibility of `vnode.props` being `null`. It doesn't appear that is actually possible from the code, but the current Preact 10 upstream typings specify that props may be an object, a string/number (this definitely can happen) or null See preactjs/preact#1658 for additional details.

andrewiggins added 20 commits May 29, 2019 09:59

Only run core tests

c2a6398

Only copy _dom from oldVNode to newVNode if necessary (-1 B)

91f043b

Replace c._prevVNode with vnode._children (-2 B)

3159b71

Fix core test teardown helper

7cbe540

Call diffChildren after rendering components (-8 B)

b73ab00

Copy the oldVNode in forceUpdate to prevent side effect modificatio…

b09318f

…n (+7 B)

Set vnode._dom pointer in diffChildren (+2 B)

dc86517

Only remove excessDomChildren after diff'ing DOM VNodes (+5 B)

fe7f20d

To fix hydration tests

Remove redundant vnode type checks in diff (-17 B)

6f06653

Since `diffChildren` is always called after `diff`, `diffChildren` is the only place where VNodes are matched against oldVNodes

Add NOTES and TODOs to describe goals and intentions of branch

dcd5e3f

Add tests verifying cDU & cDM are invoked after refs are set

6ae3ff2

WIP: attempt to fix refs...

ba91356

Apply refs after unmounting in diffChildren (+25 B)

40212eb

Note: this introduces a breaking change where component refs aren't re-invoked every render

Combine Fragment and Component diffing (-60 B)

910f98c

Update TODOs

c1a8faf

Remove Array coercion from coerceToVNode (-8 B)

27116bc

This code had 0 code coverage. It appears that toChildArray flattens all nested arrays before they go to coerceToVNode as arrays...

Remove unused exports

2c92a2f

Enable all non-core tests

6732860

Updated NOTES

d900fc2

Add note about failing core test

9c0bd1c

andrewiggins requested review from developit and marvinhagemeister May 29, 2019 18:55

Remove Notes

d715b45

andrewiggins force-pushed the vnode-prop-explorations branch from 55b3de1 to d715b45 Compare May 29, 2019 19:00

andrewiggins commented May 29, 2019

View reviewed changes

andrewiggins changed the title ~~Consistently use _children & combine Fragment and Component diff'ing (-57 B)~~ Consistently use _children & combine Fragment and Component diff'ing (-70 B) May 29, 2019

andrewiggins commented May 29, 2019

View reviewed changes

cristianbote reviewed May 30, 2019

View reviewed changes

Comment thread src/diff/children.js Outdated

Flatten top-level Fragments returned from render (+21 B)

3470318

andrewiggins changed the title ~~Consistently use _children & combine Fragment and Component diff'ing (-70 B)~~ Consistently use _children & combine Fragment and Component diffing (-57 B) Jun 1, 2019

Remove TODOs

3686f26

andrewiggins marked this pull request as ready for review June 1, 2019 07:09

JoviDeCroock approved these changes Jun 1, 2019

View reviewed changes

sventschui approved these changes Jun 1, 2019

View reviewed changes

andrewiggins added 3 commits June 2, 2019 16:01

Remove unused append in forceUpdate (-14 B)

31036a3

Merge branch 'master' into vnode-prop-explorations

6dd8194

Fix linting error

419d2e6

andrewiggins commented Jun 3, 2019

View reviewed changes

marvinhagemeister approved these changes Jun 4, 2019

View reviewed changes

Merge branch 'master' into vnode-prop-explorations

85797a7

Merge branch 'master' into vnode-prop-explorations

a665861

andrewiggins changed the title ~~Consistently use _children & combine Fragment and Component diffing (-57 B)~~ Consistently use _children & combine Fragment and Component diffing (-73 B) Jun 5, 2019

andrewiggins merged commit b8c0e87 into master Jun 5, 2019

andrewiggins deleted the vnode-prop-explorations branch June 5, 2019 22:57

andrewiggins mentioned this pull request Jun 6, 2019

Begin diff with next DOM sibling in forceUpdate #1689

Merged

robertknight mentioned this pull request Jun 10, 2019

Add support for VNode tree changes coming in next Preact 10 release preactjs/enzyme-adapter-preact-pure#55

Merged

1 task

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

andrewiggins mentioned this pull request Nov 7, 2019

Move renderCallbacks to VNode and queue refs as renderCallbacks #2098

Closed

Uh oh!

Conversation

andrewiggins commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Replace component._prevVNode with vnode._children

Combine Fragment and Component diffing

Future Considerations

Breaking change

Per change summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoviDeCroock commented May 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marvinhagemeister commented Jun 1, 2019

Uh oh!

coveralls commented Jun 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertknight commented Jun 1, 2019

Uh oh!

JoviDeCroock left a comment

Choose a reason for hiding this comment

Uh oh!

andrewiggins Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marvinhagemeister left a comment

Choose a reason for hiding this comment

Uh oh!

JoviDeCroock commented Jun 4, 2019

Uh oh!

marvinhagemeister commented Jun 4, 2019

Uh oh!

JoviDeCroock commented Jun 4, 2019

Uh oh!

sventschui commented Jun 4, 2019

Uh oh!

andrewiggins commented Jun 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

andrewiggins commented May 29, 2019 •

edited

Loading

Replace `component._prevVNode` with `vnode._children`

coveralls commented Jun 1, 2019 •

edited

Loading

andrewiggins Jun 3, 2019 •

edited

Loading