Skip to content

Commit

Permalink
Rework children diffing to run in multiple phases (#4180)
Browse files Browse the repository at this point in the history
Rework children diffing to run in multiple phases:

1. First, find matches for all new VNodes in the old VNode tree, and determine which VNodes moved
2. Second, remove any unmatched VNodes in the old tree from the DOM
3. Finally, diff, move, and insert all VNodes in the new tree

A couple additional changes to highlight
* Add new _flags VNode field to capture some diffing state (whether an oldVNode was matched and whether to insert a newVNode)
* Combine `placeChild` and `reorderChildren` into a single `insert` function & simplify insertion logic in diffChildren
* Simplify some _nextDom logic by always setting & clearing it
* Fold _hydrating field into _flags field (+12 B)

Fixes #4156 

Commit list:

* Redo two phase diff on top of main (+108 B)
* Use single flag field for matched and insert (+28 B)
* WIP: add some tests from v11
* Fix new keyed tests
* Replace oldDomRef with newParentVNode._nextDom and move unmounting into constructNewChildrenArray (-27 B)
* Always set _nextDom to simplify code (-4 B)
   Will always be cleared in parent diffChildren or commitRoot
* Combine `placeChild` and `reorderChildren` & simplify insertion condition (-24 B)
* Use .to.equalNode in test
* Always initialize commitQueue & refQueue (-5 B)
* Directly store and read newChildren on newParentVNode (-9 B)
* Fold _hydrating field into _flags field (+12 B)
* Apply Jovi's suggestions from code review
* Add additional suspense hydration test

---------

Co-authored-by: Jovi De Croock <[email protected]>
  • Loading branch information
andrewiggins and JoviDeCroock authored Nov 7, 2023
1 parent 99709ae commit 9aa4728
Show file tree
Hide file tree
Showing 15 changed files with 512 additions and 426 deletions.
8 changes: 4 additions & 4 deletions compat/src/suspense.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Component, createElement, options, Fragment } from 'preact';
import { MODE_HYDRATE } from 'preact/src/constants';
import { assign } from './util';

