Support for components that return arrays

[infinnie]

We often have use cases where our components need to return arrays, e.g.

var SomePageComponent = function (props) {
    props = props || {};
    return [<Header />,
        <SideNav />,
        <div class="panel panel--main">...</div>
    ];
};

However, it isn’t easy for us to really do so in HyperApp. I’ve written a polyfill for exactly that (using jQuery, along with other polyfill code for backward compatibility):

function patch(parent, element, oldNode, node, isSVG, nextSibling) {
    if ($.isArray(node)) {
        return $.map(node, function (t, i) {
            return patch(parent, element, i === node.length - 1 ? oldNode : null, t, isSVG, nextSibling);
        });
    }

    if ($.isArray(element)) {
        var remaining = element.slice(1);
        element = element[0];
        $(remaining).remove();
    }

    // original code ...
}

I do not quite know whether there would still be issues with components returning arrays.

Will HyperApp officially support that?

[leeoniya]

in my experience, robust, optimal and fast dom reconciliation of fragments is non-trivial. i'd be interested to see what you guys come up with and how much code it adds.

preactjs/preact#703 (comment)

[jorgebucaran]

@infinnie @leeoniya By the way, I am sure you all already know this, but hyperapp does not aim to be compatible with React in any way, so IMO there is no pressure to introduce this feature. Or at least it is not remotely as critical as it is in preact.

@zaceno @lukejacksonn @selfup @andyrj How do you guys feel about the idea of returning arrays from components?

[leeoniya]

fragments are not about React parity; they do have actual uses in both rendering optimizations (sCU the wrapper separately from the children) and flexbox cases as mentioned in the original React issue [1]. domvm also has a user that had a use-case of expanding a table row to one or multiple adjacent rows without having to diff/reconcile all siblings that could number in the thousands [2].

[1] facebook/react#2127 (comment)
[2] domvm/domvm#124

[jorgebucaran]

@leeoniya All I am saying is that if you have a component in React written this way, it will not work on Preact unless they support this new feature of React 16. That problem does not exist in Hyperapp.

EDIT: I am not snubbing the idea, but I'm not sure I would ever need this.

[andyrj]

Just to leave this as food for thought, it looks to me like we could do this with even smaller change to patch if we instead use a Fragment HOC like this and then special case in patch.

if (node.tag === "undefined") { /* just patch children without changing parent and element... */ }

edit: or maybe the document.createElement call is what's making that undefined into a string, in which case we could

if (node.tag == null && node.children != null) { /* just patch children without changing parent and element... */ }

That would avoid allowing array as a return at least... I don't see any reason someone would actually want a dom node named undefined, lol but maybe I'm wrong.

If this is only about avoiding the wrapper div's and not about the array of nodes, then we could clean that up and remove the props.list part from my codepen above...

[leeoniya]

That problem does not exist in Hyperapp.

fragments solve a vdom problem that's common to many vdom libs, React 15 included. sure, we dont have the problem of keeping up with React if we choose to live with the problems that v16 legitimately addresses :D

[jorgebucaran]

@leeoniya Read out of context, one would think that this is a critical feature.

[leeoniya]

no no :)

[zaceno]

🤔 I know that I have wanted this from time to time, but I have always found acceptable workarounds.

I'm not sure I follow what the jQuery based polyfill is doing, but I'm guessing that it is removing the unwanted enclosing element somehow (replacing it with its children). I think that might be more easily doable with lifecycle events:

(untested braindump:)

const DisappearingWrapper = (props, children) => (
  <div
    oncreate={ el => {
      while (el.childNodes.length) {
        el.parentNode.insertBefore(el.childNodes[0], el)
      }
      el.parentNode.removeChild(el)
    }}
  >{children}</div>
)

Using it:

<main>
  <DissapearingWrapper>
    <A />
    <B />
  </DissapearingWrapper>
  <DissapearingWrapper>
    <C />
    <D />
  </DissapearingWrapper>
</main>

after DOM is done rendering, it should look like

<main>
  <A />
  <B />
  <C />
  <D />
</main>

[zaceno]

@andyrj I wouldn't mind adding it to core if it's not too complicated. But I imagine it won't work well with keyed nodes. We could just skip the keys-algo in those cases but I wonder if that'd be acceptable (and how to document that).

