Issue with conditional rendering of fragments

[bluenote10]

It looks like there is an issue with conditional rendering of fragments. The following example produces a correct DOM on the first rendering of the fragment, but rendering it a second time modifies the DOM in an unexpected way:

const App = () => {
  const [state, setState] = createState({
    switch: true
  });
  let a = <div>a</div>;
  let b = (
    <>
      <div>b</div>
    </>
  );
  return (
    <div>
      <button onclick={(event) => setState({switch: !state.switch})}>click</button>
      {( state.switch ? a : b )}
    </div>
  );
};

[ryansolid]

Hmm.. I didn't think about this case. As I think you can tell by now, I tend to use control flows inline and don't do any element hoisting. Although this is a very interesting differentiator that Virtual DOM can't easily leverage.

I imagine this is because you are stealing the children from the document fragment when you attach the first time. element.appendChild(fragment) moves the children to the live child nodelist of the element.

When you go to insert it the 2nd time the children are no longer there. It's basically the same as if you wrote:

let b = (
    <div>
      <div>b</div>
    </div>
  );
  return (
    <div>
      <button onclick={(event) => setState({switch: !state.switch})}>click</button>
      {( state.switch ? a : b.firstChild )}
    </div>
  );

After the action b would have no children anymore. I'm not sure if there is a great solution to this one. Cloning always seems incredibly heavy. Passing an array, an inert list of the nodes is ok but less efficient from a template rendering standpoint.

Again others have hit these sort of issues, see whatwg/dom#736. Any thoughts?

[bluenote10]

Maybe a solution to this problem should also consider another issue I observed related to document fragments, see #38.

[ryansolid]

Yeah you are right these are related. It comes down to whether the fragment should be an array. I would very much prefer not to from a performance standpoint currently. I considered that and I do support array insertion, it's just slightly less efficient on initial render. But in a situation where you are reusing nodes like that you only hit the initial render once so I'd just use an array in your example instead of a fragment.

const App = () => {
  const [state, setState] = createState({
    switch: true
  });
  let a = <div>a</div>;
  let b = [<div>b</div>];
  return (
    <div>
      <button onclick={(event) => setState({switch: !state.switch})}>click</button>
      {( state.switch ? a : b )}
    </div>
  );
};

It's either that or if the template is really large and you want to benefit from the initial cloning call Array.prototype.slice or spread over on it like:

let b = [...(<>
  <div>b</>
</>).childNodes]

The reason I'm so focused on Node cloning is for libraries like this it is the initial rendering is the cost because of setting up the reactive graph, especially on lists. Updates is where this approach excels requiring a bit more consideration there is something of a reasonable tradeoff in my mind.

[bluenote10]

My feeling is that this could become a source of hard to find bugs in more complex scenarios. Specifically I'm thinking about generic components, which have no control over constructing a or b. Think of a generic window / tab handler component that is just given a bunch of subcomponents to manage.

I tried to come up with a generic solution

{( state.switch ?
  (a instanceof DocumentFragment ? [...(a as any as DocumentFragment).childNodes] as any : a) :
  (b instanceof DocumentFragment ? [...(b as any as DocumentFragment).childNodes] as any : b)
)}

but it still shows unexpected behavior after a while (a => b => a => empty => a => empty). I'm wondering if its possible to have a helper function that allows to safely embed subcomponents?

Update: Ah, looks like I have to apply the wrapping to the references in the outer scope like this:

  let aModified = (a instanceof DocumentFragment ? [...(a as any as DocumentFragment).childNodes] as any : a);
  let bModified = (b instanceof DocumentFragment ? [...(b as any as DocumentFragment).childNodes] as any : b);
  return (
    <div>
      <button onclick={(event) => setState({switch: !state.switch})}>click</button>
      {( state.switch ? aModified : bModified)}
    </div>
  );

So providing a helper function is only a partial solution I guess.

I encountered this issue in a case that was still trivial, and yet it took quite some time to actually find the bug. The problem is that

• everything seems to work when using non-fragment subcomponents (I tested the tab viewer only with normal components and concluded it works fine),
• even with fragments everything seems to work on initial rendering, and the issue only becomes apparent after going through cycles of DOM manipulations.
  When facing such an issue in a complex app, I think it could cause quite some headache.

Solid's performance is absolutely outstanding, which is why I personally wouldn't mind trading off a little performance for safety/usability. As far as I can see, Krause's framework benchmark doesn't capture the performance benefit of caching entire components anyway. When it comes to switching complex views, the difference to VDOM libraries is likely some orders of magnitude, which in practice is much more relevant than a few percent difference on initial rendering.

[ryansolid]

Yeah I agree, I don't like that. The helper isn't a bad option. When using React to do this in the past it was all render props. But that function call reconstructs the nodes every time you switch which we are trying to avoid.

There is one other consideration which Document Fragments handle well arrays do not. Dynamic children. Like a top level conditional or each control flow. Having a parent with DOM APIs makes this work consistently even when detached.

That being said I could fake it. The tricky part is trying to decide how this should behave. Detached is straightforward. But attached should I always be inserting and removing nodes in both places (this fragment, and actual DOM parents)? That's the thing. Regardless of the approach this container never becomes part of the final DOM. So it takes extra book keeping to keep it updated at all times if we want to maintain a reference. Unfortunately the DOM provides no simple primitives to my knowledge that handle the full case.

[ryansolid]

Not sure when I will be working on this but wanted to add some notes for when I pick this up. So trying to enumerate the options here:

• Create a helper like nodeRef(jsx) that converts fragments to arrays.
• Create a new parent container that mimics DOM behavior like: https://github.com/WebReflection/document-persistent-fragment
• Build caching into the when/switch control flow
• Use boolean form of suspend control flow

The helper can't handle dynamic top level updates. The faux DOM container probably has too much overhead. I like the idea of caching on when or switch but unsure of the implementation complexity of disposal. Technically suspend control flow works but I was thinking of deprecating that form.

Suspend moves nodes between main document and a hidden document, so if you set up a series of suspends you could achieve what you are after. It's a bit bloated.

[ryansolid]

Resolved in v0.9.0 this by better handling of arrays.