Add support for fragments

[Gozala]

I don't seem to see any support for fragments similar to react's https://facebook.github.io/react/docs/create-fragment.html in virtual-dom implementation. I would really like to get support for those.

[Raynos]

Why ? Can you explain the use-case ?

I've never had the need for fragments.

[Gozala]

Why ? Can you explain the use-case ?

I think linked page explains it pretty well actually. Sometimes your want to render a list of items where some of the originate from one view function and other originate from other view function. In such scenario without fragments you either end up wrapping two lists into additional node or you end without proper key for the nodes. Extra parent node sometimes breaks semantics of the markup & affects styling. Later get's into the way of reordering and fast diffing. While fragments do exactly what one would want in those case basically it's a wrapper children that namespaces their keys and causes children to concatenated with other children.

I would also encourage to not copy React's API and instead do what they say in a note may do in a future

Note: In the future, createFragment may be replaced by an API such as

return (
  <div>
    <x:frag key="right">{this.props.rightChildren}</x:frag>,
    <x:frag key="left">{this.props.leftChildren}</x:frag>
  </div>
);

allowing you to assign keys directly in JSX without adding wrapper elements.

In fact I use this instead of stock fragment implementation from react as I never found useful to have a multiple lists of children in a same fragment.

class Fragment {
  constructor(key, children) {
    this[key] = children.map(this.importChild, this)
  }
  get _reactFragment() {
   return this
  }
}

export let fragment = (key, children) =>
  new Fragment(key, children)

[Raynos]

That's not a concrete example. That's an abstract example.

I think a wrapped div will work; it will get you maximum re-ordering performance. Supporting a fragment for keying purposes exposes a huge amount of complexity to the re-ordering implementation.

You'd have to convince @Matt-Esch

[Gozala]

I think a wrapped div will work; it will get you maximum re-ordering performance.

Problem with wrapping it in a div is that it messes with a markup and makes styling a challenge. For example sometimes you want inline style display in other block style display. Sometimes it's cells of the table where you can't even wrap rows in the div.

Supporting a fragment for keying purposes exposes a huge amount of complexity to the re-ordering implementation.

Why so ? Looking at the vnode source it seems pretty simlpe and can be a last if case so it won't affect performance of the existing code https://github.com/Matt-Esch/virtual-dom/blob/master/vnode/vnode.js#L41 All you'd need to do is something along these lines:

else if (isFragment(child)) {
  var _children = child.children;
  var prefix = child.key;
  for (var _i = 0, _count = _children.length; _i < _count; i++) {
     children.splice(i + _i, 0, withPrefix(_children[_i]));
  }
}

[Gozala]

I'm not entirely sure how does key generation works in virtual-dom but I imagine above example can be further optimised by having a key prefix table keyed with child index to avoid wrapping each fragment child.

[Gozala]

So looking further into implementation I think this should not be too hard and will require following changes: https://github.com/Matt-Esch/virtual-dom/blob/master/vnode/vnode.js#L61

else if (isFragment(child)) {
  var keyPrefixes = this.keyPrefixes || (this.keyPrefixes = {});
  var _children = child.children;
  var prefix = child.key;
  for (var _i = 0, _count = _children.length; _i < _count; i++) {
     var index = i + _i;
     keyPrefixes[i] = prefix;
     children.splice(index, 0, _children[_i]);
  }
}

And this will have to change slightly https://github.com/Matt-Esch/virtual-dom/blob/master/vtree/diff.js#L394-L413

function keyIndex(children, keyPrefixes) {
    var keys = {}
    var free = []
    var length = children.length

    for (var i = 0; i < length; i++) {
        var child = children[i]
        var prefix = keyPrefixes[i]

        if (child.key) {
            keys[child.key] = prefix ? prefix + '.' + i : i
        } else if (prefix) {
            keys[prefix + '.' + i] = i
        } else {
            free.push(i)
        }
    }

    return {
        keys: keys,     // A hash of key name to index
        free: free      // An array of unkeyed item indices
    }
}

[Raynos]

@Gozala I don't really know what the consequences are

But if i understand correctly the use case is basically rows without nested elements.

function render() {
    h('div.my-row', [
        fragment([
            h('div.my-element', { style: ... })
        ]),
        fragment([
            h('div.my-element', { style: ... })
        ]),
        fragment([
            h('div.my-element', { style: ... }),
            h('div.my-element', { style: ... })
        ]),
    ])
}

You want the re-ordering logic to move groups of elements without wrapper divs; this would allow you to implement your own grid layouts or whatever without coupling strongly to the DOM.

I still don't have a solid argument on why the wrapper approach doesn't work; hand wavey arguments about "css is hard"

[Gozala]

So this is contrived example but it does illustrates the problem:

function viewUSBands() {
   return usBands.map((name, index) => h('option', {key: index, value: name}, name)
}

function viewBandSelector() {
 h('select', {name: 'select'}, [
    fragment('us', viewUSBands()),
    fragment('uk', viewUKBands())
 ])
}

Problem is that viewUSBands and viewUKBands don't have any idea if their keys maybe in collision if combined with other children. If they would wrap result in div that would be invalid html markup as select can only have option elements as children. So currently only solution is to make viewUSBands and viewUKBands return their options wrapped in select and then grab their children and rekey them but that requires second pass. You can argue that key prefix can be passed to viewUSBands and viewUKBands so they would prefix their children but in practice it's really hard to pre-arrange everything for any use case as you either end up overgenerelising or find yorself refactoring huge chunks of the application.

fragments provide an elegant solution and works regardless if you do control subviews you're trying to mesh or not and let's you avoid overgenerelising.

[Gozala]

@Raynos dose above example is specific enough ? @Matt-Esch does this request sounds reasonable to you ?

[Zolmeister]

I'd rather see a generic way to transform v-tree's. e.g.

fragment = (key, vtrees) ->
  _.map vtrees, (tree) ->
    h tree, {key}

[Gozala]

I'd rather see a generic way to transform v-tree's. e.g.

Boxing collections is a lot cheaper than mapping collections and makes it a lot more difficult do a reuse. Also not sure you'd be even ablo to do remapping in combination with thunks.

[Gozala]

@Zolmeister to put it other way your proposed fragment will need to do as many allocations as many vtrees you pass, while in case of boxing it's just 1 alocation + boxed vtrees could be reused.

[Raynos]

@Gozala I see and understand the use case.

This cannot be implemented today without a wrapper div; it would require some tooling; we can prototype it using Widget but it would be quite heavy.

[Gozala]

Alternative woud be to add ES6 iteraors support so vtrees could be mapped lazily, although that would still require execution of mapping function eventually, while I imagine it could be avoided entirely if boxing was used instead.

[Gozala]

This cannot be implemented today without a wrapper div; it would require some tooling; we can prototype it using Widget but it would be quite heavy.

@Raynos I would not mind on doing an implementation that will be cheap and will not reqire Widget I'm pretty sure that all this can be handled at vtree diffing phase as I pointed out earlier (see https://github.com/Matt-Esch/virtual-dom/issues/251#issuecomment-104089862) but I'd like to not waste time if such changes won't be accepted.

[Raynos]

@Gozala its a question for @Matt-Esch

[neonstalwart]

@Gozala i'm interested in the concept (but ultimately it's up to @Matt-Esch). if you think there is a way to do it elegantly, cheaply, and simply then i'd be interested in seeing it but i can't say if i would support any particular changes until i see them. if i like the changes then you would have my full support.

[Gozala]

@Gozala i'm interested in the concept (but ultimately it's up to @Matt-Esch). if you think there is a way to do it elegantly, cheaply, and simply then i'd be interested in seeing it but i can't say if i would support any particular changes until i see them. if i like the changes then you would have my full support.

@neonstalwart thanks, that makes sense. I don't expect to get a full support before the code is actually there & understand that given changes at hand it may or may not make sense to take them.

@Matt-Esch could you please let me know your view on this matter ? I imagen you also could give more insight in regards to proposed implementaiton plan.

[jails]

There's a couple of virtual dom implementation which manage nested children structures and transform a children nested array [ ['1', '2'], ['3', '4'] ] into [ '1', '2', '3', '4' ] out of the box and I aggree it can be useful. However I don't think it's a big issue since you can move this logic up to a custom hyperscript function. Or using the existing one like the following:

function viewBandSelector() {
 h('select', {name: 'select'}, [].concat.apply([], [
    withPrefix('us', viewUSBands()),
    withPrefix('uk', viewUKBands())
 ]))
}

I'm not sure that the key prefixing part of the "React fragment abstraction" worth the introduction of a specific feature. Anyhow I never needed this feature personally so I don't know.

My 2 cents.

[Gozala]

As I already mentioned simple functions to unpack aren't going to cut it as they can not handle thunks!

Sure moving code between abstraction layers does job, but that is not always possible (you do not control component you're trying to embed) or desired (your component deals with collection of items & the container of that items may vary as it's styling, deciding so there is no single right container you can choose) .

Typed on tiny keyboard

On Jun 3, 2015, at 8:59 AM, Simon JAILLET notifications@github.com wrote:

There's a couple of virtual dom implementation which manage nested children structures and transform a children nested array [ ['1', '2'], ['3', '4'] ] into [ '1', '2', '3', '4' ] out of the box and I aggree it can be useful. However I don't think it's a big issue since you can move this logic up to a custom hyperscript function. Or using the existing one like the following:

function viewBandSelector() { h('select', {name: 'select'}, [].concat.apply([], [ withPrefix('us', viewUSBands()), withPrefix('uk', viewUKBands()) ])) } I'm not sure that the key prefixing part of the "React fragment abstraction" worth the introduction of of a specific feature. Anyhow I never needed this feature personally so I don't know.

My 2 cents.

— Reply to this email directly or view it on GitHub.

[jails]

Indeed won't be able to manage a fragment as a thunk (i.e taking control of the diff'ing process of your whole fragment), but is that a really requirement ?

Unless you planned to decline everything as supporting a "Fragment". For example modifying createElement() to returns either an array of DOM element or a single DOM element depending if it's called with a "Fragment" or not. And then probably the diffing/patching algorithm accordingly. I don't see what you mean.

[Gozala]

Indeed won't be able to manage a fragment as a thunk (i.e taking control of the diff'ing process of your whole fragment), but is that a really requirement ?

Most my use cases around fragments has benig Thunk (or React.Component in case of React) returning a fragment with children as that code unit is reused in a several different scenarios and there is no single wrapper that would work in all those scenarios.

I'm afraid I don't fully understand your queston. Never the less, there are valid use cases where one would like to create a unit (thunk, React.Component) that gathers items from diffenet units and do so without extra containers that may even make markup invalid.

Unless you planned to decline everything as xxxFragment like a ThunkFragment. And modify createElement() to returns either an array of DOM element or a single DOM element depending if it's called with a "xxxFragment" or not. And then rewriting the whole patch algorithm accordingly. I don't see what you mean.

I don't follow your line of thoughts here, but changes required to implement that are going to be with-in the diff-ing code rather that patch-ing.

[Gozala]

Alternative API to fragments could be something like children(tree) that would be able to support thunks. But that sounds a more complicated as internally one would still need to introduce some sort of ChildrenThunk to make it work & also provide some way to concat arrays with ChildrenThunks.

Yet another alternative would be to allow thunks to return an array of nodes and also do internal concatination. Although I'm not sure how one would be able to implement that.

[jails]

By Thunk you mean this right ? Thought React components was only able to render a single root node you mean you can use a <x:frag> as root node ?

[Gozala]

By Thunk you mean this right ?

Yes

hought React components was only able to render a single root node you mean you can use a as root node ?

Not right now but I believe it's planned to support that.

[Zolmeister]

Not right now but I believe it's planned to support that.

I'm skeptical. After implementing Zorium I can see why they chose that path (and changing it is difficult).

Edit: to be clear, I'm specifically speaking to multiple root elements for a component

[Gozala]

I'm skeptical. After implementing Zorium I can see why they chose that path (and changing it is difficult).

I on the other hand implemented my own fragments for react that implement both react element and react fragment interfaces which makes it possible to use them from the components. I can't share implementation right now, but I can do so later when I have more time for this.

[Gozala]

I am little lost & tired of all this back end forth arguments. I do beilieve I provided a reasonable use case for fragments & there is also a precedent of it's existence in other libraries like react. If you have not had need for it yet that does not mean it's useless. All the proposed solutions here are incomplete, mere specifically they can't handle thunks which and there for they do not solve all / my use cases.

At this point I would like to avoid spending more time arguing merits of this feature. I am happy to learn about any complete solution for the problem I've described, if there is one. Otherwise I'd prefer to hear from @Matt-Esch whether this feature is in or out of the scope for this library.

There are several other features that are missing in virtual-dom library that prevents use of it for the project I'm working on. I very much would prefer to contribute these missing features over rolling out yet another virtual-dom library.

[Raynos]

@Gozala You've given your use cases; we just need to get an ok from @Matt-Esch

I'll let him know.

[Matt-Esch]

@gozala I can understand the motivation for having a collection of siblings without a root node.

Supporting a root node fragment is not something we should do IMO unless you can provide such a use case for that also.

If fragments are only collections we have some questions to ask ourselves about how they really need to live in the tree.

The first most obvious solution to this problem is to just get h to understand fragments and to unpack them into the child array. If this can (or anything else) be implemented as a transform in your own personal h I strongly encourage that you do that, and I even advocate for putting this in the default h. This could be as simple as saying that arrays inside the child array are fragments, and having h unpack these child nodes in the right location in the container's child array.

There are a few caveats to this approach of course. If you want the diff algorithm itself to diff fragment blocks as if they were invisible root nodes, this amounts to a lot of rework and will behave very differently to unpacking.

It's really a question of are you trying to use fragments to bucket together nodes for the purpose of diffing, or do you just want a way to support returning multiple nodes at the same level without a root node. If the latter, a trivial change to h could make this work.

[Gozala]

@gozala I can understand the motivation for having a collection of siblings without a root node.

Great!

Supporting a root node fragment is not something we should do IMO unless you can provide such a use case for that also.

Can you please elaborate what do you mean by "root node" here ?

The first most obvious solution to this problem is to just get h to understand fragments and to unpack them into the child array. If this can (or anything else) be implemented as a transform in your own personal h I strongly encourage that you do that, and I even advocate for putting this in the default h. This could be as simple as saying that arrays inside the child array are fragments, and having h unpack these child nodes in the right location in the container's child array

I don't believe this can be implemented at the h level as in my understand forcing thunks happens at the diff level. So if there is a thunk there is no way of knowing if it's going to produce a vnode or a fragment ahead of time.

There are a few caveats to this approach of course. If you want the diff algorithm itself to diff fragment blocks as if they were invisible root nodes, this amounts to a lot of rework and will behave very differently to unpacking.

This is what I had in mind. Have you seen https://github.com/Matt-Esch/virtual-dom/issues/251#issuecomment-104089862 is there more changes required than pointed out there ?

It's really a question of are you trying to use fragments to bucket together nodes for the purpose of diffing, or do you just want a way to support returning multiple nodes at the same level without a root node. If the latter, a trivial change to h could make this work.

Main intent is to free components from foreseeing how they are going to be embedded. Here is an example say you are defining a counter, it's markup is likely going to be something along these lines: <button>+</button><span>5</span><button>-</button>

But you you can not do that as you need to wrap that markup in some node. Problem is that author of counter can't possibly what would be preferred container by it's users span / div / ... ? It also can't possibly know what kind of styling container should have. So only solution would be allow passing tagName and all the attributes along. But what if user wants to add another button to a counter like x before + or maybe after -. In other words to make truly customisable counter component author would need to take attributes options tagName preceding and following children.