const oldCatchError = options._catchError;
Expand Down Expand Up @@ -34,7 +35,7 @@ options.unmount = function (vnode) {
// most likely it is because the component is suspended
// we set the vnode.type as `null` so that it is not a typeof function
// so the unmount will remove the vnode._dom
if (component && vnode._hydrating === true) {
if (component && vnode._flags & MODE_HYDRATE) {
vnode.type = null;
}

Expand Down Expand Up @@ -166,8 +167,7 @@ Suspense.prototype._childDidSuspend = function (promise, suspendingVNode) {
* to remain on screen and hydrate it when the suspense actually gets resolved.
* While in non-hydration cases the usual fallback -> component flow would occour.
*/
const wasHydrating = suspendingVNode._hydrating === true;
if (!c._pendingSuspensionCount++ && !wasHydrating) {
if (!c._pendingSuspensionCount++ && !(suspendingVNode._flags & MODE_HYDRATE)) {
c.setState({ _suspended: (c._detachOnNextRender = c._vnode._children[0]) });
}
promise.then(onResolved, onResolved);
Expand Down Expand Up @@ -204,7 +204,7 @@ Suspense.prototype.render = function (props, state) {
/** @type {import('./internal').VNode} */
const fallback =
state._suspended && createElement(Fragment, null, props.fallback);
if (fallback) fallback._hydrating = null;
if (fallback) fallback._flags &= ~MODE_HYDRATE;

return [
createElement(Fragment, null, state._suspended ? null : props.children),
Expand Down
4 changes: 2 additions & 2 deletions compat/test/browser/hydrate.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ describe('compat hydrate', () => {
const input = document.createElement('input');
scratch.appendChild(input);
input.focus();
expect(document.activeElement).to.equal(input);
expect(document.activeElement).to.equalNode(input);

hydrate(<input />, scratch);
expect(document.activeElement).to.equal(input);
expect(document.activeElement).to.equalNode(input);
});

it('should call the callback', () => {
Expand Down
42 changes: 41 additions & 1 deletion compat/test/browser/suspense-hydration.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import React, {
hydrate,
Fragment,
Suspense,
memo,
useState
} from 'preact/compat';
import { logCall, getLog, clearLog } from '../../../test/_util/logCall';
Expand All @@ -13,7 +14,7 @@ import {
teardown
} from '../../../test/_util/helpers';
import { ul, li, div } from '../../../test/_util/dom';
import { createLazy } from './suspense-utils';
import { createLazy, createSuspenseLoader } from './suspense-utils';

/* eslint-env browser, mocha */
describe('suspense hydration', () => {
Expand Down Expand Up @@ -659,6 +660,45 @@ describe('suspense hydration', () => {
});
});

it('should correctly hydrate and rerender a memoized lazy data loader', () => {
const originalHtml = '<p>Count: 5</p>';
scratch.innerHTML = originalHtml;

const [useSuspenseLoader, resolve] = createSuspenseLoader();

/** @type {() => void} */
let increment;
const DataLoader = memo(function DataLoader() {
const initialCount = useSuspenseLoader();
const [count, setCount] = useState(initialCount);
increment = () => setCount(c => c + 1);

return <p>Count: {count}</p>;
});

function App() {
return (
<Suspense>
<DataLoader />
</Suspense>
);
}

hydrate(<App />, scratch);
rerender();
expect(scratch.innerHTML).to.equal(originalHtml);

return resolve(5).then(() => {
rerender();
expect(scratch.innerHTML).to.equal('<p>Count: 5</p>');

increment();
rerender();

expect(scratch.innerHTML).to.equal('<p>Count: 6</p>');
});
});

// Currently not supported. Hydration doesn't set attributes... but should it
// when coming back from suspense if props were updated?
it.skip('should hydrate and update attributes with latest props', () => {
Expand Down
43 changes: 43 additions & 0 deletions compat/test/browser/suspense-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,3 +114,46 @@ export function createSuspender(DefaultComponent) {

return [Suspender, suspend];
}

/**
* @returns {[() => any, (data: any) => Promise<any>, (error: Error) => Promise<any>]}
*/
export function createSuspenseLoader() {
/** @type {(data: any) => Promise<any>} */
let resolver;
/** @type {(error: Error) => Promise<any>} */
let rejecter;
/** @type {any} */
let data = null;
/** @type {Error} */
let error = null;

/** @type {Promise<any>} */
let promise = new Promise((resolve, reject) => {
resolver = result => {
data = result;
resolve(result);
return promise;
};

rejecter = e => {
error = e;
reject(e);
return promise;
};
});

function useSuspenseLoader() {
if (error) {
throw error;
}

if (!data) {
throw promise;
}

return data;
}

return [useSuspenseLoader, resolver, rejecter];
}
5 changes: 2 additions & 3 deletions jsx-runtime/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ import { options, Fragment } from 'preact';
import { encodeEntities } from './utils';
import { IS_NON_DIMENSIONAL } from '../../src/constants';

/** @typedef {import('preact').VNode} VNode */

let vnodeId = 0;

const isArray = Array.isArray;
Expand Down Expand Up @@ -43,6 +41,7 @@ function createVNode(type, props, key, isStaticChildren, __source, __self) {
}
}

/** @type {VNode & { __source: any; __self: any }} */
const vnode = {
type,
props: normalizedProps,
Expand All @@ -54,10 +53,10 @@ function createVNode(type, props, key, isStaticChildren, __source, __self) {
_dom: null,
_nextDom: undefined,
_component: null,
_hydrating: null,
constructor: undefined,
_original: --vnodeId,
_index: -1,
_flags: 0,
__source,
__self
};
Expand Down
2 changes: 1 addition & 1 deletion mangle.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@
"$_onResolve": "__R",
"$_suspended": "__a",
"$_dom": "__e",
"$_hydrating": "__h",
"$_component": "__c",
"$_index": "__i",
"$_flags": "__u",
"$__html": "__html",
"$_parent": "__",
"$_pendingError": "__E",
Expand Down
12 changes: 6 additions & 6 deletions src/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { assign } from './util';
import { diff, commitRoot } from './diff/index';
import options from './options';
import { Fragment } from './create-element';
import { MODE_HYDRATE } from './constants';

/**
* Base Component class. Provides `setState()` and `forceUpdate()`, which
Expand Down Expand Up @@ -122,12 +123,11 @@ export function getDomSibling(vnode, childIndex) {
function renderComponent(component) {
let oldVNode = component._vnode,
oldDom = oldVNode._dom,
parentDom = component._parentDom;
parentDom = component._parentDom,
commitQueue = [],
refQueue = [];

if (parentDom) {
let commitQueue = [],
refQueue = [];

const newVNode = assign({}, oldVNode);
newVNode._original = oldVNode._original + 1;

Expand All @@ -137,10 +137,10 @@ function renderComponent(component) {
oldVNode,
component._globalContext,
parentDom.ownerSVGElement !== undefined,
oldVNode._hydrating != null ? [oldDom] : null,
oldVNode._flags & MODE_HYDRATE ? [oldDom] : null,
commitQueue,
oldDom == null ? getDomSibling(oldVNode) : oldDom,
oldVNode._hydrating,
!!(oldVNode._flags & MODE_HYDRATE),
refQueue
);

Expand Down
12 changes: 12 additions & 0 deletions src/constants.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,15 @@
/** Normal hydration that attaches to a DOM tree but does not diff it. */
export const MODE_HYDRATE = 1 << 5;
/** Signifies this VNode suspended on the previous render */
export const MODE_SUSPENDED = 1 << 7;
/** Indicates that this node needs to be inserted while patching children */
export const INSERT_VNODE = 1 << 16;
/** Indicates a VNode has been matched with another VNode in the diff */
export const MATCHED = 1 << 17;

/** Reset all mode flags */
export const RESET_MODE = ~(MODE_HYDRATE | MODE_SUSPENDED);

export const EMPTY_OBJ = /** @type {any} */ ({});
export const EMPTY_ARR = [];
export const IS_NON_DIMENSIONAL =
Expand Down
4 changes: 2 additions & 2 deletions src/create-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,10 @@ export function createVNode(type, props, key, ref, original) {
// a _nextDom that has been set to `null`
_nextDom: undefined,
_component: null,
_hydrating: null,
constructor: undefined,
_original: original == null ? ++vnodeId : original,
_index: -1
_index: -1,
_flags: 0
};

// Only invoke the vnode hook if this was *not* a direct copy:
Expand Down
Loading

0 comments on commit 9aa4728

Please sign in to comment.