Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

Preact: Hydration fails when there are HTML comments #79

Closed
luisherranz opened this issue Oct 13, 2022 · 5 comments · Fixed by #84
Closed

Preact: Hydration fails when there are HTML comments #79

luisherranz opened this issue Oct 13, 2022 · 5 comments · Fixed by #84

Comments

@luisherranz
Copy link
Member

luisherranz commented Oct 13, 2022

We found what I think is a bug in Preact's hydration. By default, if Preact finds HTML comments during the hydration, it removes them. The problem is that while doing so, it also recreates (removes and adds again) the next DOM node.

I've reproduced the issue in this repository: https://github.com/luisherranz/preact-comments-hydration-bug

I've also checked it in Preact's own tests, and it's also happening there: luisherranz/preact@004d25b

I've recorded a video to show the repository and tests:

https://www.loom.com/share/89dbe2ac14164bb9a35457cd27e10cce

@luisherranz
Copy link
Member Author

I've opened an issue in Preact's repo:

cc: @DAreRodz @michalczaplinski

@luisherranz
Copy link
Member Author

Ok, so this doesn't seem to be a bug in Preact, but the expected behavior.

I would try the following implementation here:

  • While doing the DOM -> vDOM (toVdom), remove the comments from the DOM, but keep the string and a reference to its location (parent or previous sibling).
  • Right after the hydrate, add them back.

@luisherranz
Copy link
Member Author

luisherranz commented Oct 17, 2022

This seems to work fine. It should be just as fast, if not faster:

- const children = [].map.call(node.childNodes, toVdom).filter(exists);
+ const children = [];
+ for (let i = 0; i < childNodes.length; i++) {
+   const child = childNodes[i];
+   if (child.nodeType === 8) {
+     child.remove();
+     i--;
+   } else {
+     children.push(toVdom(child));
+   }
+ }

Kudos to @DAreRodz for noticing that we need to modify the index when removing child nodes.

@luisherranz
Copy link
Member Author

Let's not add the comments back just yet to avoid introducing complexity prematurely and see what happens when we test this in real sites.

@luisherranz
Copy link
Member Author

Already solved in #84.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant