Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Skip HTML comments during hydration #3771

Closed
wants to merge 3 commits into from
Closed

Skip HTML comments during hydration #3771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4158952
Tests: Fix DOM log not always reset
marvinhagemeister Oct 16, 2022
1487005
Ignore HTML comments during hydration
marvinhagemeister Oct 16, 2022
d35efab
golf
marvinhagemeister Oct 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/diff/children.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,12 @@ function placeChild(
if (nextDom !== undefined) {
oldDom = nextDom;
} else {
oldDom = newDom.nextSibling;
oldDom = newDom;
// Skip over comment nodes. Will be removed right after calling diffChildren
// in diff so that the vnode tree matches the DOM tree again.
do {
oldDom = oldDom.nextSibling;
} while (oldDom !== null && oldDom.nodeType === 8);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be possible to use a similar (or the same somehow?) check to the one we have for node types here?

preact/src/diff/index.js

Lines 358 to 359 in de08e91

'setAttribute' in child === !!nodeType &&
(nodeType ? child.localName === nodeType : child.nodeType === 3)

The other one knows what type it's looking for, so maybe it isn't possible? Just trying to think of ways to avoid checking the nodeType getter.

}

return oldDom;
Expand Down
11 changes: 10 additions & 1 deletion test/browser/hydrate.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,11 @@ describe('hydrate()', () => {
beforeEach(() => {
scratch = setupScratch();
attributesSpy = spyOnElementAttributes();
clearLog();
});

afterEach(() => {
teardown(scratch);
clearLog();
});

it('should reuse existing DOM', () => {
Expand Down Expand Up @@ -92,6 +92,7 @@ describe('hydrate()', () => {
scratch
);
expect(scratch.innerHTML).to.equal('<p><i>0</i><b>1</b></p>');
expect(getLog()).to.deep.equal(['Comment.remove()']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's strange that we end up removing the comment. I would have thought it would be skipped and ignored?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the comment will get removed in diffElementNodes here: https://github.com/preactjs/preact/blob/master/src/diff/index.js#L447-L451

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume for streaming we need to preserve these nodes until they are revived?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Ideally we would just walk over them and ignore, but I guess we need to do that in two places due to this being a new type of Node to deal with (vs skipped Text/Element nodes, which we always walk here)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative would be to special case a branch for selective hydration where we manually walk over the comment nodes as part of diff:

if (vnode.type === Island) {
  // We know that island is surrounded with comment nodes
  oldDom = oldDom.nextSibling
}

Was thinking some more about comment nodes and I'm not sure how they would behave if we'd just skip over them. Imagine a scenario where we'll support selective hydration and have a non-hydrated subtree marked with comment nodes that is only activated when the user hovers with the mouse over the area or otherwise interacts with it. In that scenario we'd likely be rendering on the client already before all subtrees are hydrated.

This could have an effect on our ordering:

<div>
  <A />
  <B />
  <NonHydrated />
  <C />
</div>

Now, if we render on the client and need to re-order nodes due to various conditional rendering, our skip comment logic would likely bias in favor of moving the comments at the end of the childNode list.

I'm wondering if that means that we'd need to move the comment node as part of NonHydrated here. That would ensure that the order is always correct.

});

it('should reuse existing DOM when given components', () => {
Expand Down Expand Up @@ -462,5 +463,13 @@ describe('hydrate()', () => {
scratch.innerHTML = '<p>hello <!-- c -->foo</p>';
hydrate(<p>hello {'foo'}</p>, scratch);
expect(scratch.innerHTML).to.equal('<p>hello foo</p>');
expect(getLog()).to.deep.equal(['Comment.remove()']);
});

it('should skip over multiple comment nodes', () => {
scratch.innerHTML = '<p>hello <!-- a --><!-- b -->foo</p>';
hydrate(<p>hello {'foo'}</p>, scratch);
expect(scratch.innerHTML).to.equal('<p>hello foo</p>');
expect(getLog()).to.deep.equal(['Comment.remove()', 'Comment.remove()']);
});
});