(my example doesn't mention keys, but it works if we add the key from props to the enclosing div)

[infinnie]

// Today’s code:
        function patch(parent, element, oldNode, node, isSVG, nextSibling) {
            /// <param name="element" type="Array"/>
            if ($.isArray(node)) {
                if ($.isArray(element) && $.isArray(oldNode)) { // what’s new
                    var elementCopy = element.slice(0), oldCopy = oldNode.slice(0);
                    var ret = $.map(node, function (t, i) {
                        return patch(parent, elementCopy.shift(), oldCopy.shift(), t, isSVG, nextSibling);
                    });
                    $(elementCopy).remove();
                    return ret;
                }
                return $.map(node, function (t, i) {
                    return patch(parent, i === 0 ? element : null, i === 0 ? oldNode : null, t, isSVG, nextSibling);
                });
            }

            if ($.isArray(element)) {
                var remaining = element.slice(1);
                element = element[0];
                $(remaining).remove();
            }
            // ...
        }

[andyrj]

@zaceno ya my little hack thus far doesn't work with keyed nodes for the Fragment list, but I think most places someone wants to use keyed nodes they are children, so imo it would be ok to just do a simple non-keyed diff on the Fragment children presuming that the user will understand that they aren't keyed.

But I might be wrong there, I would find this useful to just avoid the wrapper div on my app, other than that I don't think I would use this really, so I might be overlooking a different use-case. I tried to work that Fragment HOC into the keyed diff but that blew up pretty spectacularly on the attempts I had thus far...

[infinnie]

@zaceno basically I mean, if node is an array, patch the array items one by one. If element is an array but node isn’t, remove the elements after the first one and only patch the first element.

[zaceno]

@infinnie Ah ok, I think I understand now.

But wouldn't you need to get into the child loop and handle the case of a single child being an array? Or maybe your use case is only for the top level? (like @andyrj )

I was assuming this was more about general components being able to return arrays of vtrees, but I may be mistaken.

[andyrj]

here is link to my hack branch that adds special case for the Fragment HOC I linked to in the codepen above... should work for any non-keyed list of nodes in the Fragment props.list or children.

so instead of just returning an array,

return <Fragment list={[<h1>Title</h1>, <div>stuff</div>]} />
//, or 
return <Fragment><h1>Title</h1><div>stuff</div></Fragment>

https://github.com/andyrj/hyperapp/tree/no-more-wrappers

need to finish test case for when Fragment updates and removes nodes from parent but it should be working at this point minus those two lines test coverage...

@zaceno this little hack would work for more than just the root case but can't handle keyed nodes still sadly 😞

Edit: also just want to be clear, not trying to push anything or hi-jack the issue, I just think keeping the return of h() the same shape will keep interpreter optimizations "happier"... not that I have benchmarks for comparison but that seems to be general rule of thumb for params and I assume there are also optimizations on return value being the same shape.

[infinnie]

@zaceno I guess that it is recursive.

[zaceno]

@infinnie Ok yeah I see ... that could probably work. It would get weird with keys though, but that may not be problem if the use cases don't overlap.

I'd suggest making a PR, but I also understand you want to get a feel for how it would be received before putting in the work.

(If/when you do make a PR: don't use jQuery or polyfills -- one of Hyperapp's key benefits is "no dependencies")

[infinnie]

@zaceno also, more tests would be needed for correctness sake.

[infinnie]

Maybe we could actually make it always an array (e.g. in h())?

[jorgebucaran]

It would be amazing if we were able to implement this as a HOF of h(). I wonder if that's possible.

Would you like to look into it? @infinnie

[infinnie]

That would be worth investigating.

[infinnie]

Seems that currently only top-level components are prohibited from returning arrays. Otherwise the h() function has managed to make it possible to do so.

[jorgebucaran]

@infinnie How's that? Can you show me an example? 🤔

[infinnie]

@jbucaran

// h.js
if (Array.isArray((node = stack.pop()))) {
    for (i = node.length; i--;) {
        stack.push(node[i])
    }
} // ...

[jorgebucaran]

@infinnie Hmm, I meant, an example of code that works as you originally intended it to work, but I think I understand what you mean now.

[jorgebucaran]

Forgot to close here. See #366 for my rationale.