From 1281599c59ae144297810fef667c07f5192cc48e Mon Sep 17 00:00:00 2001 From: jdecroock Date: Mon, 15 Jul 2024 09:26:08 +0200 Subject: [PATCH 1/5] Test approach to comment markers --- .../test/browser/suspense-hydration.test.js | 98 +++++++++++++++++++ jsx-runtime/src/index.js | 1 + mangle.json | 1 + src/create-element.js | 1 + src/diff/index.js | 40 ++++++-- src/internal.d.ts | 1 + 6 files changed, 136 insertions(+), 6 deletions(-) diff --git a/compat/test/browser/suspense-hydration.test.js b/compat/test/browser/suspense-hydration.test.js index 9143903238..fd61a2bb02 100644 --- a/compat/test/browser/suspense-hydration.test.js +++ b/compat/test/browser/suspense-hydration.test.js @@ -97,6 +97,104 @@ describe('suspense hydration', () => { }); }); + it('Should hydrate a fragment with multiple children correctly', () => { + scratch.innerHTML = '
Hello
World!
'; + clearLog(); + + const [Lazy, resolve] = createLazy(); + hydrate( + + + , + scratch + ); + rerender(); // Flush rerender queue to mimic what preact will really do + expect(scratch.innerHTML).to.equal( + '
Hello
World!
' + ); + expect(getLog()).to.deep.equal([]); + clearLog(); + + return resolve(() => ( + <> +
Hello
+
World!
+ + )).then(() => { + rerender(); + expect(scratch.innerHTML).to.equal( + '
Hello
World!
' + ); + expect(getLog()).to.deep.equal([]); + + clearLog(); + }); + }); + + it('Should hydrate a fragment with no children correctly', () => { + scratch.innerHTML = '
Hello
World!
'; + clearLog(); + + const [Lazy, resolve] = createLazy(); + hydrate( + + + , + scratch + ); + rerender(); // Flush rerender queue to mimic what preact will really do + expect(scratch.innerHTML).to.equal( + '
Hello
World!
' + ); + expect(getLog()).to.deep.equal([]); + clearLog(); + + return resolve(() => ( + <> +
Hello
+
World!
+ + )).then(() => { + rerender(); + expect(scratch.innerHTML).to.equal( + '
Hello
World!
' + ); + expect(getLog()).to.deep.equal([]); + + clearLog(); + }); + }); + + // This is in theory correct but still it shows that our oldDom becomes stale very quickly + // and moves DOM into weird places + it.skip('Should hydrate a fragment with no children and an adjacent node correctly', () => { + scratch.innerHTML = '
Baz
'; + clearLog(); + + const [Lazy, resolve] = createLazy(); + hydrate( + <> + + + +
Baz
+ , + scratch + ); + rerender(); // Flush rerender queue to mimic what preact will really do + expect(scratch.innerHTML).to.equal('
Baz
'); + expect(getLog()).to.deep.equal([]); + clearLog(); + + return resolve(() => null).then(() => { + rerender(); + expect(scratch.innerHTML).to.equal('
Baz
'); + expect(getLog()).to.deep.equal([]); + + clearLog(); + }); + }); + it('should properly attach event listeners when suspending while hydrating', () => { scratch.innerHTML = '
Hello
World
'; clearLog(); diff --git a/jsx-runtime/src/index.js b/jsx-runtime/src/index.js index 454df6308a..98551fc28a 100644 --- a/jsx-runtime/src/index.js +++ b/jsx-runtime/src/index.js @@ -59,6 +59,7 @@ function createVNode(type, props, key, isStaticChildren, __source, __self) { _nextDom: undefined, _component: null, constructor: undefined, + _excess: null, _original: --vnodeId, _index: -1, _flags: 0, diff --git a/mangle.json b/mangle.json index fcd2dce74f..2e67fca5a2 100644 --- a/mangle.json +++ b/mangle.json @@ -31,6 +31,7 @@ "$_list": "__", "$_pendingEffects": "__h", "$_value": "__", + "$_excess": "__x", "$_nextValue": "__N", "$_original": "__v", "$_args": "__H", diff --git a/src/create-element.js b/src/create-element.js index 66898b2224..ebecdf4037 100644 --- a/src/create-element.js +++ b/src/create-element.js @@ -66,6 +66,7 @@ export function createVNode(type, props, key, ref, original) { _parent: null, _depth: 0, _dom: null, + _excess: null, // _nextDom must be initialized to undefined b/c it will eventually // be set to dom.nextSibling which can return `null` and it is important // to be able to distinguish between an uninitialized _nextDom and diff --git a/src/diff/index.js b/src/diff/index.js index 3e4b17bd62..25e02fe702 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -52,8 +52,13 @@ export function diff( // If the previous diff bailed out, resume creating/hydrating. if (oldVNode._flags & MODE_SUSPENDED) { isHydrating = !!(oldVNode._flags & MODE_HYDRATE); - oldDom = newVNode._dom = oldVNode._dom; - excessDomChildren = [oldDom]; + if (oldVNode._excess) { + excessDomChildren = oldVNode._excess; + oldDom = newVNode._dom = oldVNode._dom = excessDomChildren[1]; + } else { + oldDom = newVNode._dom = oldVNode._dom; + excessDomChildren = [oldDom]; + } } if ((tmp = options._diff)) tmp(newVNode); @@ -273,13 +278,36 @@ export function diff( newVNode._original = null; // if hydrating or creating initial tree, bailout preserves DOM: if (isHydrating || excessDomChildren != null) { - newVNode._dom = oldDom; newVNode._flags |= isHydrating ? MODE_HYDRATE | MODE_SUSPENDED : MODE_HYDRATE; - excessDomChildren[excessDomChildren.indexOf(oldDom)] = null; - // ^ could possibly be simplified to: - // excessDomChildren.length = 0; + + let found = excessDomChildren.find( + child => child && child.nodeType == 8 && child.data == '$s' + ), + index = excessDomChildren.indexOf(found) + 1; + + newVNode._dom = oldDom; + if (found) { + let commentMarkersToFind = 1; + newVNode._excess = [found]; + excessDomChildren[index - 1] = null; + while (commentMarkersToFind && index <= excessDomChildren.length) { + const node = excessDomChildren[index]; + excessDomChildren[index] = null; + index++; + newVNode._excess.push(node); + if (node.nodeType == 8) { + if (node.data == '$s') { + commentMarkersToFind++; + } else if (node.data == '/$s') { + commentMarkersToFind--; + } + } + } + } else { + excessDomChildren[excessDomChildren.indexOf(oldDom)] = null; + } } else { newVNode._dom = oldVNode._dom; newVNode._children = oldVNode._children; diff --git a/src/internal.d.ts b/src/internal.d.ts index cbf23b3888..576a6ca523 100644 --- a/src/internal.d.ts +++ b/src/internal.d.ts @@ -145,6 +145,7 @@ declare global { * The [first (for Fragments)] DOM child of a VNode */ _dom: PreactElement | null; + _excess: PreactElement[] | null; /** * The last dom child of a Fragment, or components that return a Fragment */ From 7041375008c492db42b9f6146d1b61f15ca29c86 Mon Sep 17 00:00:00 2001 From: jdecroock Date: Mon, 15 Jul 2024 10:46:32 +0200 Subject: [PATCH 2/5] Fix issues by correctly skipping comments in diffChildren --- .../test/browser/suspense-hydration.test.js | 19 ++++--------- jsx-runtime/src/index.js | 1 - src/create-element.js | 1 - src/diff/children.js | 3 +++ src/diff/index.js | 27 ++++++++++++------- src/internal.d.ts | 2 +- 6 files changed, 27 insertions(+), 26 deletions(-) diff --git a/compat/test/browser/suspense-hydration.test.js b/compat/test/browser/suspense-hydration.test.js index fd61a2bb02..027cca24a8 100644 --- a/compat/test/browser/suspense-hydration.test.js +++ b/compat/test/browser/suspense-hydration.test.js @@ -132,7 +132,7 @@ describe('suspense hydration', () => { }); it('Should hydrate a fragment with no children correctly', () => { - scratch.innerHTML = '
Hello
World!
'; + scratch.innerHTML = ''; clearLog(); const [Lazy, resolve] = createLazy(); @@ -143,22 +143,13 @@ describe('suspense hydration', () => { scratch ); rerender(); // Flush rerender queue to mimic what preact will really do - expect(scratch.innerHTML).to.equal( - '
Hello
World!
' - ); + expect(scratch.innerHTML).to.equal(''); expect(getLog()).to.deep.equal([]); clearLog(); - return resolve(() => ( - <> -
Hello
-
World!
- - )).then(() => { + return resolve(() => null).then(() => { rerender(); - expect(scratch.innerHTML).to.equal( - '
Hello
World!
' - ); + expect(scratch.innerHTML).to.equal(''); expect(getLog()).to.deep.equal([]); clearLog(); @@ -167,7 +158,7 @@ describe('suspense hydration', () => { // This is in theory correct but still it shows that our oldDom becomes stale very quickly // and moves DOM into weird places - it.skip('Should hydrate a fragment with no children and an adjacent node correctly', () => { + it('Should hydrate a fragment with no children and an adjacent node correctly', () => { scratch.innerHTML = '
Baz
'; clearLog(); diff --git a/jsx-runtime/src/index.js b/jsx-runtime/src/index.js index 98551fc28a..454df6308a 100644 --- a/jsx-runtime/src/index.js +++ b/jsx-runtime/src/index.js @@ -59,7 +59,6 @@ function createVNode(type, props, key, isStaticChildren, __source, __self) { _nextDom: undefined, _component: null, constructor: undefined, - _excess: null, _original: --vnodeId, _index: -1, _flags: 0, diff --git a/src/create-element.js b/src/create-element.js index ebecdf4037..66898b2224 100644 --- a/src/create-element.js +++ b/src/create-element.js @@ -66,7 +66,6 @@ export function createVNode(type, props, key, ref, original) { _parent: null, _depth: 0, _dom: null, - _excess: null, // _nextDom must be initialized to undefined b/c it will eventually // be set to dom.nextSibling which can return `null` and it is important // to be able to distinguish between an uninitialized _nextDom and diff --git a/src/diff/children.js b/src/diff/children.js index 1e44c5cade..238768ddfe 100644 --- a/src/diff/children.js +++ b/src/diff/children.js @@ -135,6 +135,9 @@ export function diffChildren( oldDom = childVNode._nextDom; } else if (newDom) { oldDom = newDom.nextSibling; + while (oldDom && oldDom.nodeType == 8 && oldDom.nextSibling) { + oldDom = oldDom.nextSibling; + } } // Eagerly cleanup _nextDom. We don't need to persist the value because it diff --git a/src/diff/index.js b/src/diff/index.js index 25e02fe702..e74535e5a0 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -52,9 +52,12 @@ export function diff( // If the previous diff bailed out, resume creating/hydrating. if (oldVNode._flags & MODE_SUSPENDED) { isHydrating = !!(oldVNode._flags & MODE_HYDRATE); - if (oldVNode._excess) { - excessDomChildren = oldVNode._excess; - oldDom = newVNode._dom = oldVNode._dom = excessDomChildren[1]; + if (oldVNode._component._excess) { + excessDomChildren = oldVNode._component._excess; + // TODO: it's entirely possible for nested Suspense scenario's that we + // take another comment-node here as oldDom which isn't ideal however + // let's try it out for now. + oldDom = newVNode._dom = oldVNode._dom = excessDomChildren[0]; } else { oldDom = newVNode._dom = oldVNode._dom; excessDomChildren = [oldDom]; @@ -283,26 +286,32 @@ export function diff( : MODE_HYDRATE; let found = excessDomChildren.find( - child => child && child.nodeType == 8 && child.data == '$s' - ), - index = excessDomChildren.indexOf(found) + 1; + child => child && child.nodeType == 8 && child.data == '$s' + ); newVNode._dom = oldDom; if (found) { - let commentMarkersToFind = 1; - newVNode._excess = [found]; + let commentMarkersToFind = 1, + index = excessDomChildren.indexOf(found) + 1; + newVNode._component._excess = []; + // Clear the comment marker so we don't reuse them for sibling + // Suspenders. excessDomChildren[index - 1] = null; + while (commentMarkersToFind && index <= excessDomChildren.length) { const node = excessDomChildren[index]; excessDomChildren[index] = null; index++; - newVNode._excess.push(node); + // node being undefined here would be a problem as it would + // imply that we have a mismatch. if (node.nodeType == 8) { if (node.data == '$s') { commentMarkersToFind++; } else if (node.data == '/$s') { commentMarkersToFind--; } + } else { + newVNode._component._excess.push(node); } } } else { diff --git a/src/internal.d.ts b/src/internal.d.ts index 576a6ca523..e4c2461f16 100644 --- a/src/internal.d.ts +++ b/src/internal.d.ts @@ -145,7 +145,6 @@ declare global { * The [first (for Fragments)] DOM child of a VNode */ _dom: PreactElement | null; - _excess: PreactElement[] | null; /** * The last dom child of a Fragment, or components that return a Fragment */ @@ -163,6 +162,7 @@ declare global { state: S; // Override Component["state"] to not be readonly for internal use, specifically Hooks base?: PreactElement; + _excess: PreactElement[] | null; _dirty: boolean; _force?: boolean; _renderCallbacks: Array<() => void>; // Only class components From 38e1a604b53a83edc2a03744646e8a5517207301 Mon Sep 17 00:00:00 2001 From: jdecroock Date: Wed, 17 Jul 2024 11:08:54 +0200 Subject: [PATCH 3/5] convert to for loop --- src/diff/index.js | 68 +++++++++++++++++++++-------------------------- 1 file changed, 30 insertions(+), 38 deletions(-) diff --git a/src/diff/index.js b/src/diff/index.js index e74535e5a0..2e4c7176bd 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -52,16 +52,11 @@ export function diff( // If the previous diff bailed out, resume creating/hydrating. if (oldVNode._flags & MODE_SUSPENDED) { isHydrating = !!(oldVNode._flags & MODE_HYDRATE); - if (oldVNode._component._excess) { - excessDomChildren = oldVNode._component._excess; - // TODO: it's entirely possible for nested Suspense scenario's that we - // take another comment-node here as oldDom which isn't ideal however - // let's try it out for now. - oldDom = newVNode._dom = oldVNode._dom = excessDomChildren[0]; - } else { - oldDom = newVNode._dom = oldVNode._dom; - excessDomChildren = [oldDom]; - } + excessDomChildren = oldVNode._component._excess; + // TODO: it's entirely possible for nested Suspense scenario's that we + // take another comment-node here as oldDom which isn't ideal however + // let's try it out for now. + oldDom = newVNode._dom = oldVNode._dom = excessDomChildren[0]; } if ((tmp = options._diff)) tmp(newVNode); @@ -285,38 +280,35 @@ export function diff( ? MODE_HYDRATE | MODE_SUSPENDED : MODE_HYDRATE; - let found = excessDomChildren.find( - child => child && child.nodeType == 8 && child.data == '$s' - ); + let shouldFallback = true, + commentMarkersToFind = 0, + done = false; + newVNode._component._excess = []; + for (let i = 0; i < excessDomChildren.length; i++) { + let child = excessDomChildren[i]; + if (child == null || done) continue; - newVNode._dom = oldDom; - if (found) { - let commentMarkersToFind = 1, - index = excessDomChildren.indexOf(found) + 1; - newVNode._component._excess = []; - // Clear the comment marker so we don't reuse them for sibling - // Suspenders. - excessDomChildren[index - 1] = null; - - while (commentMarkersToFind && index <= excessDomChildren.length) { - const node = excessDomChildren[index]; - excessDomChildren[index] = null; - index++; - // node being undefined here would be a problem as it would - // imply that we have a mismatch. - if (node.nodeType == 8) { - if (node.data == '$s') { - commentMarkersToFind++; - } else if (node.data == '/$s') { - commentMarkersToFind--; - } - } else { - newVNode._component._excess.push(node); - } + if (commentMarkersToFind > 0) { + excessDomChildren[i] = null; } - } else { + + if (child.nodeType == 8 && child.data == '$s') { + commentMarkersToFind++; + shouldFallback = false; + } else if (child.nodeType == 8 && child.data == '/$s') { + commentMarkersToFind--; + done = commentMarkersToFind === 0; + } else if (commentMarkersToFind > 0) { + newVNode._component._excess.push(child); + } + } + + if (shouldFallback) { excessDomChildren[excessDomChildren.indexOf(oldDom)] = null; + newVNode._component._excess.push(oldDom); } + + newVNode._dom = oldDom; } else { newVNode._dom = oldVNode._dom; newVNode._children = oldVNode._children; From 02560eb0423fc26188b8aa1f542c29aad4fd2758 Mon Sep 17 00:00:00 2001 From: jdecroock Date: Wed, 17 Jul 2024 11:15:29 +0200 Subject: [PATCH 4/5] add comment --- src/diff/index.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/diff/index.js b/src/diff/index.js index 2e4c7176bd..143dbdb6df 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -283,20 +283,36 @@ export function diff( let shouldFallback = true, commentMarkersToFind = 0, done = false; + newVNode._component._excess = []; for (let i = 0; i < excessDomChildren.length; i++) { let child = excessDomChildren[i]; if (child == null || done) continue; + // When we are inside of the boundaries we set + // the excess child to null. if (commentMarkersToFind > 0) { excessDomChildren[i] = null; } + // When we encounter a boundary with $s we are opening + // a boundary, this implies that we need to bump + // the amount of markers we need to find before closing + // the outer boundary. + // We exclude the open and closing marker from + // the future excessDomChildren but any nested one + // needs to be included for future suspensions. if (child.nodeType == 8 && child.data == '$s') { + if (commentMarkersToFind > 0) { + newVNode._component._excess.push(child); + } commentMarkersToFind++; shouldFallback = false; } else if (child.nodeType == 8 && child.data == '/$s') { commentMarkersToFind--; + if (commentMarkersToFind > 0) { + newVNode._component._excess.push(child); + } done = commentMarkersToFind === 0; } else if (commentMarkersToFind > 0) { newVNode._component._excess.push(child); From b03cb784c1ff7982957a3f1ae8dad15ed008a5f1 Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Thu, 18 Jul 2024 20:59:14 +0200 Subject: [PATCH 5/5] add additional test --- .../test/browser/suspense-hydration.test.js | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/compat/test/browser/suspense-hydration.test.js b/compat/test/browser/suspense-hydration.test.js index a90f2c7a18..010aa15832 100644 --- a/compat/test/browser/suspense-hydration.test.js +++ b/compat/test/browser/suspense-hydration.test.js @@ -131,6 +131,38 @@ describe('suspense hydration', () => { }); }); + it('Should hydrate a fragment with no children correctly', () => { + scratch.innerHTML = '
Hello world
'; + clearLog(); + + const [Lazy, resolve] = createLazy(); + hydrate( + <> + + + +
Hello world
+ , + scratch + ); + rerender(); // Flush rerender queue to mimic what preact will really do + expect(scratch.innerHTML).to.equal( + '
Hello world
' + ); + expect(getLog()).to.deep.equal([]); + clearLog(); + + return resolve(() => null).then(() => { + rerender(); + expect(scratch.innerHTML).to.equal( + '
Hello world
' + ); + expect(getLog()).to.deep.equal([]); + + clearLog(); + }); + }); + it('should leave DOM untouched when suspending while hydrating', () => { scratch.innerHTML = '
Hello
'; clearLog();