Allow variadic arguments to m.fragment (#2328)

* Implement support for variadic arguments to `m.fragment`

While I was at it, I refactored the common logic out of `hyperscript`.

* Add a missed change from #2326

* Update docs + changelog [skip ci]

* Explain rationale for `hyperscriptVnode`'s calling convention

This way, it doesn't get erroneously "cleaned up" into something worse,
and so it's clearer how it'd be potentially optimized once ES5 support
is dropped.

.gitattributes

@@ -1,5 +1,7 @@
 * text=auto
 /mithril.js binary
 /mithril.min.js binary
+/mithril.mjs binary
+/mithril.min.mjs binary
 /package-lock.json binary
 /yarn.lock binary

docs/change-log.md

@@ -54,6 +54,7 @@
     - All of the `m.*` properties from `mithril` are re-exported as named exports in addition to being attached to `m`.
     - `m()` itself from `mithril` is exported as the default export.
     - `mithril/stream`'s primary export is exported as the default export.
+- fragments: allow same attrs/children overloading logic as hyperscript ([#2328](https://github.com/MithrilJS/mithril.js/pull/2328))
 
 #### Bug fixes
 

docs/fragment.md

@@ -37,9 +37,9 @@ Generates a fragment [vnode](vnodes.md)
 
 Argument    | Type                                                | Required | Description
 ----------- | --------------------------------------------------- | -------- | ---
-`attrs`     | `Object`                                            | Yes      | A map of attributes
+`attrs`     | `Object`                                            | No       | HTML attributes or element properties
-`children`  | `Array<Vnode|String|Number|Boolean|null|undefined>` | Yes      | A list of vnodes
+`children`  | `Array<Vnode>|String|Number|Boolean`                | No       | Child [vnodes](vnodes.md#structure). Can be written as [splat arguments](signatures.md#splats)
-**returns** | `Vnode`                                             |          | A fragment [vnode](vnodes.md)
+**returns** | `Vnode`                                             |          | A fragment [vnode](vnodes.md#structure)
 
 [How to read signatures](signatures.md)
 
@@ -49,14 +49,14 @@ Argument    | Type                                                | Required | D
 
 `m.fragment()` creates a [fragment vnode](vnodes.md) with attributes. It is meant for advanced use cases involving [keys](keys.md) or [lifecyle methods](lifecycle-methods.md).
 
-A fragment vnode represents a list of DOM elements. If you want a regular element vnode that represents only one DOM element, you should use [`m()`](hyperscript.md) instead.
+A fragment vnode represents a list of DOM elements. If you want a regular element vnode that represents only one DOM element and don't require keyed logic, you should use [`m()`](hyperscript.md) instead.
 
-Normally you can use simple arrays instead to denote a list of nodes:
+Normally you can use simple arrays or splats instead to denote a list of nodes:
 
 ```javascript
 var groupVisible = true
 
-m("ul", [
+m("ul",
 	m("li", "child 1"),
 	m("li", "child 2"),
 	groupVisible ? [
@@ -64,7 +64,7 @@ m("ul", [
 		m("li", "child 3"),
 		m("li", "child 4"),
 	] : null
-])
+)
 ```
 
 However, Javascript arrays cannot be keyed or hold lifecycle methods. One option would be to create a wrapper element to host the key or lifecycle method, but sometimes it is not desirable to have an extra element (for example in complex table structures). In those cases, a fragment vnode can be used instead.

docs/hyperscript.md

@@ -40,12 +40,12 @@ You can also [use HTML syntax](https://babeljs.io/repl/#?code=%2F**%20%40jsx%20m
 
 ### Signature
 
-`vnode = m(selector, attributes, children)`
+`vnode = m(selector, attrs, children)`
 
 Argument     | Type                                       | Required | Description
 ------------ | ------------------------------------------ | -------- | ---
 `selector`   | `String|Object`                            | Yes      | A CSS selector or a [component](components.md)
-`attributes` | `Object`                                   | No       | HTML attributes or element properties
+`attrs`      | `Object`                                   | No       | HTML attributes or element properties
 `children`   | `Array<Vnode>|String|Number|Boolean`       | No       | Child [vnodes](vnodes.md#structure). Can be written as [splat arguments](signatures.md#splats)
 **returns**  | `Vnode`                                    |          | A [vnode](vnodes.md#structure)
 
@@ -55,7 +55,7 @@ Argument     | Type                                       | Required | Descripti
 
 ### How it works
 
-Mithril provides a hyperscript function `m()`, which allows expressing any HTML structure using javascript syntax. It accepts a `selector` string (required), an `attributes` object (optional) and a `children` array (optional).
+Mithril provides a hyperscript function `m()`, which allows expressing any HTML structure using javascript syntax. It accepts a `selector` string (required), an `attrs` object (optional) and a `children` array (optional).
 
 ```javascript
 m("div", {id: "box"}, "hello")

render/fragment.js

@@ -1,7 +1,12 @@
 "use strict"
 
 var Vnode = require("../render/vnode")
+var hyperscriptVnode = require("./hyperscriptVnode")
 
-module.exports = function(attrs, children) {
+module.exports = function() {
-	return Vnode("[", attrs.key, attrs, Vnode.normalizeChildren(children), undefined, undefined)
+	var vnode = hyperscriptVnode.apply(0, arguments)
+
+	vnode.tag = "["
+	vnode.children = Vnode.normalizeChildren(vnode.children)
+	return vnode
 }

render/hyperscript.js

@@ -1,6 +1,7 @@
 "use strict"
 
 var Vnode = require("../render/vnode")
+var hyperscriptVnode = require("./hyperscriptVnode")
 
 var selectorParser = /(?:(^|#|\.)([^#\.\[\]]+))|(\[(.+?)(?:\s*=\s*("|'|)((?:\\["'\]]|.)*?)\5)?\])/g
 var selectorCache = {}
@@ -29,18 +30,21 @@ function compileSelector(selector) {
 	return selectorCache[selector] = {tag: tag, attrs: attrs}
 }
 
-function execSelector(state, attrs, children) {
+function execSelector(state, vnode) {
-	var hasAttrs = false, childList, text
+	var attrs = vnode.attrs
-	var classAttr = hasOwn.call(attrs, "class") ? "class" : "className"
+	var children = Vnode.normalizeChildren(vnode.children)
-	var className = attrs[classAttr]
+	var hasClass = hasOwn.call(attrs, "class")
+	var className = hasClass ? attrs.class : attrs.className
+
+	vnode.tag = state.tag
+	vnode.attrs = null
+	vnode.children = undefined
 
 	if (!isEmpty(state.attrs) && !isEmpty(attrs)) {
 		var newAttrs = {}
 
 		for (var key in attrs) {
-			if (hasOwn.call(attrs, key)) {
+			if (hasOwn.call(attrs, key)) newAttrs[key] = attrs[key]
-				newAttrs[key] = attrs[key]
-			}
 		}
 
 		attrs = newAttrs
@@ -60,22 +64,22 @@ function execSelector(state, attrs, children) {
 				? state.attrs.className
 				: null
 
-	if (classAttr === "class") attrs.class = null
+	if (hasClass) attrs.class = null
 
 	for (var key in attrs) {
 		if (hasOwn.call(attrs, key) && key !== "key") {
-			hasAttrs = true
+			vnode.attrs = attrs
 			break
 		}
 	}
 
 	if (Array.isArray(children) && children.length === 1 && children[0] != null && children[0].tag === "#") {
-		text = children[0].children
+		vnode.text = children[0].children
 	} else {
-		childList = children
+		vnode.children = children
 	}
 
-	return Vnode(state.tag, attrs.key, hasAttrs ? attrs : null, childList, text)
+	return vnode
 }
 
 function hyperscript(selector) {
@@ -83,27 +87,13 @@ function hyperscript(selector) {
 		throw Error("The selector must be either a string or a component.");
 	}
 
-	var attrs = arguments[1], start = 2, children
+	var vnode = hyperscriptVnode.apply(1, arguments)
-
-	if (attrs == null) {
-		attrs = {}
-	} else if (typeof attrs !== "object" || attrs.tag != null || Array.isArray(attrs)) {
-		attrs = {}
-		start = 1
-	}
-
-	if (arguments.length === start + 1) {
-		children = arguments[start]
-		if (!Array.isArray(children)) children = [children]
-	} else {
-		children = []
-		while (start < arguments.length) children.push(arguments[start++])
-	}
 
 	if (typeof selector === "string") {
-		return execSelector(selectorCache[selector] || compileSelector(selector), attrs, Vnode.normalizeChildren(children))
+		return execSelector(selectorCache[selector] || compileSelector(selector), vnode)
 	} else {
-		return Vnode(selector, attrs.key, attrs, children)
+		vnode.tag = selector
+		return vnode
 	}
 }
 

render/hyperscriptVnode.js

@@ -0,0 +1,53 @@
+"use strict"
+
+var Vnode = require("../render/vnode")
+
+// Call via `hyperscriptVnode.apply(startOffset, arguments)`
+//
+// The reason I do it this way, forwarding the arguments and passing the start
+// offset in `this`, is so I don't have to create a temporary array in a
+// performance-critical path.
+//
+// In native ES6, I'd instead add a final `...args` parameter to the
+// `hyperscript` and `fragment` factories and define this as
+// `hyperscriptVnode(...args)`, since modern engines do optimize that away. But
+// ES5 (what Mithril requires thanks to IE support) doesn't give me that luxury,
+// and engines aren't nearly intelligent enough to do either of these:
+//
+// 1. Elide the allocation for `[].slice.call(arguments, 1)` when it's passed to
+//    another function only to be indexed.
+// 2. Elide an `arguments` allocation when it's passed to any function other
+//    than `Function.prototype.apply` or `Reflect.apply`.
+//
+// In ES6, it'd probably look closer to this (I'd need to profile it, though):
+// module.exports = function(attrs, ...children) {
+//     if (attrs == null || typeof attrs === "object" && attrs.tag == null && !Array.isArray(attrs)) {
+//         if (children.length === 1 && Array.isArray(children[0])) children = children[0]
+//     } else {
+//         children = children.length === 0 && Array.isArray(attrs) ? attrs : [attrs, ...children]
+//         attrs = undefined
+//     }
+//
+//     if (attrs == null) attrs = {}
+//     return Vnode("", attrs.key, attrs, children)
+// }
+module.exports = function() {
+	var attrs = arguments[this], start = this + 1, children
+
+	if (attrs == null) {
+		attrs = {}
+	} else if (typeof attrs !== "object" || attrs.tag != null || Array.isArray(attrs)) {
+		attrs = {}
+		start = this
+	}
+
+	if (arguments.length === start + 1) {
+		children = arguments[start]
+		if (!Array.isArray(children)) children = [children]
+	} else {
+		children = []
+		while (start < arguments.length) children.push(arguments[start++])
+	}
+
+	return Vnode("", attrs.key, attrs, children)
+}

render/tests/test-fragment.js

@@ -32,4 +32,166 @@ o.spec("fragment", function() {
 
 		o(frag.key).equals(7)
 	})
+	o.spec("children with no attrs", function() {
+		o("handles string single child", function() {
+			var vnode = fragment(["a"])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals("a")
+		})
+		o("handles falsy string single child", function() {
+			var vnode = fragment([""])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals("")
+		})
+		o("handles number single child", function() {
+			var vnode = fragment([1])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals(1)
+		})
+		o("handles falsy number single child", function() {
+			var vnode = fragment([0])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals(0)
+		})
+		o("handles boolean single child", function() {
+			var vnode = fragment([true])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals(true)
+		})
+		o("handles falsy boolean single child", function() {
+			var vnode = fragment([false])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals("")
+		})
+		o("handles null single child", function() {
+			var vnode = fragment([null])
+
+			o(vnode.children[0]).equals(null)
+		})
+		o("handles undefined single child", function() {
+			var vnode = fragment([undefined])
+
+			o(vnode.children[0]).equals(undefined)
+		})
+		o("handles multiple string children", function() {
+			var vnode = fragment(["", "a"])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals("")
+			o(vnode.children[1].tag).equals("#")
+			o(vnode.children[1].children).equals("a")
+		})
+		o("handles multiple number children", function() {
+			var vnode = fragment([0, 1])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals(0)
+			o(vnode.children[1].tag).equals("#")
+			o(vnode.children[1].children).equals(1)
+		})
+		o("handles multiple boolean children", function() {
+			var vnode = fragment([false, true])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals("")
+			o(vnode.children[1].tag).equals("#")
+			o(vnode.children[1].children).equals(true)
+		})
+		o("handles multiple null/undefined child", function() {
+			var vnode = fragment([null, undefined])
+
+			o(vnode.children[0]).equals(null)
+			o(vnode.children[1]).equals(undefined)
+		})
+		o("handles falsy number single child without attrs", function() {
+			var vnode = fragment(0)
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals(0)
+		})
+	})
+	o.spec("children with attrs", function() {
+		o("handles string single child", function() {
+			var vnode = fragment({}, ["a"])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals("a")
+		})
+		o("handles falsy string single child", function() {
+			var vnode = fragment({}, [""])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals("")
+		})
+		o("handles number single child", function() {
+			var vnode = fragment({}, [1])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals(1)
+		})
+		o("handles falsy number single child", function() {
+			var vnode = fragment({}, [0])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals(0)
+		})
+		o("handles boolean single child", function() {
+			var vnode = fragment({}, [true])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals(true)
+		})
+		o("handles falsy boolean single child", function() {
+			var vnode = fragment({}, [false])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals("")
+		})
+		o("handles null single child", function() {
+			var vnode = fragment({}, [null])
+
+			o(vnode.children[0]).equals(null)
+		})
+		o("handles undefined single child", function() {
+			var vnode = fragment({}, [undefined])
+
+			o(vnode.children[0]).equals(undefined)
+		})
+		o("handles multiple string children", function() {
+			var vnode = fragment({}, ["", "a"])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals("")
+			o(vnode.children[1].tag).equals("#")
+			o(vnode.children[1].children).equals("a")
+		})
+		o("handles multiple number children", function() {
+			var vnode = fragment({}, [0, 1])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals(0)
+			o(vnode.children[1].tag).equals("#")
+			o(vnode.children[1].children).equals(1)
+		})
+		o("handles multiple boolean children", function() {
+			var vnode = fragment({}, [false, true])
+
+			o(vnode.children[0].tag).equals("#")
+			o(vnode.children[0].children).equals("")
+			o(vnode.children[1].tag).equals("#")
+			o(vnode.children[1].children).equals(true)
+		})
+		o("handles multiple null/undefined child", function() {
+			var vnode = fragment({}, [null, undefined])
+
+			o(vnode.children[0]).equals(null)
+			o(vnode.children[1]).equals(undefined)
+		})
+	})
 })