preact icon indicating copy to clipboard operation
preact copied to clipboard

[DRAFT] Handle subtree replaced via replaceNode

Open developit opened this issue 5 years ago • 2 comments

I really don't like the solution I came up with here. This is a case where excessDomChildren should end up with [<a></a>] in it which should remove the orphaned root element. I tried for quite a while to figure out why this was not the case, and installed a bunch of logging into these tests to figure it out. In the end I could get the tests passing by tweaking excessDomChildren to flush for known Fragment roots rendered using replaceNode, but doing so broke all the Portals tests.

developit avatar Dec 27 '19 02:12 developit

Coverage Status

Coverage increased (+0.08%) to 97.754% when pulling 33545dd4610b01a30f837c42f8d606e97f49e846 on bugfix-replacenode-differing-root into cf635d5eb274c2fba4fc23c34ef9157b90d17e81 on master.

coveralls avatar Dec 27 '19 02:12 coveralls

Just a note from me: the reason I don't think we should merge this as-is comes down to it being a patch. This directly address the issue from #2004 by removing an unmatched root after rendering, but doing so fails to unmount that root's VDOM tree. The reason it cannot currently unmount the previous tree is that we don't have a reference to it: render() was called on an element somewhere within a VDOM tree, but we can't go from Node to VNode without crawling all the way back up to the root looking for the tree, then performing a BFS on the VDOM tree to find the outermost VNode with a _dom reference to the element we're updating. Personally I think that's far too much complexity for this since it's not technically a supported use-case.

What I would like to have as a long-term solution for this case would be to drop the idea of unmounting that VDOM - invoking render() on a Node without giving Preact VDOM tree information logically shouldn't try to infer that information. Instead, we can acknowledge that we'll remove unmatched Elements but not unmount unrelated VDOM trees. If we can commit to that, fixing the issue here becomes more compelling: I would like to see if there is a way for diffChildren or diffElementNodes to identify when they are handling a mismatched root with no excessDomChildren available to search through, and in those cases use .replaceNode() where they would otherwise have used .insertBefore().

developit avatar Jan 10 '20 17:01 developit