Add m.censor, adjust m.route.Link to use it (#2538)

Also, restructure a few things for better code reuse.

api/router.js

@@ -7,7 +7,8 @@ var Promise = require("../promise/promise")
 var buildPathname = require("../pathname/build")
 var parsePathname = require("../pathname/parse")
 var compileTemplate = require("../pathname/compileTemplate")
-var assign = require("../pathname/assign")
+var assign = require("../util/assign")
+var censor = require("../util/censor")
 
 var sentinel = {}
 
@@ -189,20 +190,17 @@ module.exports = function($window, mountRedraw) {
 	route.prefix = "#!"
 	route.Link = {
 		view: function(vnode) {
-			var options = vnode.attrs.options
+			// Omit the used parameters from the rendered element - they are
-			// Remove these so they don't get overwritten
+			// internal. Also, censor the various lifecycle methods.
-			var attrs = {}, onclick, href
+			//
-			assign(attrs, vnode.attrs)
+			// We don't strip the other parameters because for convenience we
-			// The first two are internal, but the rest are magic attributes
+			// let them be specified in the selector as well.
-			// that need censored to not screw up rendering.
+			var child = m(
-			attrs.selector = attrs.options = attrs.key = attrs.oninit =
+				vnode.attrs.selector || "a",
-			attrs.oncreate = attrs.onbeforeupdate = attrs.onupdate =
+				censor(vnode.attrs, ["options", "params", "selector", "onclick"]),
-			attrs.onbeforeremove = attrs.onremove = null
+				vnode.children
-
+			)
-			// Do this now so we can get the most current `href` and `disabled`.
+			var options, onclick, href
-			// Those attributes may also be specified in the selector, and we
-			// should honor that.
-			var child = m(vnode.attrs.selector || "a", attrs, vnode.children)
 
 			// Let's provide a *right* way to disable a route link, rather than
 			// letting people screw up accessibility on accident.
@@ -213,13 +211,13 @@ module.exports = function($window, mountRedraw) {
 			if (child.attrs.disabled = Boolean(child.attrs.disabled)) {
 				child.attrs.href = null
 				child.attrs["aria-disabled"] = "true"
-				// If you *really* do want to do this on a disabled link, use
+				// If you *really* do want add `onclick` on a disabled link, use
 				// an `oncreate` hook to add it.
-				child.attrs.onclick = null
 			} else {
-				onclick = child.attrs.onclick
+				options = vnode.attrs.options
+				onclick = vnode.attrs.onclick
 				// Easier to build it now to keep it isomorphic.
-				href = buildPathname(child.attrs.href, child.attrs.params)
+				href = buildPathname(child.attrs.href, vnode.attrs.params)
 				child.attrs.href = route.prefix + href
 				child.attrs.onclick = function(e) {
 					var result

docs/censor.md

@@ -0,0 +1,147 @@
+# censor(object, extra)
+
+- [Description](#description)
+- [Signature](#signature)
+- [How it works](#signature)
+
+---
+
+### Description
+
+Returns a shallow-cloned object with lifecycle attributes and any given custom attributes omitted.
+
+```javascript
+var attrs = {one: "two", enabled: false, oninit: function() {}}
+var censored = m.censor(attrs, ["enabled"])
+// {one: "two"}
+```
+
+---
+
+### Signature
+
+`censored = m.censor(object, extra)`
+
+Argument     | Type                                       | Required | Description
+------------ | ------------------------------------------ | -------- | ---
+`object`     | `Object`                                   | Yes      | A key-value map to be converted into a string
+`extra`      | `Array<String>`                            | No       | Additional properties to omit.
+**returns**  | `Object`                                   |          | The original object if no properties to omit existed on it, a shallow-cloned object with the removed properties otherwise.
+
+[How to read signatures](signatures.md)
+
+---
+
+### How it works
+
+Ordinarily, you don't need this method, and you'll just want to specify the attributes you want. But sometimes, it's more convenient to send all attributes you don't know to another element. This is often perfectly reasonable, but it can lead you into a major trap with lifecycle methods getting called twice.
+
+```javascript
+function SomePage() {
+	return {
+		view: function() {
+			return m(SomeFancyView, {
+				oncreate: function() {
+					sendViewHit(m.route.get(), "some fancy view")
+				},
+			})
+		},
+	}
+}
+
+function SomeFancyView() {
+	return {
+		view: function(vnode) {
+			return m("div", vnode.attrs, [ // !!!
+				// ...
+			])
+		},
+	}
+}
+```
+
+This looks benign, but this creates a problem: you're sending two hits each time this view is navigated. This is where `m.censor` come in: it lets you strip that `oncreate` from the attributes so it only gets called once and so the caller can remain sane and rest assured they aren't dealing with super weird bugs because of it.
+
+```javascript
+// Fixed
+function SomeFancyView() {
+	return {
+		view: function(vnode) {
+			return m("div", m.censor(vnode.attrs), [
+				// ...
+			])
+		},
+	}
+}
+```
+
+You can also run into similar issues with keys:
+
+```javascript
+function SomePage() {
+	return {
+		view: function() {
+			return m(Layout, {
+				pageTitle: "Some Page",
+				key: someKey,
+			}, [
+				// ...
+			])
+		},
+	}
+}
+
+function Layout() {
+	return {
+		view: function(vnode) {
+			return [
+				m("header", [
+					m("h1", "My beautiful web app"),
+					m("nav"),
+				]),
+				m(".body", vnode.attrs, [ // !!!
+					m("h2", vnode.attrs.pageTitle),
+					vnode.children,
+				])
+			]
+		},
+	}
+}
+```
+
+This would end up [throwing an error](keys.md#avoid-mixing-keyed-and-non-keyed-vnodes-in-the-same-array) because here's what Mithril sees when creating the `Layout` vnode:
+
+```javascript
+return [
+	m("header", [
+		m("h1", "My beautiful web app"),
+		m("nav"),
+	]),
+	m(".body", {pageTitle: "Some Page", key: someKey}, [
+		m("h2", "Some Page"),
+		[/* ... */],
+	])
+]
+```
+
+You wouldn't likely catch that at first glance, especially in much more real-world scenarios where there might be indirection and/or other issues. To correct this, you similarly have to censor out the `key:` attribute. You can also censor out the custom `pageTitle` attribute, too, since it doesn't provide any real value being in the DOM.
+
+```javascript
+// Fixed
+function Layout() {
+	return {
+		view: function(vnode) {
+			return [
+				m("header", [
+					m("h1", "My beautiful web app"),
+					m("nav"),
+				]),
+				m(".body", m.censor(vnode.attrs, ["pageTitle"]), [
+					m("h2", vnode.attrs.pageTitle),
+					vnode.children,
+				])
+			]
+		},
+	}
+}
+```

docs/change-log.md

@@ -25,6 +25,7 @@
 - For a better debugging experience with `m.route` route resolvers, errors on `onmatch` in the default route are left unhandled and errors in `onmatch` in other routes are logged to the console before redirecting. ([#2536](https://github.com/MithrilJS/mithril.js/pull/2536) [@isiahmeadows](https://github.com/isiahmeadows))
 - Bug fix with `m.redraw` where if you removed a root that was previously visited in the current redraw pass, it would lose its place and skip the next root.
 - Add `params:` attribute to `m.route.Link`. ([#2537](https://github.com/MithrilJS/mithril.js/pull/2537) [@isiahmeadows](https://github.com/isiahmeadows))
+- Add `m.censor`. ([#2538](https://github.com/MithrilJS/mithril.js/pull/2538) [@isiahmeadows](https://github.com/isiahmeadows))
 
 -->
 

docs/signatures.md

@@ -41,7 +41,7 @@ A splat argument means that if the argument is an array, you can omit the square
 
 In the example at the top, this means that `m("div", {id: "foo"}, ["a", "b", "c"])` can also be written as `m("div", {id: "foo"}, "a", "b", "c")`.
 
-Splats are useful in some compile-to-js languages such as Coffeescript, and also allow helpful shorthands for some common use cases.
+Splats are useful in some compile-to-JS languages such as CoffeeScript, and also allow helpful shorthands for some common use cases.
 
 ---
 

index.js

@@ -20,5 +20,6 @@ m.parsePathname = require("./pathname/parse")
 m.buildPathname = require("./pathname/build")
 m.vnode = require("./render/vnode")
 m.PromisePolyfill = require("./promise/polyfill")
+m.censor = require("./util/censor")
 
 module.exports = m

pathname/assign.js

@@ -1,5 +0,0 @@
-"use strict"
-
-module.exports = Object.assign || function(target, source) {
-	if(source) Object.keys(source).forEach(function(key) { target[key] = source[key] })
-}

pathname/build.js

@@ -1,7 +1,7 @@
 "use strict"
 
 var buildQueryString = require("../querystring/build")
-var assign = require("./assign")
+var assign = require("../util/assign")
 
 // Returns `path` from `template` + `params`
 module.exports = function(template, params) {

pathname/tests/test-assign.js

@@ -1,26 +0,0 @@
-"use strict"
-
-var o = require("../../ospec/ospec")
-
-// force usage of polyfill
-var save = Object.assign
-Object.assign = null
-delete require.cache[require.resolve("../assign")]
-var assign = require("../assign")
-Object.assign = save
-
-o.spec("assign polyfill", function() {
-	o("works", function() {
-		var target = {hello: "world", foo: "bar"}
-		var source = {foo: "foo", extra: true}
-
-		assign(target, source)
-
-		o(target).deepEquals({hello: "world", foo: "foo", extra: true})
-
-		var falsySources = [null, 0, "", false, void 0]
-		falsySources.forEach(function(falsy) { assign(target, falsy) })
-
-		o(target).deepEquals({hello: "world", foo: "foo", extra: true})
-	})
-})

render/hyperscript.js

@@ -2,10 +2,10 @@
 
 var Vnode = require("../render/vnode")
 var hyperscriptVnode = require("./hyperscriptVnode")
+var hasOwn = require("../util/hasOwn")
 
 var selectorParser = /(?:(^|#|\.)([^#\.\[\]]+))|(\[(.+?)(?:\s*=\s*("|'|)((?:\\["'\]]|.)*?)\5)?\])/g
 var selectorCache = {}
-var hasOwn = {}.hasOwnProperty
 
 function isEmpty(object) {
 	for (var key in object) if (hasOwn.call(object, key)) return false

request/request.js

@@ -1,6 +1,7 @@
 "use strict"
 
 var buildPathname = require("../pathname/build")
+var hasOwn = require("../util/hasOwn")
 
 module.exports = function($window, Promise, oncompletion) {
 	var callbackCount = 0
@@ -66,7 +67,7 @@ module.exports = function($window, Promise, oncompletion) {
 
 	function hasHeader(args, name) {
 		for (var key in args.headers) {
-			if ({}.hasOwnProperty.call(args.headers, key) && name.test(key)) return true
+			if (hasOwn.call(args.headers, key) && name.test(key)) return true
 		}
 		return false
 	}
@@ -100,7 +101,7 @@ module.exports = function($window, Promise, oncompletion) {
 			xhr.responseType = responseType
 
 			for (var key in args.headers) {
-				if ({}.hasOwnProperty.call(args.headers, key)) {
+				if (hasOwn.call(args.headers, key)) {
 					xhr.setRequestHeader(key, args.headers[key])
 				}
 			}

util/assign.js

@@ -0,0 +1,10 @@
+// This exists so I'm only saving it once.
+"use strict"
+
+var hasOwn = require("./hasOwn")
+
+module.exports = Object.assign || function(target, source) {
+	for (var key in source) {
+		if (hasOwn.call(source, key)) target[key] = source[key]
+	}
+}

util/censor.js

@@ -0,0 +1,47 @@
+"use strict"
+
+// Note: this is mildly perf-sensitive.
+//
+// It does *not* use `delete` - dynamic `delete`s usually cause objects to bail
+// out into dictionary mode and just generally cause a bunch of optimization
+// issues within engines.
+//
+// Ideally, I would've preferred to do this, if it weren't for the optimization
+// issues:
+//
+// ```js
+// const hasOwn = require("./hasOwn")
+// const magic = [
+//     "key", "oninit", "oncreate", "onbeforeupdate", "onupdate",
+//     "onbeforeremove", "onremove",
+// ]
+// module.exports = (attrs, extras) => {
+//     const result = Object.assign(Object.create(null), attrs)
+//     for (const key of magic) delete result[key]
+//     if (extras != null) for (const key of extras) delete result[key]
+//     return result
+// }
+// ```
+
+var hasOwn = require("./hasOwn")
+var magic = /^(?:key|oninit|oncreate|onbeforeupdate|onupdate|onbeforeremove|onremove)$/
+
+module.exports = function(attrs, extras) {
+	var result = {}
+
+	if (extras != null) {
+		for (var key in attrs) {
+			if (hasOwn.call(attrs, key) && !magic.test(key) && extras.indexOf(key) < 0) {
+				result[key] = attrs[key]
+			}
+		}
+	} else {
+		for (var key in attrs) {
+			if (hasOwn.call(attrs, key) && !magic.test(key)) {
+				result[key] = attrs[key]
+			}
+		}
+	}
+
+	return result
+}

util/hasOwn.js

@@ -0,0 +1,4 @@
+// This exists so I'm only saving it once.
+"use strict"
+
+module.exports = {}.hasOwnProperty

util/tests/index.html

@@ -0,0 +1,17 @@
+<!doctype html>
+<html>
+	<head>
+		<meta charset="utf-8">
+	</head>
+	<body>
+		<script src="../../module/module.js"></script>
+		<script src="../../ospec/ospec.js"></script>
+
+		<script src="../../util/hasOwn.js"></script>
+		<script src="../../util/assign.js"></script>
+		<script src="../../util/censor.js"></script>
+		<script src="test-censor.js"></script>
+
+		<script>require("../../ospec/ospec").run()</script>
+	</body>
+</html>

util/tests/test-censor.js

@@ -0,0 +1,238 @@
+"use strict"
+
+var o = require("../../ospec/ospec")
+var censor = require("../../util/censor")
+
+o.spec("censor", function() {
+	o.spec("magic missing, no extras", function() {
+		o("returns new object", function() {
+			var original = {one: "two"}
+			var censored = censor(original)
+			o(censored).notEquals(original)
+			o(censored).deepEquals({one: "two"})
+		})
+		o("does not modify original object", function() {
+			var original = {one: "two"}
+			censor(original)
+			o(original).deepEquals({one: "two"})
+		})
+	})
+
+	o.spec("magic present, no extras", function() {
+		o("returns new object", function() {
+			var original = {
+				one: "two",
+				key: "test",
+				oninit: "test",
+				oncreate: "test",
+				onbeforeupdate: "test",
+				onupdate: "test",
+				onbeforeremove: "test",
+				onremove: "test",
+			}
+			var censored = censor(original)
+			o(censored).notEquals(original)
+			o(censored).deepEquals({one: "two"})
+		})
+		o("does not modify original object", function() {
+			var original = {
+				one: "two",
+				key: "test",
+				oninit: "test",
+				oncreate: "test",
+				onbeforeupdate: "test",
+				onupdate: "test",
+				onbeforeremove: "test",
+				onremove: "test",
+			}
+			censor(original)
+			o(original).deepEquals({
+				one: "two",
+				key: "test",
+				oninit: "test",
+				oncreate: "test",
+				onbeforeupdate: "test",
+				onupdate: "test",
+				onbeforeremove: "test",
+				onremove: "test",
+			})
+		})
+	})
+
+	o.spec("magic missing, null extras", function() {
+		o("returns new object", function() {
+			var original = {one: "two"}
+			var censored = censor(original, null)
+			o(censored).notEquals(original)
+			o(censored).deepEquals({one: "two"})
+		})
+		o("does not modify original object", function() {
+			var original = {one: "two"}
+			censor(original, null)
+			o(original).deepEquals({one: "two"})
+		})
+	})
+
+	o.spec("magic present, null extras", function() {
+		o("returns new object", function() {
+			var original = {
+				one: "two",
+				key: "test",
+				oninit: "test",
+				oncreate: "test",
+				onbeforeupdate: "test",
+				onupdate: "test",
+				onbeforeremove: "test",
+				onremove: "test",
+			}
+			var censored = censor(original, null)
+			o(censored).notEquals(original)
+			o(censored).deepEquals({one: "two"})
+		})
+		o("does not modify original object", function() {
+			var original = {
+				one: "two",
+				key: "test",
+				oninit: "test",
+				oncreate: "test",
+				onbeforeupdate: "test",
+				onupdate: "test",
+				onbeforeremove: "test",
+				onremove: "test",
+			}
+			censor(original, null)
+			o(original).deepEquals({
+				one: "two",
+				key: "test",
+				oninit: "test",
+				oncreate: "test",
+				onbeforeupdate: "test",
+				onupdate: "test",
+				onbeforeremove: "test",
+				onremove: "test",
+			})
+		})
+	})
+
+	o.spec("magic missing, extras missing", function() {
+		o("returns new object", function() {
+			var original = {one: "two"}
+			var censored = censor(original, ["extra"])
+			o(censored).notEquals(original)
+			o(censored).deepEquals({one: "two"})
+		})
+		o("does not modify original object", function() {
+			var original = {one: "two"}
+			censor(original, ["extra"])
+			o(original).deepEquals({one: "two"})
+		})
+	})
+
+	o.spec("magic present, extras missing", function() {
+		o("returns new object", function() {
+			var original = {
+				one: "two",
+				key: "test",
+				oninit: "test",
+				oncreate: "test",
+				onbeforeupdate: "test",
+				onupdate: "test",
+				onbeforeremove: "test",
+				onremove: "test",
+			}
+			var censored = censor(original, ["extra"])
+			o(censored).notEquals(original)
+			o(censored).deepEquals({one: "two"})
+		})
+		o("does not modify original object", function() {
+			var original = {
+				one: "two",
+				key: "test",
+				oninit: "test",
+				oncreate: "test",
+				onbeforeupdate: "test",
+				onupdate: "test",
+				onbeforeremove: "test",
+				onremove: "test",
+			}
+			censor(original, ["extra"])
+			o(original).deepEquals({
+				one: "two",
+				key: "test",
+				oninit: "test",
+				oncreate: "test",
+				onbeforeupdate: "test",
+				onupdate: "test",
+				onbeforeremove: "test",
+				onremove: "test",
+			})
+		})
+	})
+
+	o.spec("magic missing, extras present", function() {
+		o("returns new object", function() {
+			var original = {
+				one: "two",
+				extra: "test",
+			}
+			var censored = censor(original, ["extra"])
+			o(censored).notEquals(original)
+			o(censored).deepEquals({one: "two"})
+		})
+		o("does not modify original object", function() {
+			var original = {
+				one: "two",
+				extra: "test",
+			}
+			censor(original, ["extra"])
+			o(original).deepEquals({
+				one: "two",
+				extra: "test",
+			})
+		})
+	})
+
+	o.spec("magic present, extras present", function() {
+		o("returns new object", function() {
+			var original = {
+				one: "two",
+				extra: "test",
+				key: "test",
+				oninit: "test",
+				oncreate: "test",
+				onbeforeupdate: "test",
+				onupdate: "test",
+				onbeforeremove: "test",
+				onremove: "test",
+			}
+			var censored = censor(original, ["extra"])
+			o(censored).notEquals(original)
+			o(censored).deepEquals({one: "two"})
+		})
+		o("does not modify original object", function() {
+			var original = {
+				one: "two",
+				extra: "test",
+				key: "test",
+				oninit: "test",
+				oncreate: "test",
+				onbeforeupdate: "test",
+				onupdate: "test",
+				onbeforeremove: "test",
+				onremove: "test",
+			}
+			censor(original, ["extra"])
+			o(original).deepEquals({
+				one: "two",
+				extra: "test",
+				key: "test",
+				oninit: "test",
+				oncreate: "test",
+				onbeforeupdate: "test",
+				onupdate: "test",
+				onbeforeremove: "test",
+				onremove: "test",
+			})
+		})
+	})
+})