Skip to content

Commit

Permalink
Fix issues by correctly skipping comments in diffChildren
Browse files Browse the repository at this point in the history
  • Loading branch information
JoviDeCroock committed Jul 15, 2024
1 parent 1281599 commit 647d138
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 22 deletions.
19 changes: 5 additions & 14 deletions compat/test/browser/suspense-hydration.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ describe('suspense hydration', () => {
});

it('Should hydrate a fragment with no children correctly', () => {
scratch.innerHTML = '<!--$s--><div>Hello</div><div>World!</div><!--/$s-->';
scratch.innerHTML = '<!--$s--><!--/$s-->';
clearLog();

const [Lazy, resolve] = createLazy();
Expand All @@ -143,22 +143,13 @@ describe('suspense hydration', () => {
scratch
);
rerender(); // Flush rerender queue to mimic what preact will really do
expect(scratch.innerHTML).to.equal(
'<!--$s--><div>Hello</div><div>World!</div><!--/$s-->'
);
expect(scratch.innerHTML).to.equal('<!--$s--><!--/$s-->');
expect(getLog()).to.deep.equal([]);
clearLog();

return resolve(() => (
<>
<div>Hello</div>
<div>World!</div>
</>
)).then(() => {
return resolve(() => null).then(() => {
rerender();
expect(scratch.innerHTML).to.equal(
'<!--$s--><div>Hello</div><div>World!</div><!--/$s-->'
);
expect(scratch.innerHTML).to.equal('<!--$s--><!--/$s-->');
expect(getLog()).to.deep.equal([]);

clearLog();
Expand All @@ -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 = '<!--$s--><!--/$s--><div>Baz</div>';
clearLog();

Expand Down
6 changes: 5 additions & 1 deletion src/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,11 @@ export function getDomSibling(vnode, childIndex) {
for (; childIndex < vnode._children.length; childIndex++) {
sibling = vnode._children[childIndex];

if (sibling != null && sibling._dom != null) {
if (
sibling != null &&
sibling._dom != null &&
sibling._dom.nodeType !== 8
) {
// 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
Expand Down
3 changes: 3 additions & 0 deletions src/diff/children.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ export function diffChildren(
oldDom = childVNode._nextDom;
} else if (newDom) {
oldDom = newDom.nextSibling;
while (oldDom && oldDom.nodeType == 8) {
oldDom = oldDom.nextSibling;
}
}

// Eagerly cleanup _nextDom. We don't need to persist the value because it
Expand Down
20 changes: 13 additions & 7 deletions src/diff/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export function diff(
isHydrating = !!(oldVNode._flags & MODE_HYDRATE);
if (oldVNode._excess) {
excessDomChildren = oldVNode._excess;
oldDom = newVNode._dom = oldVNode._dom = excessDomChildren[1];
oldDom = newVNode._dom = oldVNode._dom = excessDomChildren[0];
} else {
oldDom = newVNode._dom = oldVNode._dom;
excessDomChildren = [oldDom];
Expand Down Expand Up @@ -283,26 +283,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._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._excess.push(node);
}
}
} else {
Expand Down

0 comments on commit 647d138

Please sign in to comment.