From 95f69784e44a184997841d79c06c285d674f40b8 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Sat, 28 Oct 2023 02:25:51 -0700 Subject: [PATCH 01/13] === BEGIN two-phase-diffChildren-2 === Redo two phase diff on top of main (+108 B) --- jsx-runtime/src/index.js | 2 + mangle.json | 1 + src/create-element.js | 4 +- src/diff/children.js | 338 ++++++++++++++++++++--------- src/internal.d.ts | 2 + test/browser/createContext.test.js | 2 +- test/browser/fragments.test.js | 325 +++++++-------------------- test/browser/keys.test.js | 12 +- 8 files changed, 320 insertions(+), 366 deletions(-) diff --git a/jsx-runtime/src/index.js b/jsx-runtime/src/index.js index 0bdfd38c5a..768d925eb7 100644 --- a/jsx-runtime/src/index.js +++ b/jsx-runtime/src/index.js @@ -54,6 +54,8 @@ function createVNode(type, props, key, isStaticChildren, __source, __self) { constructor: undefined, _original: --vnodeId, _index: -1, + _insert: false, + _matched: false, __source, __self }; diff --git a/mangle.json b/mangle.json index 76e9fb5956..1f3372808b 100644 --- a/mangle.json +++ b/mangle.json @@ -55,6 +55,7 @@ "$_hydrating": "__h", "$_component": "__c", "$_index": "__i", + "$_flags": "__u", "$__html": "__html", "$_parent": "__", "$_pendingError": "__E", diff --git a/src/create-element.js b/src/create-element.js index 239b7efdd7..bf46e40146 100644 --- a/src/create-element.js +++ b/src/create-element.js @@ -75,7 +75,9 @@ export function createVNode(type, props, key, ref, original) { _hydrating: null, constructor: undefined, _original: original == null ? ++vnodeId : original, - _index: -1 + _index: -1, + _insert: false, + _matched: false }; // Only invoke the vnode hook if this was *not* a direct copy: diff --git a/src/diff/children.js b/src/diff/children.js index 251c6b63d2..3872cf0a33 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -40,7 +40,6 @@ export function diffChildren( refQueue ) { let i, - j, /** @type {VNode} */ oldVNode, /** @type {VNode} */ @@ -48,8 +47,7 @@ export function diffChildren( /** @type {PreactElement} */ newDom, /** @type {PreactElement} */ - firstChildDom, - skew = 0; + firstChildDom; // This is a compression of oldParentVNode!=null && oldParentVNode != EMPTY_OBJ && oldParentVNode._children || EMPTY_ARR // as EMPTY_OBJ._children should be `undefined`. @@ -57,10 +55,165 @@ export function diffChildren( let oldChildren = (oldParentVNode && oldParentVNode._children) || EMPTY_ARR; let oldChildrenLength = oldChildren.length, - remainingOldChildren = oldChildrenLength, newChildrenLength = renderResult.length; - newParentVNode._children = []; + // TODO: Consider replacing with newParentVNode._nextDom; + // TODO: Is there a better way to track oldDom then a ref? Since + // constructNewChildrenArray can unmount DOM nodes while looping (to handle + // null placeholders, i.e. VNode => null in unkeyed children), we need to adjust + // oldDom in that method. + const oldDomRef = { _current: oldDom }; + const newChildren = (newParentVNode._children = constructNewChildrenArray( + newParentVNode, + renderResult, + oldChildren, + oldDomRef + )); + oldDom = oldDomRef._current; + + // Remove remaining oldChildren if there are any. Loop forwards so that as we + // unmount DOM from the beginning of the oldChildren, we can adjust oldDom to + // point to the next child, which needs to be the first DOM node that won't be + // unmounted. + for (i = 0; i < oldChildrenLength; i++) { + oldVNode = oldChildren[i]; + if (oldVNode != null && !oldVNode._matched) { + if (oldDom == oldVNode._dom) { + oldDom = getDomSibling(oldVNode); + + if (typeof newParentVNode.type == 'function') { + // If the parent VNode is a component/fragment, make sure its diff + // continues with a DOM node that is still mounted in case this loop + // exits here because the rest of the new children are `null`. + newParentVNode._nextDom = oldDom; + } + } + + unmount(oldVNode, oldVNode); + } + } + + for (i = 0; i < newChildrenLength; i++) { + childVNode = newChildren[i]; + + if ( + childVNode == null || + typeof childVNode == 'boolean' || + typeof childVNode == 'function' + ) { + continue; + } + + // At this point, constructNewChildrenArray has assigned _index to be the + // matchingIndex for this VNode's oldVNode (or -1 if there is no oldVNode). + if (childVNode._index === -1) { + oldVNode = EMPTY_OBJ; + } else { + oldVNode = oldChildren[childVNode._index] || EMPTY_OBJ; + } + + // Update childVNode._index to its final index + childVNode._index = i; + + // Morph the old element into the new one, but don't append it to the dom yet + diff( + parentDom, + childVNode, + oldVNode, + globalContext, + isSvg, + excessDomChildren, + commitQueue, + oldDom, + isHydrating, + refQueue + ); + + // Adjust DOM nodes + newDom = childVNode._dom; + if (childVNode.ref && oldVNode.ref != childVNode.ref) { + if (oldVNode.ref) { + applyRef(oldVNode.ref, null, childVNode); + } + refQueue.push( + childVNode.ref, + childVNode._component || newDom, + childVNode + ); + } + + if (firstChildDom == null && newDom != null) { + firstChildDom = newDom; + } + + if (typeof childVNode.type == 'function') { + if (childVNode._insert || oldVNode._children === childVNode._children) { + oldDom = reorderChildren(childVNode, oldDom, parentDom); + } else if (childVNode._nextDom !== undefined) { + // Only Fragments or components that return Fragment like VNodes will + // have a non-undefined _nextDom. Continue the diff from the sibling + // of last DOM child of this child VNode + oldDom = childVNode._nextDom; + } else if (newDom) { + oldDom = newDom.nextSibling; + } + + // Eagerly cleanup _nextDom. 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. Once we store it the nextDOM local var, we + // can clean up the property + childVNode._nextDom = undefined; + } else if (newDom) { + if (childVNode._insert) { + oldDom = placeChild(parentDom, newDom, oldDom); + } else { + oldDom = newDom.nextSibling; + } + } + + // TODO: With new child diffing algo, consider alt ways to diff Fragments. + // Such as dropping oldDom and moving fragments in place + if (typeof newParentVNode.type == 'function') { + // Because the newParentVNode is Fragment-like, we need to set it's + // _nextDom property to the nextSibling of its last child DOM node. + // + // `oldDom` contains the correct value here because if the last child + // is a Fragment-like, then oldDom has already been set to that child's _nextDom. + // If the last child is a DOM VNode, then oldDom will be set to that DOM + // node's nextSibling. + newParentVNode._nextDom = oldDom; + } + + // Unset diffing flags + childVNode._insert = false; + } + + newParentVNode._dom = firstChildDom; +} + +/** + * @param {VNode} newParentVNode + * @param {ComponentChildren[]} renderResult + * @param {VNode[]} oldChildren + * @param {{ _current: PreactElement }} oldDomRef + * @returns {VNode[]} + */ +function constructNewChildrenArray( + newParentVNode, + renderResult, + oldChildren, + oldDomRef +) { + /** @type {number} */ + let i; + /** @type {VNode} */ + let childVNode; + let newChildrenLength = renderResult.length; + let remainingOldChildren = oldChildren.length; + let skew = 0; + + /** @type {VNode[]} */ + const newChildren = []; for (i = 0; i < newChildrenLength; i++) { // @ts-expect-error We are reusing the childVNode variable to hold both the // pre and post normalized childVNode @@ -71,7 +224,7 @@ export function diffChildren( typeof childVNode == 'boolean' || typeof childVNode == 'function' ) { - childVNode = newParentVNode._children[i] = null; + childVNode = newChildren[i] = null; } // If this newVNode is being reused (e.g.
{reuse}{reuse}
) in the same diff, // or we are rendering a component (e.g. setState) copy the oldVNodes so it can have @@ -82,7 +235,7 @@ export function diffChildren( // eslint-disable-next-line valid-typeof typeof childVNode == 'bigint' ) { - childVNode = newParentVNode._children[i] = createVNode( + childVNode = newChildren[i] = createVNode( null, childVNode, null, @@ -90,7 +243,7 @@ export function diffChildren( childVNode ); } else if (isArray(childVNode)) { - childVNode = newParentVNode._children[i] = createVNode( + childVNode = newChildren[i] = createVNode( Fragment, { children: childVNode }, null, @@ -102,7 +255,7 @@ export function diffChildren( // scenario: // const reuse =
//
{reuse}{reuse}
- childVNode = newParentVNode._children[i] = createVNode( + childVNode = newChildren[i] = createVNode( childVNode.type, childVNode.props, childVNode.key, @@ -110,27 +263,37 @@ export function diffChildren( childVNode._original ); } else { - childVNode = newParentVNode._children[i] = childVNode; + childVNode = newChildren[i] = childVNode; } - // Terser removes the `continue` here and wraps the loop body - // in a `if (childVNode) { ... } condition + // Handle unmounting null placeholders, i.e. VNode => null in unkeyed children if (childVNode == null) { - oldVNode = oldChildren[i]; + const oldVNode = oldChildren[i]; if (oldVNode && oldVNode.key == null && oldVNode._dom) { - if (oldVNode._dom == oldDom) { - oldDom = getDomSibling(oldVNode); + if (oldVNode._dom == oldDomRef._current) { + oldDomRef._current = getDomSibling(oldVNode); if (typeof newParentVNode.type == 'function') { // If the parent VNode is a component/fragment, make sure its diff // continues with a DOM node that is still mounted in case this loop // exits here because the rest of the new children are `null`. - newParentVNode._nextDom = oldDom; + newParentVNode._nextDom = oldDomRef._current; } } unmount(oldVNode, oldVNode, false); + + // Explicitly nullify this position in oldChildren instead of just + // setting `_match=true` to prevent other routines (e.g. + // `findMatchingIndex` or `getDomSibling`) from thinking VNodes or DOM + // nodes in this position are still available to be used in diffing when + // they have actually already been unmounted. For example, by only + // setting `_match=true` here, the unmounting loop later would attempt + // to unmount this VNode again seeing `_match==true`. Further, + // getDomSibling doesn't know about _match and so would incorrectly + // assume DOM nodes in this subtree are mounted and usable. oldChildren[i] = null; + remainingOldChildren--; } continue; @@ -138,7 +301,6 @@ export function diffChildren( childVNode._parent = newParentVNode; childVNode._depth = newParentVNode._depth + 1; - childVNode._index = i; let skewedIndex = i + skew; const matchingIndex = findMatchingIndex( @@ -148,41 +310,30 @@ export function diffChildren( remainingOldChildren ); - if (matchingIndex === -1) { - oldVNode = EMPTY_OBJ; - } else { - oldVNode = oldChildren[matchingIndex] || EMPTY_OBJ; - oldChildren[matchingIndex] = undefined; - remainingOldChildren--; - } - - // Morph the old element into the new one, but don't append it to the dom yet - diff( - parentDom, - childVNode, - oldVNode, - globalContext, - isSvg, - excessDomChildren, - commitQueue, - oldDom, - isHydrating, - refQueue - ); + // Temporarily store the matchingIndex on the _index property so we can pull + // out the oldVNode in diffChildren. We'll override this to the VNode's + // final index after using this property to get the oldVNode + childVNode._index = matchingIndex; - newDom = childVNode._dom; - if ((j = childVNode.ref) && oldVNode.ref != j) { - if (oldVNode.ref) { - applyRef(oldVNode.ref, null, childVNode); + if (matchingIndex !== -1) { + remainingOldChildren--; + if (oldChildren[matchingIndex]) { + // TODO: Can we somehow not use this property? or override another + // property? We need it now so we can pull off the matchingIndex in + // diffChildren and when unmounting we can get the next DOM element by + // calling getDomSibling, which needs a complete oldTree + oldChildren[matchingIndex]._matched = true; } - refQueue.push(j, childVNode._component || newDom, childVNode); } - if (firstChildDom == null && newDom != null) { - firstChildDom = newDom; - } + // Here, we define isMounting for the purposes of the skew diffing + // algorithm. Nodes that are unsuspending are considered mounting and we detect + // this by checking if oldVNode._original === null + let isMounting = + matchingIndex === -1 || + oldChildren[matchingIndex] == null || + oldChildren[matchingIndex]._original === null; - let isMounting = oldVNode === EMPTY_OBJ || oldVNode._original === null; if (isMounting) { if (matchingIndex == -1) { skew--; @@ -208,67 +359,17 @@ export function diffChildren( } } - skewedIndex = i + skew; - - if (typeof childVNode.type == 'function') { - if ( - matchingIndex !== skewedIndex || - oldVNode._children === childVNode._children - ) { - oldDom = reorderChildren(childVNode, oldDom, parentDom); - } else if (childVNode._nextDom !== undefined) { - // Only Fragments or components that return Fragment like VNodes will - // have a non-undefined _nextDom. Continue the diff from the sibling - // of last DOM child of this child VNode - oldDom = childVNode._nextDom; - } else if (newDom) { - oldDom = newDom.nextSibling; - } - - // Eagerly cleanup _nextDom. 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. Once we store it the nextDOM local var, we - // can clean up the property - childVNode._nextDom = undefined; - } else if (newDom) { - if (matchingIndex !== skewedIndex || isMounting) { - oldDom = placeChild(parentDom, newDom, oldDom); - } else { - oldDom = newDom.nextSibling; - } - } - - if (typeof newParentVNode.type == 'function') { - // Because the newParentVNode is Fragment-like, we need to set it's - // _nextDom property to the nextSibling of its last child DOM node. - // - // `oldDom` contains the correct value here because if the last child - // is a Fragment-like, then oldDom has already been set to that child's _nextDom. - // If the last child is a DOM VNode, then oldDom will be set to that DOM - // node's nextSibling. - newParentVNode._nextDom = oldDom; + // Move this VNode's DOM if the original index (matchingIndex) doesn't match + // the new skew index (i + skew) or it's a mounting component VNode + if ( + matchingIndex !== i + skew || + (typeof childVNode.type != 'function' && isMounting) + ) { + childVNode._insert = true; } } - newParentVNode._dom = firstChildDom; - - // Remove remaining oldChildren if there are any. - for (i = oldChildrenLength; i--; ) { - if (oldChildren[i] != null) { - if ( - typeof newParentVNode.type == 'function' && - oldChildren[i]._dom != null && - oldChildren[i]._dom == oldDom - ) { - // If oldDom points to a dom node that is about to be unmounted, then - // get the next sibling of that vnode and set _nextDom to it, so the - // parent's diff continues diffing an existing DOM node - newParentVNode._nextDom = oldChildren[i]._dom.nextSibling; - } - - unmount(oldChildren[i], oldChildren[i]); - } - } + return newChildren; } /** @@ -354,16 +455,32 @@ function findMatchingIndex( let y = skewedIndex + 1; let oldVNode = oldChildren[skewedIndex]; + // We only need to perform a search if there are more children + // (remainingOldChildren) to search. However, if the oldVNode we just looked + // at skewedIndex was not already used in this diff, then there must be at + // least 1 other (so greater than 1) remainingOldChildren to attempt to match + // against. So the following condition checks that ensuring + // remainingOldChildren > 1 if the oldVNode is not already used/matched. Else + // if the oldVNode was null or matched, then there could needs to be at least + // 1 (aka `remainingOldChildren > 0`) children to find and compare against. + let shouldSearch = + remainingOldChildren > (oldVNode != null && !oldVNode._matched ? 1 : 0); + if ( oldVNode === null || (oldVNode && key == oldVNode.key && type === oldVNode.type) ) { return skewedIndex; - } else if (remainingOldChildren > (oldVNode != null ? 1 : 0)) { + } else if (shouldSearch) { while (x >= 0 || y < oldChildren.length) { if (x >= 0) { oldVNode = oldChildren[x]; - if (oldVNode && key == oldVNode.key && type === oldVNode.type) { + if ( + oldVNode && + !oldVNode._matched && + key == oldVNode.key && + type === oldVNode.type + ) { return x; } x--; @@ -371,7 +488,12 @@ function findMatchingIndex( if (y < oldChildren.length) { oldVNode = oldChildren[y]; - if (oldVNode && key == oldVNode.key && type === oldVNode.type) { + if ( + oldVNode && + !oldVNode._matched && + key == oldVNode.key && + type === oldVNode.type + ) { return y; } y++; diff --git a/src/internal.d.ts b/src/internal.d.ts index ea56157b1e..6953b403af 100644 --- a/src/internal.d.ts +++ b/src/internal.d.ts @@ -151,6 +151,8 @@ declare global { constructor: undefined; _original: number; _index: number; + _insert: boolean; + _matched: boolean; } export interface Component

extends preact.Component { diff --git a/test/browser/createContext.test.js b/test/browser/createContext.test.js index f4610ed372..b27f20b625 100644 --- a/test/browser/createContext.test.js +++ b/test/browser/createContext.test.js @@ -877,8 +877,8 @@ describe('createContext', () => { expect(events).to.deep.equal([ 'render 0', 'mount 0', - 'render 1', 'unmount 0', + 'render 1', 'mount 1' ]); }); diff --git a/test/browser/fragments.test.js b/test/browser/fragments.test.js index 3e5292f2c5..20fed865e4 100644 --- a/test/browser/fragments.test.js +++ b/test/browser/fragments.test.js @@ -1,7 +1,7 @@ import { setupRerender } from 'preact/test-utils'; import { createElement, render, Component, Fragment } from 'preact'; import { setupScratch, teardown } from '../_util/helpers'; -import { span, div, ul, ol, li, section, p } from '../_util/dom'; +import { span, div, ul, ol, li, section } from '../_util/dom'; import { logCall, clearLog, getLog } from '../_util/logCall'; /** @jsx createElement */ @@ -235,9 +235,9 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal(div([div(1), span(2), span(2)])); expectDomLogToBe([ + '1.remove()', '

.appendChild(#text)', - '
122.insertBefore(
1, 1)', - '1.remove()' + '
22.insertBefore(
1, 2)' ]); }); @@ -357,9 +357,9 @@ describe('Fragment', () => { expect(ops).to.deep.equal([]); expect(scratch.innerHTML).to.equal('
Hello
'); expectDomLogToBe([ + '
Hello.remove()', '
.appendChild(#text)', - '
Hello.insertBefore(
Hello,
Hello)', - '
Hello.remove()' + '
.appendChild(
Hello)' ]); clearLog(); @@ -368,10 +368,10 @@ describe('Fragment', () => { expect(ops).to.deep.equal([]); expect(scratch.innerHTML).to.equal('
Hello
'); expectDomLogToBe([ + '
Hello.remove()', '
.appendChild(#text)', // Re-append the Stateful DOM since it has been re-parented - '
Hello.insertBefore(
Hello,
Hello)', - '
Hello.remove()' + '
.appendChild(
Hello)' ]); }); @@ -396,9 +396,9 @@ describe('Fragment', () => { expect(ops).to.deep.equal([]); expect(scratch.innerHTML).to.equal('
Hello
'); expectDomLogToBe([ + '
Hello.remove()', '
.appendChild(#text)', - '
Hello.insertBefore(
Hello,
Hello)', - '
Hello.remove()' + '
.appendChild(
Hello)' ]); clearLog(); @@ -407,9 +407,9 @@ describe('Fragment', () => { expect(ops).to.deep.equal([]); expect(scratch.innerHTML).to.equal('
Hello
'); expectDomLogToBe([ + '
Hello.remove()', '
.appendChild(#text)', - '
Hello.insertBefore(
Hello,
Hello)', - '
Hello.remove()' + '
.appendChild(
Hello)' ]); }); @@ -647,7 +647,6 @@ describe('Fragment', () => { }); it('should preserve order for fragment switching', () => { - /** @type {(newState: { isLoading: boolean; data: number | null }) => void} */ let set; class Foo extends Component { constructor(props) { @@ -678,84 +677,6 @@ describe('Fragment', () => { ); }); - it('should preserve order for fragment switching with sibling DOM', () => { - /** @type {(newState: { isLoading: boolean; data: number | null }) => void} */ - let set; - class Foo extends Component { - constructor(props) { - super(props); - this.state = { isLoading: true, data: null }; - set = this.setState.bind(this); - } - render(props, { isLoading, data }) { - return ( - -
HEADER
- {isLoading ?
Loading...
: null} - {data ?
Content: {data}
: null} -
FOOTER
- - ); - } - } - - render(, scratch); - expect(scratch.innerHTML).to.equal( - '
HEADER
Loading...
FOOTER
' - ); - - set({ isLoading: false, data: 2 }); - rerender(); - expect(scratch.innerHTML).to.equal( - '
HEADER
Content: 2
FOOTER
' - ); - }); - - it('should preserve order for fragment switching with sibling Components', () => { - /** @type {(newState: { isLoading: boolean; data: number | null }) => void} */ - let set; - class Foo extends Component { - constructor(props) { - super(props); - this.state = { isLoading: true, data: null }; - set = this.setState.bind(this); - } - render(props, { isLoading, data }) { - return ( - -
HEADER
- {isLoading ?
Loading...
: null} - {data ?
Content: {data}
: null} - - ); - } - } - - function Footer() { - return
FOOTER
; - } - - function App() { - return ( - - -