Fix keys, normalize holes (#2452)

* Fix #2434

* Treat holes as unkeyed, normalize boolean/null/undefined

This brings a lot better consistency with that API, even though it's
slightly breaking. (I had to update a bunch of tests to correspond with
it.)

* Fill in PR number [skip ci]

Author: dead-claudia

docs/change-log.md

@@ -45,6 +45,15 @@
     - Previously, it was only set for all non-`GET` methods and only when `useBody: true` was passed (the default), and it was always set for them. Now it's automatically omitted when no body is present, so the hole is slightly broadened.
 - route: query parameters in hash strings are no longer supported ([#2448](https://github.com/MithrilJS/mithril.js/pull/2448) [@isiahmeadows](https://github.com/isiahmeadows))
     - It's technically invalid in hashes, so I'd rather push people to keep in line with spec.
+- render: validate all elements are either keyed or unkeyed, and treat `null`/`undefined`/booleans as strictly unkeyed ([#2452](https://github.com/MithrilJS/mithril.js/pull/2452) [@isiahmeadows](https://github.com/isiahmeadows))
+    - Gives a nice little perf boost with keyed fragments.
+    - Minor, but imperceptible impact (within the margin of error) with unkeyed fragments.
+    - Also makes the model a lot more consistent - all values are either keyed or unkeyed.
+- vnodes: normalize boolean children to `null`/`undefined` at the vnode level, always stringify non-object children that aren't holes ([#2452](https://github.com/MithrilJS/mithril.js/pull/2452) [@isiahmeadows](https://github.com/isiahmeadows))
+    - Previously, `true` was equivalent to `"true"` and `false` was equivalent to `""`.
+    - Previously, numeric children weren't coerced. Now, they are.
+    - Unlikely to break most components, but it *could* break some users.
+    - This increases consistency with how booleans are handled with children, so it should be more intuitive.
 
 #### News
 
@@ -101,6 +110,7 @@
 - route: arbitrary prefixes are properly supported now, including odd prefixes like `?#` and invalid prefixes like `#foo#bar` ([#2448](https://github.com/MithrilJS/mithril.js/pull/2448) [@isiahmeadows](https://github.com/isiahmeadows))
 - request: correct IE workaround for response type non-support ([#2449](https://github.com/MithrilJS/mithril.js/pull/2449) [@isiahmeadows](https://github.com/isiahmeadows))
 - render: correct `contenteditable` check to also check for `contentEditable` property name ([#2450](https://github.com/MithrilJS/mithril.js/pull/2450) [@isiahmeadows](https://github.com/isiahmeadows))
+- docs: clarify valid key usage ([#2452](https://github.com/MithrilJS/mithril.js/pull/2452) [@isiahmeadows](https://github.com/isiahmeadows))
 
 ---
 

docs/hyperscript.md

@@ -159,7 +159,7 @@ m("button", {
 
 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 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`.
 

docs/keys.md

@@ -75,6 +75,27 @@ function correctUserList(users) {
 }
 ```
 
+Also, you might want to reinitialize a component. You can use the common pattern of a single-item keyed fragment where you change the key to destroy and reinitialize the element.
+
+```javascript
+function ResettableToggle() {
+	var toggleKey = false
+
+	function reset() {
+		toggleKey = !toggleKey
+	}
+
+	return {
+		view: function() {
+			return [
+				m("button", {onclick: reset}, "Reset toggle"),
+				[m(Toggle, {key: toggleKey})]
+			]
+		}
+	}
+}
+```
+
 ---
 
 ### Debugging key related issues
@@ -178,6 +199,10 @@ m("div", [
 ])
 ```
 
+In fact, this will cause an error to be thrown, to remind you to not do it. So don't do it!
+
+Note that `null`s, `undefined`s, and booleans are considered unkeyed nodes. If you want the keyed equivalent, use `m.fragment({key: ...}, [])`, a keyed empty fragment.
+
 #### Avoid passing model data directly to components if the model uses `key` as a data property
 
 The `key` property may appear in your data model in a way that conflicts with Mithril's key logic. For example, a component may represent an entity whose `key` property is expected to change over time. This can lead to components receiving the wrong data, re-initialize, or change positions unexpectedly. If your data model uses the `key` property, make sure to wrap the data such that Mithril doesn't misinterpret it as a rendering instruction:

render/hyperscript.js

@@ -93,7 +93,7 @@ function hyperscript(selector) {
 		vnode.children = Vnode.normalizeChildren(vnode.children)
 		if (selector !== "[") return execSelector(selectorCache[selector] || compileSelector(selector), vnode)
 	}
-	
+
 	vnode.tag = selector
 	return vnode
 }

render/render.js

@@ -44,8 +44,24 @@ module.exports = function($window) {
 			return null
 		}
 	}
+	function validateKeys(vnodes, isKeyed) {
+		// Note: this is a *very* perf-sensitive check.
+		// Fun fact: merging the loop like this is somehow faster than splitting
+		// it, noticeably so.
+		for (var i = 1; i < vnodes.length; i++) {
+			if ((vnodes[i] != null && vnodes[i].key != null) !== isKeyed) {
+				throw new TypeError("Vnodes must either always have keys or never have keys!")
+			}
+		}
+	}
 	//create
-	function createNodes(parent, vnodes, start, end, hooks, nextSibling, ns) {
+	function createNodesChecked(parent, vnodes, hooks, nextSibling, ns) {
+		if (vnodes.length) {
+			validateKeys(vnodes, vnodes[0] != null && vnodes[0].key != null)
+			createNodesUnchecked(parent, vnodes, 0, vnodes.length, hooks, nextSibling, ns)
+		}
+	}
+	function createNodesUnchecked(parent, vnodes, start, end, hooks, nextSibling, ns) {
 		for (var i = start; i < end; i++) {
 			var vnode = vnodes[i]
 			if (vnode != null) {
@@ -99,7 +115,7 @@ module.exports = function($window) {
 		var fragment = $doc.createDocumentFragment()
 		if (vnode.children != null) {
 			var children = vnode.children
-			createNodes(fragment, children, 0, children.length, hooks, null, ns)
+			createNodesChecked(fragment, children, hooks, null, ns)
 		}
 		vnode.dom = fragment.firstChild
 		vnode.domSize = fragment.childNodes.length
@@ -130,7 +146,7 @@ module.exports = function($window) {
 			}
 			if (vnode.children != null) {
 				var children = vnode.children
-				createNodes(element, children, 0, children.length, hooks, null, ns)
+				createNodesChecked(element, children, hooks, null, ns)
 				if (vnode.tag === "select" && attrs != null) setLateSelectAttrs(vnode, attrs)
 			}
 		}
@@ -273,26 +289,19 @@ module.exports = function($window) {
 
 	function updateNodes(parent, old, vnodes, hooks, nextSibling, ns) {
 		if (old === vnodes || old == null && vnodes == null) return
-		else if (old == null || old.length === 0) createNodes(parent, vnodes, 0, vnodes.length, hooks, nextSibling, ns)
+		else if (old == null || old.length === 0) createNodesChecked(parent, vnodes, hooks, nextSibling, ns)
 		else if (vnodes == null || vnodes.length === 0) removeNodes(old, 0, old.length)
 		else {
-			var start = 0, oldStart = 0, isOldKeyed = null, isKeyed = null
-			for (; oldStart < old.length; oldStart++) {
-				if (old[oldStart] != null) {
-					isOldKeyed = old[oldStart].key != null
-					break
-				}
-			}
-			for (; start < vnodes.length; start++) {
-				if (vnodes[start] != null) {
-					isKeyed = vnodes[start].key != null
-					break
-				}
-			}
+			var isOldKeyed = old[0] != null && old[0].key != null
+			var isKeyed = vnodes[0] != null && vnodes[0].key != null
+			var start = 0, oldStart = 0
+			validateKeys(vnodes, isKeyed)
+			if (!isOldKeyed) while (oldStart < old.length && old[oldStart] == null) oldStart++
+			if (!isKeyed) while (start < vnodes.length && vnodes[start] == null) start++
 			if (isKeyed === null && isOldKeyed == null) return // both lists are full of nulls
 			if (isOldKeyed !== isKeyed) {
 				removeNodes(old, oldStart, old.length)
-				createNodes(parent, vnodes, start, vnodes.length, hooks, nextSibling, ns)
+				createNodesUnchecked(parent, vnodes, start, vnodes.length, hooks, nextSibling, ns)
 			} else if (!isKeyed) {
 				// Don't index past the end of either list (causes deopts).
 				var commonLength = old.length < vnodes.length ? old.length : vnodes.length
@@ -309,7 +318,7 @@ module.exports = function($window) {
 					else updateNode(parent, o, v, hooks, getNextSibling(old, start + 1, nextSibling), ns)
 				}
 				if (old.length > commonLength) removeNodes(old, start, old.length)
-				if (vnodes.length > commonLength) createNodes(parent, vnodes, start, vnodes.length, hooks, nextSibling, ns)
+				if (vnodes.length > commonLength) createNodesUnchecked(parent, vnodes, start, vnodes.length, hooks, nextSibling, ns)
 			} else {
 				// keyed diff
 				var oldEnd = old.length - 1, end = vnodes.length - 1, map, o, v, oe, ve, topSibling
@@ -318,90 +327,67 @@ module.exports = function($window) {
 				while (oldEnd >= oldStart && end >= start) {
 					oe = old[oldEnd]
 					ve = vnodes[end]
-					if (oe == null) oldEnd--
-					else if (ve == null) end--
-					else if (oe.key === ve.key) {
-						if (oe !== ve) updateNode(parent, oe, ve, hooks, nextSibling, ns)
-						if (ve.dom != null) nextSibling = ve.dom
-						oldEnd--, end--
-					} else {
-						break
-					}
+					if (oe.key !== ve.key) break
+					if (oe !== ve) updateNode(parent, oe, ve, hooks, nextSibling, ns)
+					if (ve.dom != null) nextSibling = ve.dom
+					oldEnd--, end--
 				}
 				// top-down
 				while (oldEnd >= oldStart && end >= start) {
 					o = old[oldStart]
 					v = vnodes[start]
-					if (o == null) oldStart++
-					else if (v == null) start++
-					else if (o.key === v.key) {
-						oldStart++, start++
-						if (o !== v) updateNode(parent, o, v, hooks, getNextSibling(old, oldStart, nextSibling), ns)
-					} else {
-						break
-					}
+					if (o.key !== v.key) break
+					oldStart++, start++
+					if (o !== v) updateNode(parent, o, v, hooks, getNextSibling(old, oldStart, nextSibling), ns)
 				}
 				// swaps and list reversals
 				while (oldEnd >= oldStart && end >= start) {
-					if (o == null) oldStart++
-					else if (v == null) start++
-					else if (oe == null) oldEnd--
-					else if (ve == null) end--
-					else if (start === end) break
-					else {
-						if (o.key !== ve.key || oe.key !== v.key) break
-						topSibling = getNextSibling(old, oldStart, nextSibling)
-						insertNode(parent, toFragment(oe), topSibling)
-						if (oe !== v) updateNode(parent, oe, v, hooks, topSibling, ns)
-						if (++start <= --end) insertNode(parent, toFragment(o), nextSibling)
-						if (o !== ve) updateNode(parent, o, ve, hooks, nextSibling, ns)
-						if (ve.dom != null) nextSibling = ve.dom
-						oldStart++; oldEnd--
-					}
+					if (start === end) break
+					if (o.key !== ve.key || oe.key !== v.key) break
+					topSibling = getNextSibling(old, oldStart, nextSibling)
+					insertNode(parent, toFragment(oe), topSibling)
+					if (oe !== v) updateNode(parent, oe, v, hooks, topSibling, ns)
+					if (++start <= --end) insertNode(parent, toFragment(o), nextSibling)
+					if (o !== ve) updateNode(parent, o, ve, hooks, nextSibling, ns)
+					if (ve.dom != null) nextSibling = ve.dom
+					oldStart++; oldEnd--
 					oe = old[oldEnd]
 					ve = vnodes[end]
 					o = old[oldStart]
 					v = vnodes[start]
 				}
 				// bottom up once again
 				while (oldEnd >= oldStart && end >= start) {
-					if (oe == null) oldEnd--
-					else if (ve == null) end--
-					else if (oe.key === ve.key) {
-						if (oe !== ve) updateNode(parent, oe, ve, hooks, nextSibling, ns)
-						if (ve.dom != null) nextSibling = ve.dom
-						oldEnd--, end--
-					} else {
-						break
-					}
+					if (oe.key !== ve.key) break
+					if (oe !== ve) updateNode(parent, oe, ve, hooks, nextSibling, ns)
+					if (ve.dom != null) nextSibling = ve.dom
+					oldEnd--, end--
 					oe = old[oldEnd]
 					ve = vnodes[end]
 				}
 				if (start > end) removeNodes(old, oldStart, oldEnd + 1)
-				else if (oldStart > oldEnd) createNodes(parent, vnodes, start, end + 1, hooks, nextSibling, ns)
+				else if (oldStart > oldEnd) createNodesUnchecked(parent, vnodes, start, end + 1, hooks, nextSibling, ns)
 				else {
 					// inspired by ivi https://github.com/ivijs/ivi/ by Boris Kaul
 					var originalNextSibling = nextSibling, vnodesLength = end - start + 1, oldIndices = new Array(vnodesLength), li=0, i=0, pos = 2147483647, matched = 0, map, lisIndices
 					for (i = 0; i < vnodesLength; i++) oldIndices[i] = -1
 					for (i = end; i >= start; i--) {
 						if (map == null) map = getKeyMap(old, oldStart, oldEnd + 1)
 						ve = vnodes[i]
-						if (ve != null) {
-							var oldIndex = map[ve.key]
-							if (oldIndex != null) {
-								pos = (oldIndex < pos) ? oldIndex : -1 // becomes -1 if nodes were re-ordered
-								oldIndices[i-start] = oldIndex
-								oe = old[oldIndex]
-								old[oldIndex] = null
-								if (oe !== ve) updateNode(parent, oe, ve, hooks, nextSibling, ns)
-								if (ve.dom != null) nextSibling = ve.dom
-								matched++
-							}
+						var oldIndex = map[ve.key]
+						if (oldIndex != null) {
+							pos = (oldIndex < pos) ? oldIndex : -1 // becomes -1 if nodes were re-ordered
+							oldIndices[i-start] = oldIndex
+							oe = old[oldIndex]
+							old[oldIndex] = null
+							if (oe !== ve) updateNode(parent, oe, ve, hooks, nextSibling, ns)
+							if (ve.dom != null) nextSibling = ve.dom
+							matched++
 						}
 					}
 					nextSibling = originalNextSibling
 					if (matched !== oldEnd - oldStart + 1) removeNodes(old, oldStart, oldEnd + 1)
-					if (matched === 0) createNodes(parent, vnodes, start, end + 1, hooks, nextSibling, ns)
+					if (matched === 0) createNodesUnchecked(parent, vnodes, start, end + 1, hooks, nextSibling, ns)
 					else {
 						if (pos === -1) {
 							// the indices of the indices of the items that are part of the
@@ -541,26 +527,26 @@ module.exports = function($window) {
 	// occur multiple times) and returns an array with the indices
 	// of the items that are part of the longest increasing
 	// subsequece
+	var lisTemp = []
 	function makeLisIndices(a) {
-		var p = a.slice()
-		var result = []
-		result.push(0)
-		var u
-		var v
-		for (var i = 0, il = a.length; i < il; ++i) {
-			if (a[i] === -1) {
-				continue
-			}
+		var result = [0]
+		var u = 0, v = 0, i = 0
+		var il = lisTemp.length = a.length
+		for (var i = 0; i < il; i++) lisTemp[i] = a[i]
+		for (var i = 0; i < il; ++i) {
+			if (a[i] === -1) continue
 			var j = result[result.length - 1]
 			if (a[j] < a[i]) {
-				p[i] = j
+				lisTemp[i] = j
 				result.push(i)
 				continue
 			}
 			u = 0
 			v = result.length - 1
 			while (u < v) {
-				var c = ((u + v) / 2) | 0 // eslint-disable-line no-bitwise
+				// Fast integer average without overflow.
+				// eslint-disable-next-line no-bitwise
+				var c = (u >>> 1) + (v >>> 1) + (u & v & 1)
 				if (a[result[c]] < a[i]) {
 					u = c + 1
 				}
@@ -569,18 +555,17 @@ module.exports = function($window) {
 				}
 			}
 			if (a[i] < a[result[u]]) {
-				if (u > 0) {
-					p[i] = result[u - 1]
-				}
+				if (u > 0) lisTemp[i] = result[u - 1]
 				result[u] = i
 			}
 		}
 		u = result.length
 		v = result[u - 1]
 		while (u-- > 0) {
 			result[u] = v
-			v = p[v]
+			v = lisTemp[v]
 		}
+		lisTemp.length = 0
 		return result
 	}