Preact: Difference in DOM diffing compared to React

[chinchang]

I noticed this different behaviour in reconciliation between Preact and React. Here are the 2 demos:

Instructions: Click on the button to see the difference.
Preact: https://codesandbox.io/s/61nk3p4z1r
React: https://codesandbox.io/s/mymwzvnx28

Split component add a resizable gutter through out-of-react dom changes. In React, the dom changes stay after state changes (on pressing the button). Whereas in Preact, they vanish.
I believe Preact's behavior is correct, but not sure :|
Thoughts?

[marvinhagemeister]

That's interesting! I had a look at both sandboxes and it seems like the gutter-div will be removed upon a state change. Stepping through the source code of react-split and the underlying split.js library it looks like the gutter-div itself is not part of the virtual dom and is instead inserted at a later point in time by split.js.

https://github.com/nathancahill/react-split/blob/f55c3c2fa48bdf819e6766206794607955c23a42/src/index.js#L19

When an update is triggered via a state change, preact sees a div that is not part of the virtual dom and removes it accordingly. This is in my opinion the correct behaviour. React seems to have some heuristics in place to check if they rendered the dom node themselves or if it was inserted by some third-party code that is not part of the virtual-dom tree.

Looking at the code of split.js it seems like it mainly tries to work around issues in older browsers like IE8+. The easiest solution would be to track the panel sizes yourself and add custom event listeners. That would be a lot smaller and likely faster than split.js.

[chinchang]

Agree. I have been trying to replicate this library in Preact.
Though, do you think we would have similar heuristics in preact also in future (or X)?

[marvinhagemeister]

It's unlikely that we will add something like this in the foreseeable future as it can lead to false positives and would lead to a significant increase in size. There was a thread not to long ago on the react issue tracker about google translate interfering with the vdom model, causing the same problem as in these sandboxes.

[chinchang]

Makes sense. Thanks for replying @marvinhagemeister :)

[leeoniya]

@marvinhagemeister there actually are legit cases for handling untracked dom, the main one being password manager browser extensions. they sometimes inject dom and the only way to solve this without the lib handling it transparently is to tell the user not to use their favorite password manager, which can be rather awkward.

i ran into this a while back: https://github.com/domvm/domvm/issues/128

[marvinhagemeister]

@leeoniya Ohh, didn't know that! Thanks for chiming in 👍 I'm curious about which approach you chose for domvm? Do you compare the dom against a ref inside your vnodes for all children before manipulating them?

[leeoniya]

Do you compare the dom against a ref inside your vnodes for all children before manipulating them?

yeah, basically. domvm first does the diff on the old/new vdoms but still walks the real dom during the final node reordering/insertion/removal pass. since all domvm-created elements have a ._node property, ones that don't get skipped over.

the future plan is to avoid walking the dom at all, so this would hopefully be unnecessary as well as making fragments implementable without hacks.

[developit]

The diff is supposed to skip elements not created by preact, but it looks like changes in 8.0 added a way for that to be bypassed. Specifically, originalChildren points to the complete list of Element childNodes without any filtering to include only nodes created by preact. During child reordering traversal, we access this list based on index without passing over externally created nodes. That results in the "gutter" div in this split.js library being removed here:

https://github.com/developit/preact/blob/377e31b5c6d42c4ca92085571d5d4f0c9dbe4ba2/src/vdom/diff.js#L245

The issue is, innerDiffNode can't use a filtered Array of Nodes in place of its current .childNodes reference, because we rely on the semantics of it being a Live NodeList. For now, that means something like this will be required:

function innerDiffNode( .. ) {
    let offset = 0;  // stores childNodes offset due to skipping

    // replacing L245
    f = originalChildren[i + offset];
    while (f != null && !isHydrating && f[ATTR_KEY] == null) {
        offset++;
        f = originalChildren[i + offset];
    }
    // now we know `f` isn't an externally-created node

[developit]

@leeoniya good to know you're heading the direction of skipping walk entirely - Preact is about to be doing the same.

[leeoniya]

good to know you're heading the direction of skipping walk entirely - Preact is about to be doing the same.

i'm not sure if this will actually be possible without a perf hit. the dom must be mutated either way, and the current reconciler uses the live dom to make iterative decisions about what to do next (there is no "grand plan" constructed in advance for ideal mutation sequences). if only the vtree is used, then the old vtree must be kept in sync along with any actual dom mutations. this will surely carry with it a bunch of overhead that doesnt exist today. because of this, i'm not sure this is the correct path forward as i don't consider lack of fragments to be that huge an inconvenience (you guys kinda have to keep up with React, though).

i dont want to take this issue further off-topic, but would be interested to know what your experience has been with moving in that direction, especially with fragments coming in v9.

[marvinhagemeister]

@leeoniya I talked a bit about the upcoming version of preact last week: https://youtu.be/hl2bAuoiX8k?t=3543 . We still need to make a few changes to fix this issue, but we're in a much better position with Preact X than with current preact.

[marvinhagemeister]

Closing, I can't reproduce the original issue anymore with the code in master :tada: