Mithril.js: [rewrite] Issues with the way fragments are implemented

[pygy]

The way the rewrite supports fragments is potentially problematic for v0.2 migration, since there, vnodes in nested arrays are sharing the same key space whereas they don't in the rewrite.

There's also a missed opportunity in the fact that fragments support lifecycle hooks, but those can't be defined using hyperscript. Fragments are lighter than components when all you want is to add hooks to a pre-existing vnode without altering it, and you don't end up with a wrapper element.

I'd suggest the following:

1) Restore the flattening behaviour of v0.2 for nested arrays.

2) Introduce a syntax for deliberately creating fragments:

• m([vnodes], attrs)
• m([], attrs, children)
• m(null, attrs, children)
• m('[', attrs, children)
• m('', attrs, children)? Now this one could make sense...
• _your choice here_

Alternatively, for the key space problem, the users can be asked to flatten the tree manually, I have a draft paragraph for the migration guide lying around somewhere.

[lhorie]

I removed flattening behavior because a) it's quite expensive b) it's hard to reason about key spaces that mix keyed and non-keyed vnodes (especially when they cross component boundaries), and c) there were weird bugs when trying to do b)

I don't mind 2). Bikeshed away :)

[barneycarroll]

It's important to note that the change in diffing behaviour is a benefit: by using v1 fragments I can have adjacent conditional items and dynamic lists without the need for intervening DOM nodes. Typically you'll want this behaviour when you have eg conditional admin controls in a toolbar, or a space for stateful messages in forms. Previously, you would have needed to create otherwise useless or problematic wrapping elements or explicitly key every adjacent element. This is immensely useful IMO! Therefore I move to rule out option 1.

Point 2 is valid and good though. I don't like 2.5 (empty selector) because my intuitive parsing tells me this is equivalent to an omitted div with no trailing attribute declarations. 2.4 makes excellent sense to me though, and could also be applied to text nodes (#) and trusted islands (<). I had assumed this behaviour when I first started playing around with the rewrite.

[tivac]

• m([vnodes], attrs) - Ok but awkward to read
• m([], attrs, children) - Looks like a mistake
• m(null, attrs, children) - Looks like a mistake
• m('[', attrs, children) - Looks like a mistake
• m('', attrs, children) - Looks like a mistake

I think using m() to support some random non-HTML leading argument is confusing.

If fragments are expected to be a thing that people use adding something more explicit like m.fragment(attrs, children) would help keep it from looking like an accident.

[pygy]

@barneycarroll since Leo seems against flattening, this point is moot, but since you can create fragments explicitly in v1 (not conveniently right now), the issues you were running into with v0.x would go away, even if arrays were flattened before diffing.

Also, +1 for @tivac's proposition.

[barneycarroll]

Fair point WRT looking like mistakes. The bracket looks like you started with the intention of declaring an attribute. The existing parser error makes more sense than the alternative, from this angle.

I misread m( [ vnodes], attrs ) the first time over: shouldn't those arguments be the other way round — attrs first, nodes after? m( attrs?, ...vnodes )?

Thinking it over I'm doubting the merit of the whole thing — the only attribute that makes sense in the context of a fragment is key, right (AFAIK fragments don't do lifecycle methods)? At which point, we're talking about a very niche use case whereby you want a dynamic list of lists without intervening DOM nodes (which is totally impossible in v0).

[lhorie]

AFAIK fragments don't do lifecycle methods

They do, actually

[barneycarroll]

@lhorie can't reproduce. Am I typoing this?

https://jsbin.com/jaleho/edit?js,output

The fact I'm uncertain lends credibility to @pygy's proposal IMO.

[lhorie]

@barneycarroll You forgot to wrap it in an attrs object: https://jsbin.com/pekowekefu/1/edit?js,output

[thysultan]

m(...vnodes)

if the first arg is not a tag/component create a fragment.

[lhorie]

looks like m.fragment(attrs, children) it is. Anyone want to take a stab at a PR?

[pygy]

In its own file?

[lhorie]

yeah, like m.trust

[pygy]

Ongoing, adding tests. I'll also add ./hyperscript.js that tacks on trust and fragment.

[pygy]

I have a PR for the migration guide waiting home, I'll send it this evening, along with a IE9+ compatible flatten npm module that doesn't depend on Array.isArray().

Edit: I just verified; Array.isArray() is actually defined in ES5, so the normal flatten module is suitable for the rewrite users.