Hyperscript: fix #1773, fix #2172

docs/change-log.md

@@ -24,6 +24,8 @@
 - API: `m.mount()` will only render its own root when called, it will not trigger a `redraw()` ([#1592](https://github.com/MithrilJS/mithril.js/pull/1592))
 - API: Assigning to `vnode.state` (as in `vnode.state = ...`) is no longer supported. Instead, an error is thrown if `vnode.state` changes upon the invocation of a lifecycle hook.
 - API: `m.request` will no longer reject the Promise on server errors (eg. status >= 400) if the caller supplies an `extract` callback. This gives applications more control over handling server responses.
+- hyperscript: when attributes have a `null` or `undefined` value, they are treated as if they were absent. [#1773](https://github.com/MithrilJS/mithril.js/issues/1773) ([#2174](https://github.com/MithrilJS/mithril.js/pull/2174))
+- hyperscript: when an attribute is defined on both the first and second argument (as a CSS selector and an `attrs` field, respectively), the latter takes precedence, except for `class` attributes that are still added together. [#2172](https://github.com/MithrilJS/mithril.js/issues/2172) ([#2174](https://github.com/MithrilJS/mithril.js/pull/2174))
 
 #### News
 

docs/hyperscript.md

@@ -5,6 +5,7 @@
 - [How it works](#how-it-works)
 - [Flexibility](#flexibility)
 - [CSS selectors](#css-selectors)
+- [Attributes passed as the second argument](attributes-passed-as-the-second-argument)
 - [DOM attributes](#dom-attributes)
 - [Style attribute](#style-attribute)
 - [Events](#events)
@@ -144,7 +145,23 @@ m("a.link[href=/]", {
 // <a href="/" class="link selected">Home</a>
 ```
 
-If there are class names in both first and second arguments of `m()`, they are merged together as you would expect.
+### Attributes passed as the second argument
+
+You can pass attributes, properties, events and lifecycle hooks in the second, optional argument (see the next sections for details).
+
+```JS
+m("button", {
+	class: "my-button",
+	onclick: function() {/* ... */},
+	oncreate: function() {/* ... */}
+})
+```
+
+If the value of such an attribute is `null` or `undefined`, it is treated as if the attribute was absent.
+
+If there are class names in both first and second arguments of `m()`, they are merged together as you would expect. If the value of the class in the second argument is `null`or `undefined`, it is ignored.
+
+If another attribute is present in both the first and the second argument, the second one takes precedence even if it is  is `null` or `undefined`.
 
 ---
 

performance/index.html

@@ -13,7 +13,9 @@
 		<script src="../test-utils/pushStateMock.js"></script>
 		<script src="../test-utils/xhrMock.js"></script>
 		<script src="../test-utils/browserMock.js"></script>
-		<script src="../mithril.js"></script>
+		<script src="../render/vnode.js"></script>
+		<script src="../render/render.js"></script>
+		<script src="../render/hyperscript.js"></script>
 		<script src="../node_modules/lodash/lodash.js"></script>
 		<script src="../node_modules/benchmark/benchmark.js"></script>
 		<script src="test-perf.js"></script>

performance/test-perf.js

@@ -31,7 +31,7 @@ var browserMock = require("../test-utils/browserMock")
 // Do this silly dance so browser testing works
 var B = typeof Benchmark === "undefined" ? require("benchmark") : Benchmark
 
-var m, scratch;
+var scratch;
 
 // set up browser env on before running tests
 var doc = typeof document !== "undefined" ? document : null
@@ -43,15 +43,20 @@ if(!doc) {
 	doc = mock.document
 }
 
-// Have to include mithril AFTER browser polyfill is set up
-m = require("../mithril") // eslint-disable-line global-require
+var m = require("../render/hyperscript")
+m.render = require("../render/render")(window).render
 
-scratch = doc.createElement("div");
 
-(doc.body || doc.documentElement).appendChild(scratch)
+function resetScratch() {
+	doc.documentElement.innerHTML = "<div></div>"
+	scratch = doc.documentElement.firstChild
+}
+
+resetScratch()
 
 // Initialize benchmark suite
 var suite = new B.Suite("mithril perf")
+var xuite = {add: function(options) {console.log("skipping " + options.name)}} // eslint-disable-line no-unused-vars
 
 suite.on("start", function() {
 	this.start = Date.now();
@@ -60,7 +65,7 @@ suite.on("start", function() {
 suite.on("cycle", function(e) {
 	console.log(e.target.toString())
 
-	scratch.innerHTML = ""
+	resetScratch()
 })
 
 suite.on("complete", function() {
@@ -228,7 +233,7 @@ suite.add({
 
 suite.add({
 	name : "mutate styles/properties",
-
+	// minSamples: 100,
 	onStart : function () {
 		var counter = 0
 		var keyLooper = function (n) { return function (c) { return c % n ? (c + "px") : c } }
@@ -259,21 +264,26 @@ suite.add({
 
 		this.count = 0
 		this.app = function (index) {
-			return m("div",
-				{
-					class: get(classes, index),
-					"data-index": index,
-					title: index.toString(36)
-				},
-				m("input", {type: "checkbox", checked: index % 3 == 0}),
-				m("input", {value: "test " + (Math.floor(index / 4)), disabled: index % 10 ? null : true}),
-				m("div", {class: get(classes, index * 11)},
-					m("p", {style: get(styles, index)}, "p1"),
-					m("p", {style: get(styles, index + 1)}, "p2"),
-					m("p", {style: get(styles, index * 2)}, "p3"),
-					m("p", {style: get(styles, index * 3 + 1)}, "p4")
+			var last = index + 300
+			var vnodes = []
+			for (; index < last; index++) vnodes.push(
+				m("div.booga",
+					{
+						class: get(classes, index),
+						"data-index": index,
+						title: index.toString(36)
+					},
+					m("input.dooga", {type: "checkbox", checked: index % 3 == 0}),
+					m("input", {value: "test " + (Math.floor(index / 4)), disabled: index % 10 ? null : true}),
+					m("div", {class: get(classes, index * 11)},
+						m("p", {style: get(styles, index)}, "p1"),
+						m("p", {style: get(styles, index + 1)}, "p2"),
+						m("p", {style: get(styles, index * 2)}, "p3"),
+						m("p.zooga", {style: get(styles, index * 3 + 1), className: get(classes, index * 7)}, "p4")
+					)
 				)
 			)
+			return vnodes
 		}
 	},
 

render/hyperscript.js

@@ -31,7 +31,8 @@ function compileSelector(selector) {
 
 function execSelector(state, attrs, children) {
 	var hasAttrs = false, childList, text
-	var className = attrs.className || attrs.class
+	var classAttr = hasOwn.call(attrs, "class") ? "class" : "className"
+	var className = attrs[classAttr]
 
 	if (!isEmpty(state.attrs) && !isEmpty(attrs)) {
 		var newAttrs = {}
@@ -46,21 +47,20 @@ function execSelector(state, attrs, children) {
 	}
 
 	for (var key in state.attrs) {
-		if (hasOwn.call(state.attrs, key)) {
+		if (hasOwn.call(state.attrs, key) && key !== "className" && !hasOwn.call(attrs, key)){
 			attrs[key] = state.attrs[key]
 		}
 	}
+	if (className != null || state.attrs.className != null) attrs.className =
+		className != null
+			? state.attrs.className != null
+				? state.attrs.className + " " + className
+				: className
+			: state.attrs.className != null
+				? state.attrs.className
+				: null
 
-	if (className !== undefined) {
-		if (attrs.class !== undefined) {
-			attrs.class = undefined
-			attrs.className = className
-		}
-
-		if (state.attrs.className != null) {
-			attrs.className = state.attrs.className + " " + className
-		}
-	}
+	if (classAttr === "class") attrs.class = null
 
 	for (var key in attrs) {
 		if (hasOwn.call(attrs, key) && key !== "key") {
@@ -75,7 +75,7 @@ function execSelector(state, attrs, children) {
 		childList = children
 	}
 
-	return Vnode(state.tag, attrs.key, hasAttrs ? attrs : undefined, childList, text)
+	return Vnode(state.tag, attrs.key, hasAttrs ? attrs : null, childList, text)
 }
 
 function hyperscript(selector) {

render/tests/test-hyperscript.js

@@ -16,52 +16,51 @@ o.spec("hyperscript", function() {
 
 			o(vnode.tag).equals("a")
 		})
-		o("v1.0.1 bug-for-bug regression suite", function(){
+		o("class and className normalization", function(){
 			o(m("a", {
 				class: null
 			}).attrs).deepEquals({
-				class: undefined,
-				className: null
+				class: null
 			})
 			o(m("a", {
 				class: undefined
 			}).attrs).deepEquals({
-				class: undefined,
+				class: null
 			})
 			o(m("a", {
 				class: false
 			}).attrs).deepEquals({
-				class: undefined,
+				class: null,
 				className: false
 			})
 			o(m("a", {
 				class: true
 			}).attrs).deepEquals({
-				class: undefined,
+				class: null,
 				className: true
 			})
 			o(m("a.x", {
 				class: null
 			}).attrs).deepEquals({
-				class: undefined,
-				className: "x null"
+				class: null,
+				className: "x"
 			})
 			o(m("a.x", {
 				class: undefined
 			}).attrs).deepEquals({
-				class: undefined,
+				class: null,
 				className: "x"
 			})
 			o(m("a.x", {
 				class: false
 			}).attrs).deepEquals({
-				class: undefined,
+				class: null,
 				className: "x false"
 			})
 			o(m("a.x", {
 				class: true
 			}).attrs).deepEquals({
-				class: undefined,
+				class: null,
 				className: "x true"
 			})
 			o(m("a", {
@@ -97,7 +96,7 @@ o.spec("hyperscript", function() {
 			o(m("a.x", {
 				className: false
 			}).attrs).deepEquals({
-				className: "x"
+				className: "x false"
 			})
 			o(m("a.x", {
 				className: true
@@ -272,7 +271,7 @@ o.spec("hyperscript", function() {
 			var vnode = m("div", {key:"a"})
 
 			o(vnode.tag).equals("div")
-			o(vnode.attrs).equals(undefined)
+			o(vnode.attrs).equals(null)
 			o(vnode.key).equals("a")
 		})
 		o("handles many attrs", function() {
@@ -490,20 +489,20 @@ o.spec("hyperscript", function() {
 		o("handles children without attr", function() {
 			var vnode = m("div", [m("i"), m("s")])
 
-			o(vnode.attrs).equals(undefined)
+			o(vnode.attrs).equals(null)
 			o(vnode.children[0].tag).equals("i")
 			o(vnode.children[1].tag).equals("s")
 		})
 		o("handles child without attr unwrapped", function() {
 			var vnode = m("div", m("i"))
 
-			o(vnode.attrs).equals(undefined)
+			o(vnode.attrs).equals(null)
 			o(vnode.children[0].tag).equals("i")
 		})
 		o("handles children without attr unwrapped", function() {
 			var vnode = m("div", m("i"), m("s"))
 
-			o(vnode.attrs).equals(undefined)
+			o(vnode.attrs).equals(null)
 			o(vnode.children[0].tag).equals("i")
 			o(vnode.children[1].tag).equals("s")
 		})
@@ -524,6 +523,15 @@ o.spec("hyperscript", function() {
 			m(".a", attrs)
 			o(attrs).deepEquals({a: "b"})
 		})
+		o("non-nullish attr takes precedence over selector", function() {
+			o(m("[a=b]", {a: "c"}).attrs).deepEquals({a: "c"})
+		})
+		o("null attr takes precedence over selector", function() {
+			o(m("[a=b]", {a: null}).attrs).deepEquals({a: null})
+		})
+		o("undefined attr takes precedence over selector", function() {
+			o(m("[a=b]", {a: undefined}).attrs).deepEquals({a: undefined})
+		})
 		o("handles fragment children without attr unwrapped", function() {
 			var vnode = m("div", [m("i")], [m("s")])