With fragments author can simple define reusable component like <x:fragment><button>+</button><span>5</span><button>-</button><x:fragment> and give full flexibility to a user on the embedding part.

I think fragments should be first class and thunks should be able to return them as well. I believe for that doing it at the diff level is required.

[Matt-Esch]

Can you please elaborate what do you mean by "root node" here ?

Patching onto a fragment as the root element of the DOM (and thus virtual) tree

We are currenly solving your button example with simple functions:

function renderPage(items) {
    return h('body', items.map(containedButton));
 }

function containedButton(item) {
    return h('div', { className: item.name }, button(item));
}

function button(item) {
    return [
        h('button', ['+']),
        h('span', [item.count]),
        h('button', ['-'])
    ];
}

The thunk argument bears some weight because that really is something that cannot be done, but I am inclined to ignore it. I'd really like to deprecate thunks because dirty checking state in the tree itself is ugly and defeats the point of having observable state. I would simply use an observable array of child nodes and concat them into the child array.

The biggest argument against fragments is that all nodes in the tree map onto a node in the DOM currently, and breaking that is likely to be difficult and expensive for perf. I'm not convinced that a first class fragment in the tree overcomes any otherwise unsolvable problem or adds any performance gains.

[Matt-Esch]

The namespacing argument is a strong one. Let's try and make a first pass at this and see what it looks like. I don't want to reject the feature for being too complex if it is indeed simple. It's a major version bump. Render will have to be able to handle it. All tree walking code will need an extra check to get the indexing correct (more than what you suggest). It is not ok to mutate the child contents at any point, they are frozen.

[Matt-Esch]

Honestly I think it's going to be really complicated to add as a feature and pretty error prone.

But I imagine somer performance benefits. How about removing individual items without keys:

[A, B, C, D, E, F]

remove D

[A, B, C, {empty frag}, E, F]

Result: D gets removed with a single operation without needing keys.

[Gozala]

The thunk argument bears some weight because that really is something that cannot be done, but I am inclined to ignore it. I'd really like to deprecate thunks because dirty checking state in the tree itself is ugly and defeats the point of having observable state. I would simply use an observable array of child nodes and concat them into the child array.

Deprecating thunks is interesting subject. I'm am not sure what do you mean by "the dirty checking state in the tree" or observable arrays. I think it would be a shame to tie virtual-dom library to observable-*, but maybe I'm not following what you mean.

My primary use of thunks is related associated with computation caching, meaning if function is passed an equal inputs it should avoid recomputing a vnodes. I don't see how this can be achieved without thunks without unbound cache.

Are your thoughts about thunk alternatives described somewhere, I'd love to read that.

[Gozala]

All tree walking code will need an extra check to get the indexing correct (more than what you suggest).

That sounds right, I believe react unpacks fragments in their tree traverse function so probably working code will indeed need to be updated.

It is not ok to mutate the child contents at any point, they are frozen.

Yeah that makes sense.

[Gozala]

Honestly I think it's going to be really complicated to add as a feature and pretty error prone.

But I imagine somer performance benefits. How about removing individual items without keys:

[A, B, C, D, E, F] remove D

[A, B, C, {empty frag}, E, F] Result: D gets removed with a single operation without needing keys.

I am not following what you are suggesting here, I'm sorry. Can you give some more context please ?

[Gozala]

It is not ok to mutate the child contents at any point, they are frozen.

Were you referring to mutations of children proposed here:

So looking further into implementation I think this should not be too hard and will require following changes: https://github.com/Matt-Esch/virtual-dom/blob/master/vnode/vnode.js#L61

else if (isFragment(child)) {
  var keyPrefixes = this.keyPrefixes || (this.keyPrefixes = {});
  var _children = child.children;
  var prefix = child.key;
  for (var _i = 0, _count = _children.length; _i < _count; i++) {
     var index = i + _i;
     keyPrefixes[i] = prefix;
     children.splice(index, 0, _children[_i]);
  }
}

Anyway I think this proposal did not made much sense anyhow as it was unable to cover the thunks which was my primary goal. So all changes should probably happen in diff.