From 61fb57988f42463fbd762312c38be60847889d1e Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 20 Oct 2023 16:09:05 -0700 Subject: [PATCH 01/15] Pass render.tests.js with new diffing algo --- src/constants.js | 3 + src/create-element.js | 6 +- src/diff/children.js | 288 +++++++++++++++++++++++++++++++++++- src/internal.d.ts | 5 + test/browser/render.test.js | 8 +- 5 files changed, 303 insertions(+), 7 deletions(-) diff --git a/src/constants.js b/src/constants.js index c30ffc381f..bdaa7cd77a 100644 --- a/src/constants.js +++ b/src/constants.js @@ -1,4 +1,7 @@ export const EMPTY_OBJ = {}; +export const EMPTY_VNODE = /** @type {import('./internal').VNode} */ ( + EMPTY_OBJ +); export const EMPTY_ARR = []; export const IS_NON_DIMENSIONAL = /acit|ex(?:s|g|n|p|$)|rph|grid|ows|mnc|ntw|ine[ch]|zoo|^ord|itera/i; diff --git a/src/create-element.js b/src/create-element.js index b55be840ad..10572838b5 100644 --- a/src/create-element.js +++ b/src/create-element.js @@ -72,7 +72,11 @@ export function createVNode(type, props, key, ref, original) { _component: null, _hydrating: null, constructor: undefined, - _original: original == null ? ++vnodeId : original + _original: original == null ? ++vnodeId : original, + _prevVNode: undefined, + _insert: false, + _matched: false, + _index: -1 }; // 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 c39e54dc2a..06a12c8813 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -1,9 +1,11 @@ import { diff, unmount, applyRef } from './index'; import { createVNode, Fragment } from '../create-element'; -import { EMPTY_OBJ, EMPTY_ARR } from '../constants'; +import { EMPTY_OBJ, EMPTY_ARR, EMPTY_VNODE } from '../constants'; import { isArray } from '../util'; import { getDomSibling } from '../component'; +export const diffChildren = diffChildren2; + /** * Diff the children of a virtual node * @param {import('../internal').PreactElement} parentDom The DOM element whose @@ -25,7 +27,7 @@ import { getDomSibling } from '../component'; * @param {boolean} isHydrating Whether or not we are in hydration * @param {Array} refQueue an array of elements needed to invoke refs */ -export function diffChildren( +function diffChildren1( parentDom, renderResult, newParentVNode, @@ -69,6 +71,8 @@ export function diffChildren( // or we are rendering a component (e.g. setState) copy the oldVNodes so it can have // it's own DOM & etc. pointers else if ( + // TODO: To make TS happy, I've added this check, but it's not necessary. Remove + typeof childVNode == 'string' || childVNode.constructor == String || typeof childVNode == 'number' || // eslint-disable-next-line valid-typeof @@ -257,6 +261,286 @@ export function diffChildren( } } +/** + * Diff the children of a virtual node + * @param {import('../internal').PreactElement} parentDom The DOM element whose + * children are being diffed + * @param {import('../internal').ComponentChildren[]} renderResult + * @param {import('../internal').VNode} newParentVNode The new virtual + * node whose children should be diff'ed against oldParentVNode + * @param {import('../internal').VNode} oldParentVNode The old virtual + * node whose children should be diff'ed against newParentVNode + * @param {object} globalContext The current context object - modified by getChildContext + * @param {boolean} isSvg Whether or not this DOM node is an SVG node + * @param {Array} excessDomChildren + * @param {Array} commitQueue List of components + * which have callbacks to invoke in commitRoot + * @param {import('../internal').PreactElement} oldDom The current attached DOM + * element any new dom elements should be placed around. Likely `null` on first + * render (except when hydrating). Can be a sibling DOM element when diffing + * Fragments that have siblings. In most cases, it starts out as `oldChildren[0]._dom`. + * @param {boolean} isHydrating Whether or not we are in hydration + * @param {Array} refQueue an array of elements needed to invoke refs + */ +function diffChildren2( + parentDom, + renderResult, + newParentVNode, + oldParentVNode, + globalContext, + isSvg, + excessDomChildren, + commitQueue, + oldDom, + isHydrating, + refQueue +) { + let i, j, childVNode, newDom, firstChildDom; + + // This is a compression of oldParentVNode!=null && oldParentVNode != EMPTY_VNODE && oldParentVNode._children || EMPTY_ARR + // as EMPTY_VNODE._children should be `undefined`. + let oldChildren = (oldParentVNode && oldParentVNode._children) || EMPTY_ARR; + + let oldChildrenLength = oldChildren.length, + newChildrenLength = renderResult.length; + + const newChildren = (newParentVNode._children = constructNewChildrenArray( + newParentVNode, + renderResult, + oldChildren + )); + + // 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 == newParentVNode._nextDom + ) { + // If the newParentVNode.__nextDom points to a dom node that is about to + // be unmounted, then get the next sibling of that vnode and set + // _nextDom to it + + newParentVNode._nextDom = oldChildren[i]._dom.nextSibling; + } + */ + + unmount(oldChildren[i], oldChildren[i]); + } + } + + for (let i = 0; i < newChildrenLength; i++) { + childVNode = newChildren[i]; + + if ( + childVNode == null || + typeof childVNode == 'boolean' || + typeof childVNode == 'function' + ) { + continue; + } + + diff( + parentDom, + childVNode, + childVNode._prevVNode, + globalContext, + isSvg, + excessDomChildren, + commitQueue, + oldDom, + isHydrating, + refQueue + ); + + // Adjust DOM nodes + newDom = childVNode._dom; + if ((j = childVNode.ref) && childVNode._prevVNode.ref != j) { + if (childVNode._prevVNode.ref) { + applyRef(childVNode._prevVNode.ref, null, childVNode); + } + refQueue.push(j, childVNode._component || newDom, childVNode); + } + + if (newDom != null) { + if (firstChildDom == null) { + firstChildDom = newDom; + } + + if (childVNode._insert || childVNode._prevVNode === EMPTY_VNODE) { + if (typeof childVNode.type == 'function') { + oldDom = reorderChildren(childVNode, oldDom, parentDom); + } else { + oldDom = placeChild(parentDom, newDom, oldDom); + } + } + + oldDom = newDom.nextSibling; + } + + // Unset diffing properties + childVNode._prevVNode = null; + childVNode._insert = false; + childVNode._matched = false; + } + + newParentVNode._dom = firstChildDom; +} + +/** + * @param {import('../internal').VNode} newParentVNode + * @param {import('../internal').ComponentChildren[]} renderResult + * @param {import('../internal').VNode[]} oldChildren + * @returns {import('../internal').VNode[]} + */ +function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { + /** @type {number} */ + let i; + /** @type {import('../internal').VNode} */ + let childVNode; + /** @type {import('../internal').VNode} */ + let oldVNode; + let remainingOldChildren = oldChildren.length; + let lastPlacedIndex = 0; + // TODO: Bring back skewed diff? + // let skew = 0; + + const newChildren = []; + for (i = 0; i < renderResult.length; i++) { + childVNode = renderResult[i]; + + // Convert render results to VNodes (e.g. strings => VNodes, etc.) + if ( + childVNode == null || + typeof childVNode == 'boolean' || + typeof childVNode == 'function' + ) { + 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 + // it's own DOM & etc. pointers + else if ( + // TODO: To make TS happy, I've added this check, but it's not necessary. Remove + typeof childVNode == 'string' || + childVNode.constructor == String || + typeof childVNode == 'number' || + // eslint-disable-next-line valid-typeof + typeof childVNode == 'bigint' + ) { + childVNode = newChildren[i] = createVNode( + null, + childVNode, + null, + null, + childVNode + ); + } else if (isArray(childVNode)) { + childVNode = newChildren[i] = createVNode( + Fragment, + { children: childVNode }, + null, + null, + null + ); + } else if (childVNode._depth > 0) { + // VNode is already in use, clone it. This can happen in the following + // scenario: + // const reuse =
+ //
{reuse}{reuse}
+ childVNode = newChildren[i] = createVNode( + childVNode.type, + childVNode.props, + childVNode.key, + childVNode.ref ? childVNode.ref : null, + childVNode._original + ); + } else { + childVNode = newChildren[i] = childVNode; + } + + // Handle unmounting null placeholders, i.e. VNode => null in unkeyed children + if (childVNode == null) { + oldVNode = oldChildren[i]; + if (oldVNode && oldVNode.key == null && oldVNode._dom) { + /* + // TODO: Bring this back? + if (oldVNode._dom == oldDom) { + oldVNode._parent = oldParentVNode; + oldDom = getDomSibling(oldVNode); + } + */ + + unmount(oldVNode, oldVNode, false); + oldChildren[i] = null; + } + + continue; + } + + childVNode._parent = newParentVNode; + childVNode._depth = newParentVNode._depth + 1; + childVNode._index = i; + + // let skewedIndex = i + skew; + const matchingIndex = findMatchingIndex( + childVNode, + oldChildren, + i, // skewedIndex, + remainingOldChildren + ); + + childVNode._prevVNode = oldVNode = + oldChildren[matchingIndex] || EMPTY_VNODE; + if (matchingIndex === -1) { + childVNode._matched = false; + childVNode._insert = true; + } else { + childVNode._matched = true; + if (oldVNode._index < lastPlacedIndex) { + childVNode._insert = true; + } else { + lastPlacedIndex = oldVNode._index; + } + + oldChildren[matchingIndex] = undefined; + remainingOldChildren--; + } + + /* + let isMounting = oldVNode === EMPTY_VNODE || oldVNode._original === null; + if (isMounting) { + if (matchingIndex == -1) { + skew--; + } + } else if (matchingIndex !== skewedIndex) { + if (matchingIndex === skewedIndex + 1) { + skew++; + } else if (matchingIndex > skewedIndex) { + if (remainingOldChildren > newChildrenLength - skewedIndex) { + skew += matchingIndex - skewedIndex; + } else { + // ### Change from keyed: I think this was missing from the algo... + skew--; + } + } else if (matchingIndex < skewedIndex) { + if (matchingIndex == skewedIndex - 1) { + skew = matchingIndex - skewedIndex; + } else { + skew = 0; + } + } else { + skew = 0; + } + } + */ + } + + return newChildren; +} + function reorderChildren(childVNode, oldDom, parentDom) { // Note: VNodes in nested suspended trees may be missing _children. let c = childVNode._children; diff --git a/src/internal.d.ts b/src/internal.d.ts index 57faa0ded0..ded94b41c8 100644 --- a/src/internal.d.ts +++ b/src/internal.d.ts @@ -122,6 +122,11 @@ export interface VNode

extends preact.VNode

{ _hydrating: boolean | null; constructor: undefined; _original: number; + // New properties + _prevVNode: VNode; + _insert: boolean; + _matched: boolean; + _index: number; } export interface Component

extends preact.Component { diff --git a/test/browser/render.test.js b/test/browser/render.test.js index 6dab0019ab..62c6b3ad8d 100644 --- a/test/browser/render.test.js +++ b/test/browser/render.test.js @@ -26,7 +26,7 @@ function getAttributes(node) { const isIE11 = /Trident\//.test(navigator.userAgent); -describe('render()', () => { +describe.only('render()', () => { let scratch, rerender; let resetAppendChild; @@ -1064,7 +1064,7 @@ describe('render()', () => { it('should not remove iframe', () => { let setState; const Iframe = () => { - return

' + '
Test3
Test2
' ); clearLog(); setState({ value: false }); rerender(); expect(scratch.innerHTML).to.equal( - '
' + '
' ); expect(getLog()).to.deep.equal([ '
Test3.remove()', From d639ae05d02bd920320ab0dc38e779a3fee1aa53 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 20 Oct 2023 16:25:07 -0700 Subject: [PATCH 02/15] Prepare code for bringing back skew Separate matching and inserting logic since skew logic needs to run in between those --- src/diff/children.js | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/src/diff/children.js b/src/diff/children.js index 06a12c8813..8f748c7158 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -375,9 +375,23 @@ function diffChildren2( } else { oldDom = placeChild(parentDom, newDom, oldDom); } + } else { + oldDom = newDom.nextSibling; } - oldDom = newDom.nextSibling; + // TODO: Do we still need this? + /* + 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 properties @@ -409,7 +423,7 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { const newChildren = []; for (i = 0; i < renderResult.length; i++) { - childVNode = renderResult[i]; + childVNode = /** @type {import('../internal').VNode} */ (renderResult[i]); // Convert render results to VNodes (e.g. strings => VNodes, etc.) if ( @@ -494,23 +508,17 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { childVNode._prevVNode = oldVNode = oldChildren[matchingIndex] || EMPTY_VNODE; - if (matchingIndex === -1) { + + let isMounting = oldVNode === EMPTY_VNODE || oldVNode._original === null; + if (isMounting) { childVNode._matched = false; - childVNode._insert = true; } else { childVNode._matched = true; - if (oldVNode._index < lastPlacedIndex) { - childVNode._insert = true; - } else { - lastPlacedIndex = oldVNode._index; - } - oldChildren[matchingIndex] = undefined; remainingOldChildren--; } /* - let isMounting = oldVNode === EMPTY_VNODE || oldVNode._original === null; if (isMounting) { if (matchingIndex == -1) { skew--; @@ -536,6 +544,14 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { } } */ + + if (isMounting) { + childVNode._insert = true; + } else if (oldVNode._index < lastPlacedIndex) { + childVNode._insert = true; + } else { + lastPlacedIndex = oldVNode._index; + } } return newChildren; From dc37be19c4dbfb99eae5afe981c3cebb7d6724a2 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 20 Oct 2023 17:27:33 -0700 Subject: [PATCH 03/15] Fix placeholder tests --- src/diff/children.js | 47 ++++++++++++++++--------------- test/browser/placeholders.test.js | 2 +- 2 files changed, 26 insertions(+), 23 deletions(-) diff --git a/src/diff/children.js b/src/diff/children.js index 8f748c7158..178f868c79 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -304,28 +304,28 @@ function diffChildren2( let oldChildrenLength = oldChildren.length, newChildrenLength = renderResult.length; + // 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 + oldChildren, + oldDomRef )); - // 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 == newParentVNode._nextDom - ) { - // If the newParentVNode.__nextDom points to a dom node that is about to - // be unmounted, then get the next sibling of that vnode and set - // _nextDom to it + oldDom = oldDomRef.current; - newParentVNode._nextDom = oldChildren[i]._dom.nextSibling; + // 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. + for (i = 0; i < oldChildrenLength; i++) { + if (oldChildren[i] != null && !oldChildren[i]._matched) { + if (oldDom == oldChildren[i]._dom) { + oldDom = getDomSibling(oldChildren[i]); } - */ unmount(oldChildren[i], oldChildren[i]); } @@ -407,9 +407,15 @@ function diffChildren2( * @param {import('../internal').VNode} newParentVNode * @param {import('../internal').ComponentChildren[]} renderResult * @param {import('../internal').VNode[]} oldChildren + * @param {{ current: import('../internal').PreactElement }} oldDomRef * @returns {import('../internal').VNode[]} */ -function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { +function constructNewChildrenArray( + newParentVNode, + renderResult, + oldChildren, + oldDomRef +) { /** @type {number} */ let i; /** @type {import('../internal').VNode} */ @@ -479,13 +485,10 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) { if (childVNode == null) { oldVNode = oldChildren[i]; if (oldVNode && oldVNode.key == null && oldVNode._dom) { - /* - // TODO: Bring this back? - if (oldVNode._dom == oldDom) { - oldVNode._parent = oldParentVNode; - oldDom = getDomSibling(oldVNode); + if (oldVNode._dom == oldDomRef.current) { + // oldVNode._parent = oldParentVNode; + oldDomRef.current = getDomSibling(oldVNode); } - */ unmount(oldVNode, oldVNode, false); oldChildren[i] = null; diff --git a/test/browser/placeholders.test.js b/test/browser/placeholders.test.js index 0fbdb6f181..ab0cc2e9e5 100644 --- a/test/browser/placeholders.test.js +++ b/test/browser/placeholders.test.js @@ -6,7 +6,7 @@ import { div } from '../_util/dom'; /** @jsx createElement */ -describe('null placeholders', () => { +describe.only('null placeholders', () => { /** @type {HTMLDivElement} */ let scratch; From 0774f260a54d4ed2b008dd472953d435355baca8 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Sat, 21 Oct 2023 00:16:05 -0700 Subject: [PATCH 04/15] Fix keyed tests by implementing skewed diff --- src/diff/children.js | 70 ++++++++++++++++++++++++++++----------- test/browser/keys.test.js | 14 ++++---- 2 files changed, 58 insertions(+), 26 deletions(-) diff --git a/src/diff/children.js b/src/diff/children.js index 178f868c79..7ca44e9834 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -422,10 +422,9 @@ function constructNewChildrenArray( let childVNode; /** @type {import('../internal').VNode} */ let oldVNode; + let newChildrenLength = renderResult.length; let remainingOldChildren = oldChildren.length; - let lastPlacedIndex = 0; - // TODO: Bring back skewed diff? - // let skew = 0; + let skew = 0; const newChildren = []; for (i = 0; i < renderResult.length; i++) { @@ -491,7 +490,18 @@ function constructNewChildrenArray( } 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; @@ -501,11 +511,11 @@ function constructNewChildrenArray( childVNode._depth = newParentVNode._depth + 1; childVNode._index = i; - // let skewedIndex = i + skew; + let skewedIndex = i + skew; const matchingIndex = findMatchingIndex( childVNode, oldChildren, - i, // skewedIndex, + skewedIndex, remainingOldChildren ); @@ -513,15 +523,12 @@ function constructNewChildrenArray( oldChildren[matchingIndex] || EMPTY_VNODE; let isMounting = oldVNode === EMPTY_VNODE || oldVNode._original === null; - if (isMounting) { - childVNode._matched = false; - } else { - childVNode._matched = true; - oldChildren[matchingIndex] = undefined; + if (!isMounting) { + // Mark oldVNode as matched so it isn't unmounted + oldVNode._matched = true; remainingOldChildren--; } - /* if (isMounting) { if (matchingIndex == -1) { skew--; @@ -546,14 +553,19 @@ function constructNewChildrenArray( skew = 0; } } - */ - if (isMounting) { + skewedIndex = i + skew; + if ( + typeof childVNode.type == 'function' && + (matchingIndex !== skewedIndex || + oldVNode._children === childVNode._children) + ) { childVNode._insert = true; - } else if (oldVNode._index < lastPlacedIndex) { + } else if ( + typeof childVNode.type != 'function' && + (matchingIndex !== skewedIndex || isMounting) + ) { childVNode._insert = true; - } else { - lastPlacedIndex = oldVNode._index; } } @@ -632,17 +644,32 @@ function findMatchingIndex( let x = skewedIndex - 1; 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--; @@ -650,7 +677,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/test/browser/keys.test.js b/test/browser/keys.test.js index 7bc883418d..1130ad14c6 100644 --- a/test/browser/keys.test.js +++ b/test/browser/keys.test.js @@ -6,7 +6,7 @@ import { setupRerender } from 'preact/test-utils'; /** @jsx createElement */ -describe('keys', () => { +describe.only('keys', () => { /** @type {HTMLDivElement} */ let scratch; @@ -252,9 +252,9 @@ describe('keys', () => { render(, scratch); expect(scratch.textContent).to.equal('abcd'); expect(getLog()).to.deep.equal([ - '
  • z.remove()', + '
  • x.remove()', '
  • y.remove()', - '
  • x.remove()' + '
  • z.remove()' ]); }); @@ -462,8 +462,8 @@ describe('keys', () => { expect(scratch.innerHTML).to.equal(expectedHtml); expect(ops).to.deep.equal([ - 'Unmount Stateful2', 'Unmount Stateful1', + 'Unmount Stateful2', 'Mount Stateful1', 'Mount Stateful2' ]); @@ -475,8 +475,8 @@ describe('keys', () => { expect(scratch.innerHTML).to.equal(expectedHtml); expect(ops).to.deep.equal([ - 'Unmount Stateful2', 'Unmount Stateful1', + 'Unmount Stateful2', 'Mount Stateful1', 'Mount Stateful2' ]); @@ -731,8 +731,8 @@ describe('keys', () => { expect(scratch.innerHTML).to.equal(expectedHtml); expect(ops).to.deep.equal([ - 'Unmount Stateful2', 'Unmount Stateful1', + 'Unmount Stateful2', 'Mount Stateful1', 'Mount Stateful2' ]); @@ -744,8 +744,8 @@ describe('keys', () => { expect(scratch.innerHTML).to.equal(expectedHtml); expect(ops).to.deep.equal([ - 'Unmount Stateful2', 'Unmount Stateful1', + 'Unmount Stateful2', 'Mount Stateful1', 'Mount Stateful2' ]); From 477ca83dd0d698a7170ecd2311a25157242e385a Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Sat, 21 Oct 2023 01:16:45 -0700 Subject: [PATCH 05/15] Fix Fragment tests Had to move component vnode children equality check out of constructNewChildrenArray since at this point we haven't diffed the vnode and so don't know if it's gonna shortcut and reuse the existing children array, requiring us to call reorderChildren to calculate `_nextDom` --- src/diff/children.js | 38 +++++---- test/browser/fragments.test.js | 149 +++++++++++++++++---------------- 2 files changed, 97 insertions(+), 90 deletions(-) diff --git a/src/diff/children.js b/src/diff/children.js index 7ca44e9834..ba5d0ed80b 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -369,18 +369,34 @@ function diffChildren2( firstChildDom = newDom; } - if (childVNode._insert || childVNode._prevVNode === EMPTY_VNODE) { + // TODO: Golf this `if` block + if ( + childVNode._insert || + (typeof childVNode.type == 'function' && + childVNode._children == childVNode._prevVNode._children) + ) { if (typeof childVNode.type == 'function') { oldDom = reorderChildren(childVNode, oldDom, parentDom); } else { oldDom = placeChild(parentDom, newDom, oldDom); } + } 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; + + // 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 { oldDom = newDom.nextSibling; } - // TODO: Do we still need this? - /* + // 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. @@ -391,7 +407,6 @@ function diffChildren2( // node's nextSibling. newParentVNode._nextDom = oldDom; } - */ } // Unset diffing properties @@ -555,18 +570,9 @@ function constructNewChildrenArray( } skewedIndex = i + skew; - if ( - typeof childVNode.type == 'function' && - (matchingIndex !== skewedIndex || - oldVNode._children === childVNode._children) - ) { - childVNode._insert = true; - } else if ( - typeof childVNode.type != 'function' && - (matchingIndex !== skewedIndex || isMounting) - ) { - childVNode._insert = true; - } + childVNode._insert = + matchingIndex !== skewedIndex || + (typeof childVNode.type != 'function' && isMounting); } return newChildren; diff --git a/test/browser/fragments.test.js b/test/browser/fragments.test.js index f04a80d3a7..bccf2165e8 100644 --- a/test/browser/fragments.test.js +++ b/test/browser/fragments.test.js @@ -7,7 +7,7 @@ import { logCall, clearLog, getLog } from '../_util/logCall'; /** @jsx createElement */ /* eslint-disable react/jsx-boolean-value */ -describe('Fragment', () => { +describe.only('Fragment', () => { /** @type {HTMLDivElement} */ let scratch; @@ -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)' ]); }); @@ -843,12 +843,12 @@ describe('Fragment', () => { expect(ops).to.deep.equal([]); // Component should not have updated (empty op log) expect(scratch.innerHTML).to.equal(html); expectDomLogToBe([ + '1.remove()', + '
    Hello.remove()', '.appendChild(#text)', - '
    1Hello2.insertBefore(1, 1)', + '
    2.insertBefore(1, 2)', '
    .appendChild(#text)', - '
    11Hello2.insertBefore(
    Hello, 1)', - '1.remove()', - '
    Hello.remove()' + '
    12.insertBefore(
    Hello, 2)' ]); clearLog(); @@ -857,12 +857,12 @@ describe('Fragment', () => { expect(ops).to.deep.equal([]); // Component should not have updated (empty op log) expect(scratch.innerHTML).to.equal(html); expectDomLogToBe([ + '1.remove()', + '
    Hello.remove()', '.appendChild(#text)', - '
    1Hello2.insertBefore(1, 1)', + '
    2.insertBefore(1, 2)', '
    .appendChild(#text)', - '
    11Hello2.insertBefore(
    Hello, 1)', - '1.remove()', - '
    Hello.remove()' + '
    12.insertBefore(
    Hello, 2)' ]); }); @@ -925,9 +925,9 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal('foobar'); expectDomLogToBe([ - '
    spamfoobar.insertBefore(#text, #text)', '#text.remove()', - '#text.remove()' + '#text.remove()', + '
    foo.appendChild(#text)' ]); }); @@ -1449,14 +1449,14 @@ describe('Fragment', () => { 'rendering from false to true' ); expectDomLogToBe([ + // Remove 3 & 4 (replaced by null) + '
  • 3.remove()', + '
  • 4.remove()', // Insert 0 and 1 '
  • .appendChild(#text)', - '
      2234.insertBefore(
    1. 0,
    2. 2)', + '
        22.insertBefore(
      1. 0,
      2. 2)', '
      3. .appendChild(#text)', - '
          02234.insertBefore(
        1. 1,
        2. 2)', - // Remove 3 & 4 (replaced by null) - '
        3. 3.remove()', - '
        4. 4.remove()' + '
            022.insertBefore(
          1. 1,
          2. 2)' ]); }); @@ -1520,14 +1520,14 @@ describe('Fragment', () => { 'rendering from false to true' ); expectDomLogToBe([ + // Remove 4 & 5 (replaced by null) + '
          3. 4.remove()', + '
          4. 5.remove()', // Insert 0 and 1 back into the DOM '
          5. .appendChild(#text)', - '
              2345.insertBefore(
            1. 0,
            2. 2)', + '
                23.insertBefore(
              1. 0,
              2. 2)', '
              3. .appendChild(#text)', - '
                  02345.insertBefore(
                1. 1,
                2. 2)', - // Remove 4 & 5 (replaced by null) - '
                3. 4.remove()', - '
                4. 5.remove()' + '
                    023.insertBefore(
                  1. 1,
                  2. 2)' ]); }); @@ -1581,10 +1581,10 @@ describe('Fragment', () => { ); expectDomLogToBe( [ - '
                    barHellobeepboop.insertBefore(
                    bar,
                    beep)', - '
                    Hellobarbeepboop.insertBefore(
                    Hello,
                    boop)', - '
                    barbeepHelloboop.insertBefore(
                    bar,
                    boop)', - '
                    boop.remove()' + '
                    boop.remove()', + '
                    barHellobeep.insertBefore(
                    bar,
                    beep)', + '
                    Hellobarbeep.appendChild(
                    Hello)', + '
                    barbeepHello.appendChild(
                    bar)' ], 'rendering from true to false' ); @@ -1776,12 +1776,12 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal(htmlForFalse); expectDomLogToBe( [ + '
                    2.remove()', + '#text.remove()', '
                    .appendChild(#text)', - '
                    1.insertBefore(
                    3, #text)', + '
                    .appendChild(
                    3)', '
                    .appendChild(#text)', - '
                    31.insertBefore(
                    4, #text)', - '#text.remove()', - '
                    2.remove()' + '
                    3.appendChild(
                    4)' ], 'rendering from true to false' ); @@ -1792,9 +1792,9 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal(htmlForTrue); expectDomLogToBe( [ - '
                    34.insertBefore(#text,
                    3)', - '
                    4.remove()', '
                    3.remove()', + '
                    4.remove()', + '
                    .appendChild(#text)', '
                    .appendChild(#text)', '
                    1.appendChild(
                    2)' ], @@ -2056,9 +2056,9 @@ describe('Fragment', () => { `
                    A
                    B2
                    C
                    ` ); expectDomLogToBe([ + '
                    B1.remove()', '
                    .appendChild(#text)', - '
                    AB1C.insertBefore(
                    B2,
                    B1)', - '
                    B1.remove()' + '
                    AC.insertBefore(
                    B2,
                    C)' ]); }); @@ -2106,12 +2106,12 @@ describe('Fragment', () => { div([div('A'), section('B3'), section('B4'), div('C')]) ); expectDomLogToBe([ + '
                    B1.remove()', + '
                    B2.remove()', '
                    .appendChild(#text)', - '
                    AB1B2C.insertBefore(
                    B3,
                    B1)', + '
                    AC.insertBefore(
                    B3,
                    C)', '
                    .appendChild(#text)', - '
                    AB3B1B2C.insertBefore(
                    B4,
                    B1)', - '
                    B2.remove()', - '
                    B1.remove()' + '
                    AB3C.insertBefore(
                    B4,
                    C)' ]); }); @@ -2424,9 +2424,9 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.eql(`
                    A2
                    C
                    `); expectDomLogToBe([ + '
                    A.remove()', '.appendChild(#text)', - '
                    AC.insertBefore(A2,
                    A)', - '
                    A.remove()' + '
                    C.insertBefore(A2,
                    C)' ]); }); @@ -2491,12 +2491,12 @@ describe('Fragment', () => { `
                    A3A4
                    C
                    ` ); expectDomLogToBe([ + '
                    A1.remove()', + '
                    A2.remove()', '.appendChild(#text)', - '
                    A1A2C.insertBefore(A3,
                    A1)', + '
                    C.insertBefore(A3,
                    C)', '.appendChild(#text)', - '
                    A3A1A2C.insertBefore(A4,
                    A1)', - '
                    A2.remove()', - '
                    A1.remove()' + '
                    A3C.insertBefore(A4,
                    C)' ]); }); @@ -2572,9 +2572,9 @@ describe('Fragment', () => { 'updateA' ); expectDomLogToBe([ + '
                    A.remove()', '.appendChild(#text)', - '
                    ABC.insertBefore(A2,
                    A)', - '
                    A.remove()' + '
                    BC.insertBefore(A2,
                    B)' ]); }); @@ -2629,12 +2629,12 @@ describe('Fragment', () => { 'updateA' ); expectDomLogToBe([ + '
                    A1.remove()', + '
                    A2.remove()', '.appendChild(#text)', - '
                    A1A2.insertBefore(A3,
                    A1)', + '
                    .appendChild(A3)', '.appendChild(#text)', - '
                    A3A1A2.insertBefore(A4,
                    A1)', - '
                    A2.remove()', - '
                    A1.remove()' + '
                    A3.appendChild(A4)' ]); clearLog(); @@ -2652,7 +2652,8 @@ describe('Fragment', () => { }); it('should properly place conditional elements around strictly equal vnodes', () => { - let set; + /** @type {() => void} */ + let toggle; const Children = () => ( @@ -2665,7 +2666,7 @@ describe('Fragment', () => { constructor(props) { super(props); this.state = { panelPosition: 'bottom' }; - set = this.tooglePanelPosition = this.tooglePanelPosition.bind(this); + toggle = this.tooglePanelPosition = this.tooglePanelPosition.bind(this); } tooglePanelPosition() { @@ -2699,17 +2700,17 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal(bottom); clearLog(); - set(); + toggle(); rerender(); expect(scratch.innerHTML).to.equal(top); expectDomLogToBe([ + '
                    bottom panel.remove()', '
                    .appendChild(#text)', - '
                    NavigationContentbottom panel.insertBefore(
                    top panel,
                    Navigation)', - '
                    bottom panel.remove()' + '
                    NavigationContent.insertBefore(
                    top panel,
                    Navigation)' ]); clearLog(); - set(); + toggle(); rerender(); expect(scratch.innerHTML).to.equal(bottom); expectDomLogToBe([ @@ -2719,13 +2720,13 @@ describe('Fragment', () => { ]); clearLog(); - set(); + toggle(); rerender(); expect(scratch.innerHTML).to.equal(top); expectDomLogToBe([ + '
                    bottom panel.remove()', '
                    .appendChild(#text)', - '
                    NavigationContentbottom panel.insertBefore(
                    top panel,
                    Navigation)', - '
                    bottom panel.remove()' + '
                    NavigationContent.insertBefore(
                    top panel,
                    Navigation)' ]); }); @@ -2941,10 +2942,10 @@ describe('Fragment', () => { div([div(1), div(4), div('A'), div('B')]) ); expectDomLogToBe([ - '
                    .appendChild(#text)', - '
                    123AB.insertBefore(
                    4,
                    2)', '
                    2.remove()', - '
                    3.remove()' + '
                    3.remove()', + '
                    .appendChild(#text)', + '
                    1AB.insertBefore(
                    4,
                    A)' ]); }); @@ -3039,11 +3040,11 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal(div([span(1), div('A'), div('B')])); expectDomLogToBe([ - '.appendChild(#text)', - '
                    123AB.insertBefore(1,
                    1)', + '
                    1.remove()', '
                    2.remove()', '
                    3.remove()', - '
                    1.remove()' + '.appendChild(#text)', + '
                    AB.insertBefore(1,
                    A)' ]); }); From b23dd6f92e46e8babcadfda56bb1413acc3c93c9 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Sat, 21 Oct 2023 01:21:10 -0700 Subject: [PATCH 06/15] Enable & fix all tests but compat --- jsx-runtime/src/index.js | 4 ++++ karma.conf.js | 2 +- src/create-element.js | 2 ++ test/browser/createContext.test.js | 2 +- test/browser/fragments.test.js | 2 +- test/browser/keys.test.js | 2 +- test/browser/placeholders.test.js | 2 +- test/browser/render.test.js | 2 +- 8 files changed, 12 insertions(+), 6 deletions(-) diff --git a/jsx-runtime/src/index.js b/jsx-runtime/src/index.js index b578b9da7e..c38b1497e4 100644 --- a/jsx-runtime/src/index.js +++ b/jsx-runtime/src/index.js @@ -53,6 +53,10 @@ function createVNode(type, props, key, isStaticChildren, __source, __self) { _hydrating: null, constructor: undefined, _original: --vnodeId, + _prevVNode: undefined, + _insert: false, + _matched: false, + _index: -1, __source, __self }; diff --git a/karma.conf.js b/karma.conf.js index bec8c1ad76..858f8e3f06 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -319,7 +319,7 @@ module.exports = function (config) { { pattern: config.grep || - '{debug,devtools,hooks,compat,test-utils,jsx-runtime,}/test/{browser,shared}/**/*.test.js', + '{debug,devtools,hooks,test-utils,jsx-runtime,}/test/{browser,shared}/**/*.test.js', watched: false, type: 'js' } diff --git a/src/create-element.js b/src/create-element.js index 10572838b5..f5782f5139 100644 --- a/src/create-element.js +++ b/src/create-element.js @@ -73,6 +73,8 @@ export function createVNode(type, props, key, ref, original) { _hydrating: null, constructor: undefined, _original: original == null ? ++vnodeId : original, + // TODO: Consolidate new VNode fields and add mangle.json entries for new + // ones that stay _prevVNode: undefined, _insert: false, _matched: false, 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 bccf2165e8..20fed865e4 100644 --- a/test/browser/fragments.test.js +++ b/test/browser/fragments.test.js @@ -7,7 +7,7 @@ import { logCall, clearLog, getLog } from '../_util/logCall'; /** @jsx createElement */ /* eslint-disable react/jsx-boolean-value */ -describe.only('Fragment', () => { +describe('Fragment', () => { /** @type {HTMLDivElement} */ let scratch; diff --git a/test/browser/keys.test.js b/test/browser/keys.test.js index 1130ad14c6..6ad8789ca0 100644 --- a/test/browser/keys.test.js +++ b/test/browser/keys.test.js @@ -6,7 +6,7 @@ import { setupRerender } from 'preact/test-utils'; /** @jsx createElement */ -describe.only('keys', () => { +describe('keys', () => { /** @type {HTMLDivElement} */ let scratch; diff --git a/test/browser/placeholders.test.js b/test/browser/placeholders.test.js index ab0cc2e9e5..0fbdb6f181 100644 --- a/test/browser/placeholders.test.js +++ b/test/browser/placeholders.test.js @@ -6,7 +6,7 @@ import { div } from '../_util/dom'; /** @jsx createElement */ -describe.only('null placeholders', () => { +describe('null placeholders', () => { /** @type {HTMLDivElement} */ let scratch; diff --git a/test/browser/render.test.js b/test/browser/render.test.js index 62c6b3ad8d..a8be04376c 100644 --- a/test/browser/render.test.js +++ b/test/browser/render.test.js @@ -26,7 +26,7 @@ function getAttributes(node) { const isIE11 = /Trident\//.test(navigator.userAgent); -describe.only('render()', () => { +describe('render()', () => { let scratch, rerender; let resetAppendChild; From 79f0c82296407c8f2b19f1bfb8a3dfe794946914 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Sat, 21 Oct 2023 01:46:51 -0700 Subject: [PATCH 07/15] Fix linting error --- src/diff/children.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/diff/children.js b/src/diff/children.js index ba5d0ed80b..4db52406cc 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -27,6 +27,7 @@ export const diffChildren = diffChildren2; * @param {boolean} isHydrating Whether or not we are in hydration * @param {Array} refQueue an array of elements needed to invoke refs */ +// eslint-disable-next-line no-unused-vars function diffChildren1( parentDom, renderResult, @@ -282,6 +283,7 @@ function diffChildren1( * @param {boolean} isHydrating Whether or not we are in hydration * @param {Array} refQueue an array of elements needed to invoke refs */ +// eslint-disable-next-line no-unused-vars function diffChildren2( parentDom, renderResult, From 09db106385b000b1ed754b98b7c5c630c54f1e65 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Sat, 21 Oct 2023 02:11:16 -0700 Subject: [PATCH 08/15] Add mangle config so minified tests pass --- jsx-runtime/src/index.js | 2 +- mangle.json | 4 ++++ src/create-element.js | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/jsx-runtime/src/index.js b/jsx-runtime/src/index.js index c38b1497e4..ffbb0cae19 100644 --- a/jsx-runtime/src/index.js +++ b/jsx-runtime/src/index.js @@ -53,7 +53,7 @@ function createVNode(type, props, key, isStaticChildren, __source, __self) { _hydrating: null, constructor: undefined, _original: --vnodeId, - _prevVNode: undefined, + _prevVNode: null, _insert: false, _matched: false, _index: -1, diff --git a/mangle.json b/mangle.json index 22b96e72bf..d5b10b0c67 100644 --- a/mangle.json +++ b/mangle.json @@ -54,6 +54,10 @@ "$_dom": "__e", "$_hydrating": "__h", "$_component": "__c", + "$_prevVNode": "__u", + "$_insert": "__r", + "$_matched": "__s", + "$_index": "__i", "$__html": "__html", "$_parent": "__", "$_pendingError": "__E", diff --git a/src/create-element.js b/src/create-element.js index f5782f5139..dea86e1bca 100644 --- a/src/create-element.js +++ b/src/create-element.js @@ -75,7 +75,7 @@ export function createVNode(type, props, key, ref, original) { _original: original == null ? ++vnodeId : original, // TODO: Consolidate new VNode fields and add mangle.json entries for new // ones that stay - _prevVNode: undefined, + _prevVNode: null, _insert: false, _matched: false, _index: -1 From 4095c1a9e6126478e3cb396d44ddbb889daa67c5 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Sat, 21 Oct 2023 02:44:03 -0700 Subject: [PATCH 09/15] Fix suspense tests WIP: suspense hydration & suspense list --- src/diff/children.js | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/diff/children.js b/src/diff/children.js index 4db52406cc..c92a7f7bf6 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -536,16 +536,23 @@ function constructNewChildrenArray( remainingOldChildren ); - childVNode._prevVNode = oldVNode = - oldChildren[matchingIndex] || EMPTY_VNODE; - - let isMounting = oldVNode === EMPTY_VNODE || oldVNode._original === null; - if (!isMounting) { + if (matchingIndex === -1) { + oldVNode = EMPTY_VNODE; + } else if (matchingIndex !== -1) { // Mark oldVNode as matched so it isn't unmounted - oldVNode._matched = true; + oldVNode = oldChildren[matchingIndex] || EMPTY_VNODE; + if (oldVNode !== EMPTY_VNODE) { + oldVNode._matched = true; + } remainingOldChildren--; } + childVNode._prevVNode = oldVNode; + + // 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 = oldVNode === EMPTY_VNODE || oldVNode._original === null; if (isMounting) { if (matchingIndex == -1) { skew--; From e7847df66e024345de32703042574d957af95de9 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Sat, 21 Oct 2023 18:31:15 -0700 Subject: [PATCH 10/15] Fix suspense hydration tests Comparing children arrays in diffChildren needs to use tripe-equals strict equality to avoid having `null` and `undefined` appear equal. Children is `undefined` when mounting and oldVNode is EMPTY_OBJ. Children can be null when a component renders null. --- karma.conf.js | 2 +- src/diff/children.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/karma.conf.js b/karma.conf.js index 858f8e3f06..bec8c1ad76 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -319,7 +319,7 @@ module.exports = function (config) { { pattern: config.grep || - '{debug,devtools,hooks,test-utils,jsx-runtime,}/test/{browser,shared}/**/*.test.js', + '{debug,devtools,hooks,compat,test-utils,jsx-runtime,}/test/{browser,shared}/**/*.test.js', watched: false, type: 'js' } diff --git a/src/diff/children.js b/src/diff/children.js index c92a7f7bf6..3461b36014 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -375,7 +375,7 @@ function diffChildren2( if ( childVNode._insert || (typeof childVNode.type == 'function' && - childVNode._children == childVNode._prevVNode._children) + childVNode._children === childVNode._prevVNode._children) ) { if (typeof childVNode.type == 'function') { oldDom = reorderChildren(childVNode, oldDom, parentDom); From 6e9ba3fa2ac0a2390a76dc23e05d39b64aeaac0a Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Tue, 24 Oct 2023 07:45:26 -0600 Subject: [PATCH 11/15] Fix suspense-list tests (investigate further) --- src/component.js | 2 +- src/diff/children.js | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/component.js b/src/component.js index 88817c2d10..2270391120 100644 --- a/src/component.js +++ b/src/component.js @@ -103,7 +103,7 @@ export function getDomSibling(vnode, childIndex) { // Since updateParentDomPointers keeps _dom pointer correct, // we can rely on _dom to tell us if this subtree contains a // rendered DOM node, and what the first rendered DOM node is - return sibling._nextDom || sibling._dom; + return sibling._dom; } } diff --git a/src/diff/children.js b/src/diff/children.js index 3461b36014..1043004f8e 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -392,6 +392,7 @@ function diffChildren2( // 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 + // TODO: Should we always set this to undefined to ensure it is cleaned up? childVNode._nextDom = undefined; } else { oldDom = newDom.nextSibling; From eb3dddc1519a92dbf6e786a5d2bf136da5bf30f3 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Thu, 26 Oct 2023 08:26:55 -0600 Subject: [PATCH 12/15] Reuse iterator variable in second loop --- src/diff/children.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/diff/children.js b/src/diff/children.js index 1043004f8e..7922f005f5 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -333,7 +333,7 @@ function diffChildren2( } } - for (let i = 0; i < newChildrenLength; i++) { + for (i = 0; i < newChildrenLength; i++) { childVNode = newChildren[i]; if ( From 08e4a2bc42d066b4ef1204113020e7a043ea3fb4 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Thu, 26 Oct 2023 08:32:25 -0600 Subject: [PATCH 13/15] Remove _prevVNode field and reuse _index to temporarily hold matchingIndex... So diffChildren can get the oldVNode out of oldChildren using _index before setting it to the VNode final position/index --- jsx-runtime/src/index.js | 1 - src/create-element.js | 1 - src/diff/children.js | 87 ++++++++++++++++++++++++++-------------- src/internal.d.ts | 1 - 4 files changed, 56 insertions(+), 34 deletions(-) diff --git a/jsx-runtime/src/index.js b/jsx-runtime/src/index.js index ffbb0cae19..7fc508bfe4 100644 --- a/jsx-runtime/src/index.js +++ b/jsx-runtime/src/index.js @@ -53,7 +53,6 @@ function createVNode(type, props, key, isStaticChildren, __source, __self) { _hydrating: null, constructor: undefined, _original: --vnodeId, - _prevVNode: null, _insert: false, _matched: false, _index: -1, diff --git a/src/create-element.js b/src/create-element.js index dea86e1bca..e0853ef33a 100644 --- a/src/create-element.js +++ b/src/create-element.js @@ -75,7 +75,6 @@ export function createVNode(type, props, key, ref, original) { _original: original == null ? ++vnodeId : original, // TODO: Consolidate new VNode fields and add mangle.json entries for new // ones that stay - _prevVNode: null, _insert: false, _matched: false, _index: -1 diff --git a/src/diff/children.js b/src/diff/children.js index 7922f005f5..70bdf11a54 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -297,7 +297,14 @@ function diffChildren2( isHydrating, refQueue ) { - let i, j, childVNode, newDom, firstChildDom; + let i, + j, + /** @type {import('../internal').VNode} */ + oldVNode, + /** @type {import('../internal').VNode} */ + childVNode, + newDom, + firstChildDom; // This is a compression of oldParentVNode!=null && oldParentVNode != EMPTY_VNODE && oldParentVNode._children || EMPTY_ARR // as EMPTY_VNODE._children should be `undefined`. @@ -322,14 +329,16 @@ function diffChildren2( // 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. + // point to the next child, which needs to be the first DOM node that won't be + // unmounted. for (i = 0; i < oldChildrenLength; i++) { - if (oldChildren[i] != null && !oldChildren[i]._matched) { - if (oldDom == oldChildren[i]._dom) { - oldDom = getDomSibling(oldChildren[i]); + oldVNode = oldChildren[i]; + if (oldVNode != null && !oldVNode._matched) { + if (oldDom == oldVNode._dom) { + oldDom = getDomSibling(oldVNode); } - unmount(oldChildren[i], oldChildren[i]); + unmount(oldVNode, oldVNode); } } @@ -344,10 +353,21 @@ function diffChildren2( 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_VNODE; + } else { + oldVNode = oldChildren[childVNode._index] || EMPTY_VNODE; + } + + // Update childVNode._index to its final index + childVNode._index = i; + diff( parentDom, childVNode, - childVNode._prevVNode, + oldVNode, globalContext, isSvg, excessDomChildren, @@ -359,9 +379,9 @@ function diffChildren2( // Adjust DOM nodes newDom = childVNode._dom; - if ((j = childVNode.ref) && childVNode._prevVNode.ref != j) { - if (childVNode._prevVNode.ref) { - applyRef(childVNode._prevVNode.ref, null, childVNode); + if ((j = childVNode.ref) && oldVNode.ref != j) { + if (oldVNode.ref) { + applyRef(oldVNode.ref, null, childVNode); } refQueue.push(j, childVNode._component || newDom, childVNode); } @@ -375,7 +395,7 @@ function diffChildren2( if ( childVNode._insert || (typeof childVNode.type == 'function' && - childVNode._children === childVNode._prevVNode._children) + childVNode._children === oldVNode._children) ) { if (typeof childVNode.type == 'function') { oldDom = reorderChildren(childVNode, oldDom, parentDom); @@ -413,7 +433,6 @@ function diffChildren2( } // Unset diffing properties - childVNode._prevVNode = null; childVNode._insert = false; childVNode._matched = false; } @@ -438,8 +457,6 @@ function constructNewChildrenArray( let i; /** @type {import('../internal').VNode} */ let childVNode; - /** @type {import('../internal').VNode} */ - let oldVNode; let newChildrenLength = renderResult.length; let remainingOldChildren = oldChildren.length; let skew = 0; @@ -500,7 +517,8 @@ function constructNewChildrenArray( // Handle unmounting null placeholders, i.e. VNode => null in unkeyed children if (childVNode == null) { - oldVNode = oldChildren[i]; + /** @type {import('../internal').VNode} */ + let oldVNode = oldChildren[i]; if (oldVNode && oldVNode.key == null && oldVNode._dom) { if (oldVNode._dom == oldDomRef.current) { // oldVNode._parent = oldParentVNode; @@ -527,7 +545,6 @@ function constructNewChildrenArray( childVNode._parent = newParentVNode; childVNode._depth = newParentVNode._depth + 1; - childVNode._index = i; let skewedIndex = i + skew; const matchingIndex = findMatchingIndex( @@ -537,23 +554,30 @@ function constructNewChildrenArray( remainingOldChildren ); - if (matchingIndex === -1) { - oldVNode = EMPTY_VNODE; - } else if (matchingIndex !== -1) { - // Mark oldVNode as matched so it isn't unmounted - oldVNode = oldChildren[matchingIndex] || EMPTY_VNODE; - if (oldVNode !== EMPTY_VNODE) { - oldVNode._matched = true; - } + // 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; + + 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; + } } - childVNode._prevVNode = oldVNode; + // // 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; - // 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 = oldVNode === EMPTY_VNODE || oldVNode._original === null; if (isMounting) { if (matchingIndex == -1) { skew--; @@ -579,9 +603,10 @@ function constructNewChildrenArray( } } - skewedIndex = i + skew; + // 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 childVNode._insert = - matchingIndex !== skewedIndex || + matchingIndex !== i + skew || (typeof childVNode.type != 'function' && isMounting); } diff --git a/src/internal.d.ts b/src/internal.d.ts index ded94b41c8..52ce76e103 100644 --- a/src/internal.d.ts +++ b/src/internal.d.ts @@ -123,7 +123,6 @@ export interface VNode

                    extends preact.VNode

                    { constructor: undefined; _original: number; // New properties - _prevVNode: VNode; _insert: boolean; _matched: boolean; _index: number; From c60ff18dbd7fb3ac68c721b811db02046b40474f Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Thu, 26 Oct 2023 08:38:16 -0600 Subject: [PATCH 14/15] Some golfing --- src/diff/children.js | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/diff/children.js b/src/diff/children.js index 70bdf11a54..a310fbb0bf 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -298,7 +298,6 @@ function diffChildren2( refQueue ) { let i, - j, /** @type {import('../internal').VNode} */ oldVNode, /** @type {import('../internal').VNode} */ @@ -317,7 +316,7 @@ function diffChildren2( // 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 oldDomRef = { _current: oldDom }; const newChildren = (newParentVNode._children = constructNewChildrenArray( newParentVNode, renderResult, @@ -325,7 +324,7 @@ function diffChildren2( oldDomRef )); - oldDom = oldDomRef.current; + 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 @@ -379,11 +378,15 @@ function diffChildren2( // Adjust DOM nodes newDom = childVNode._dom; - if ((j = childVNode.ref) && oldVNode.ref != j) { + if (childVNode.ref && oldVNode.ref != childVNode.ref) { if (oldVNode.ref) { applyRef(oldVNode.ref, null, childVNode); } - refQueue.push(j, childVNode._component || newDom, childVNode); + refQueue.push( + childVNode.ref, + childVNode._component || newDom, + childVNode + ); } if (newDom != null) { @@ -444,7 +447,7 @@ function diffChildren2( * @param {import('../internal').VNode} newParentVNode * @param {import('../internal').ComponentChildren[]} renderResult * @param {import('../internal').VNode[]} oldChildren - * @param {{ current: import('../internal').PreactElement }} oldDomRef + * @param {{ _current: import('../internal').PreactElement }} oldDomRef * @returns {import('../internal').VNode[]} */ function constructNewChildrenArray( @@ -520,9 +523,8 @@ function constructNewChildrenArray( /** @type {import('../internal').VNode} */ let oldVNode = oldChildren[i]; if (oldVNode && oldVNode.key == null && oldVNode._dom) { - if (oldVNode._dom == oldDomRef.current) { - // oldVNode._parent = oldParentVNode; - oldDomRef.current = getDomSibling(oldVNode); + if (oldVNode._dom == oldDomRef._current) { + oldDomRef._current = getDomSibling(oldVNode); } unmount(oldVNode, oldVNode, false); From 184d5787d9066c05b7cab7a909dff262b6829837 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 27 Oct 2023 23:10:58 -0700 Subject: [PATCH 15/15] Replace _matched & _insert with _flags --- jsx-runtime/src/index.js | 3 +-- mangle.json | 2 +- src/constants.js | 7 +++++++ src/create-element.js | 3 +-- src/diff/children.js | 32 +++++++++++++++++++++----------- src/internal.d.ts | 3 +-- 6 files changed, 32 insertions(+), 18 deletions(-) diff --git a/jsx-runtime/src/index.js b/jsx-runtime/src/index.js index 7fc508bfe4..40bfe1b395 100644 --- a/jsx-runtime/src/index.js +++ b/jsx-runtime/src/index.js @@ -53,8 +53,7 @@ function createVNode(type, props, key, isStaticChildren, __source, __self) { _hydrating: null, constructor: undefined, _original: --vnodeId, - _insert: false, - _matched: false, + _flags: 0, _index: -1, __source, __self diff --git a/mangle.json b/mangle.json index d5b10b0c67..4747c8a6f9 100644 --- a/mangle.json +++ b/mangle.json @@ -54,7 +54,7 @@ "$_dom": "__e", "$_hydrating": "__h", "$_component": "__c", - "$_prevVNode": "__u", + "$_flags": "__u", "$_insert": "__r", "$_matched": "__s", "$_index": "__i", diff --git a/src/constants.js b/src/constants.js index bdaa7cd77a..fcc53a3115 100644 --- a/src/constants.js +++ b/src/constants.js @@ -1,3 +1,10 @@ +/** Indicates that this node needs to be inserted while patching children */ +export const INSERT_VNODE = 1 << 16; +export const MATCHED = 1 << 17; + +/** Reset all mode flags */ +export const RESET_MODE = ~(INSERT_VNODE | MATCHED); + export const EMPTY_OBJ = {}; export const EMPTY_VNODE = /** @type {import('./internal').VNode} */ ( EMPTY_OBJ diff --git a/src/create-element.js b/src/create-element.js index e0853ef33a..4255cadf6d 100644 --- a/src/create-element.js +++ b/src/create-element.js @@ -75,8 +75,7 @@ export function createVNode(type, props, key, ref, original) { _original: original == null ? ++vnodeId : original, // TODO: Consolidate new VNode fields and add mangle.json entries for new // ones that stay - _insert: false, - _matched: false, + _flags: 0, _index: -1 }; diff --git a/src/diff/children.js b/src/diff/children.js index a310fbb0bf..21a1a78a11 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -1,6 +1,13 @@ import { diff, unmount, applyRef } from './index'; import { createVNode, Fragment } from '../create-element'; -import { EMPTY_OBJ, EMPTY_ARR, EMPTY_VNODE } from '../constants'; +import { + EMPTY_OBJ, + EMPTY_ARR, + EMPTY_VNODE, + MATCHED, + INSERT_VNODE, + RESET_MODE +} from '../constants'; import { isArray } from '../util'; import { getDomSibling } from '../component'; @@ -332,7 +339,7 @@ function diffChildren2( // unmounted. for (i = 0; i < oldChildrenLength; i++) { oldVNode = oldChildren[i]; - if (oldVNode != null && !oldVNode._matched) { + if (oldVNode != null && (oldVNode._flags & MATCHED) === 0) { if (oldDom == oldVNode._dom) { oldDom = getDomSibling(oldVNode); } @@ -396,7 +403,7 @@ function diffChildren2( // TODO: Golf this `if` block if ( - childVNode._insert || + childVNode._flags & INSERT_VNODE || (typeof childVNode.type == 'function' && childVNode._children === oldVNode._children) ) { @@ -436,8 +443,7 @@ function diffChildren2( } // Unset diffing properties - childVNode._insert = false; - childVNode._matched = false; + childVNode._flags &= RESET_MODE; } newParentVNode._dom = firstChildDom; @@ -568,7 +574,7 @@ function constructNewChildrenArray( // 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; + oldChildren[matchingIndex]._flags |= MATCHED; } } @@ -607,9 +613,12 @@ function constructNewChildrenArray( // 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 - childVNode._insert = + if ( matchingIndex !== i + skew || - (typeof childVNode.type != 'function' && isMounting); + (typeof childVNode.type != 'function' && isMounting) + ) { + childVNode._flags |= INSERT_VNODE; + } } return newChildren; @@ -696,7 +705,8 @@ function findMatchingIndex( // 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); + remainingOldChildren > + (oldVNode != null && (oldVNode._flags & MATCHED) === 0 ? 1 : 0); if ( oldVNode === null || @@ -709,7 +719,7 @@ function findMatchingIndex( oldVNode = oldChildren[x]; if ( oldVNode && - !oldVNode._matched && + (oldVNode._flags & MATCHED) === 0 && key == oldVNode.key && type === oldVNode.type ) { @@ -722,7 +732,7 @@ function findMatchingIndex( oldVNode = oldChildren[y]; if ( oldVNode && - !oldVNode._matched && + (oldVNode._flags & MATCHED) === 0 && key == oldVNode.key && type === oldVNode.type ) { diff --git a/src/internal.d.ts b/src/internal.d.ts index 52ce76e103..7ebf580c74 100644 --- a/src/internal.d.ts +++ b/src/internal.d.ts @@ -123,8 +123,7 @@ export interface VNode

                    extends preact.VNode

                    { constructor: undefined; _original: number; // New properties - _insert: boolean; - _matched: boolean; + _flags: number; _index: number; }