Reverse hook order for all but onbeforeupdate (#2297)

- Drive-by: `onbeforeupdate` prevents subtree redraw if *either* hook
  returns `false`, not *both*.

docs/change-log.md

@@ -32,6 +32,7 @@
 - render: Align custom elements to work like normal elements, minus all the HTML-specific magic. ([#2221](https://github.com/MithrilJS/mithril.js/pull/2221))
 - render: simplify component removal ([#2214](https://github.com/MithrilJS/mithril.js/pull/2214))
 - cast className using toString ([#2309](https://github.com/MithrilJS/mithril.js/pull/2309))
+- render: call attrs' hooks first, with express exception of `onbeforeupdate` to allow attrs to block components from even diffing ([#2297](https://github.com/MithrilJS/mithril.js/pull/2297))
 
 #### News
 

render/render.js

@@ -152,8 +152,8 @@ module.exports = function($window) {
 			sentinel.$$reentrantLock$$ = true
 			vnode.state = (vnode.tag.prototype != null && typeof vnode.tag.prototype.view === "function") ? new vnode.tag(vnode) : vnode.tag(vnode)
 		}
-		if (vnode.attrs != null) initLifecycle(vnode.attrs, vnode, hooks)
 		initLifecycle(vnode.state, vnode, hooks)
+		if (vnode.attrs != null) initLifecycle(vnode.attrs, vnode, hooks)
 		vnode.instance = Vnode.normalize(callHook.call(vnode.state.view, vnode))
 		if (vnode.instance === vnode) throw Error("A view cannot return the vnode it received as argument")
 		sentinel.$$reentrantLock$$ = null
@@ -511,8 +511,8 @@ module.exports = function($window) {
 	function updateComponent(parent, old, vnode, hooks, nextSibling, ns) {
 		vnode.instance = Vnode.normalize(callHook.call(vnode.state.view, vnode))
 		if (vnode.instance === vnode) throw Error("A view cannot return the vnode it received as argument")
-		if (vnode.attrs != null) updateLifecycle(vnode.attrs, vnode, hooks)
 		updateLifecycle(vnode.state, vnode, hooks)
+		if (vnode.attrs != null) updateLifecycle(vnode.attrs, vnode, hooks)
 		if (vnode.instance != null) {
 			if (old.instance == null) createNode(parent, vnode.instance, hooks, ns, nextSibling)
 			else updateNode(parent, old.instance, vnode.instance, hooks, nextSibling, ns)
@@ -632,15 +632,15 @@ module.exports = function($window) {
 	function removeNode(vnode) {
 		var expected = 1, called = 0
 		var original = vnode.state
-		if (vnode.attrs && typeof vnode.attrs.onbeforeremove === "function") {
+		if (typeof vnode.tag !== "string" && typeof vnode.state.onbeforeremove === "function") {
-			var result = callHook.call(vnode.attrs.onbeforeremove, vnode)
+			var result = callHook.call(vnode.state.onbeforeremove, vnode)
 			if (result != null && typeof result.then === "function") {
 				expected++
 				result.then(continuation, continuation)
 			}
 		}
-		if (typeof vnode.tag !== "string" && typeof vnode.state.onbeforeremove === "function") {
+		if (vnode.attrs && typeof vnode.attrs.onbeforeremove === "function") {
-			var result = callHook.call(vnode.state.onbeforeremove, vnode)
+			var result = callHook.call(vnode.attrs.onbeforeremove, vnode)
 			if (result != null && typeof result.then === "function") {
 				expected++
 				result.then(continuation, continuation)
@@ -661,9 +661,9 @@ module.exports = function($window) {
 		}
 	}
 	function onremove(vnode) {
+		if (typeof vnode.tag !== "string" && typeof vnode.state.onremove === "function") callHook.call(vnode.state.onremove, vnode)
 		if (vnode.attrs && typeof vnode.attrs.onremove === "function") callHook.call(vnode.attrs.onremove, vnode)
 		if (typeof vnode.tag !== "string") {
-			if (typeof vnode.state.onremove === "function") callHook.call(vnode.state.onremove, vnode)
 			if (vnode.instance != null) onremove(vnode.instance)
 		} else {
 			var children = vnode.children
@@ -863,21 +863,22 @@ module.exports = function($window) {
 		if (typeof source.onupdate === "function") hooks.push(callHook.bind(source.onupdate, vnode))
 	}
 	function shouldNotUpdate(vnode, old) {
-		var forceVnodeUpdate, forceComponentUpdate
+		do {
 			if (vnode.attrs != null && typeof vnode.attrs.onbeforeupdate === "function") {
-			forceVnodeUpdate = callHook.call(vnode.attrs.onbeforeupdate, vnode, old)
+				var force = callHook.call(vnode.attrs.onbeforeupdate, vnode, old)
+				if (force !== undefined && !force) break
 			}
 			if (typeof vnode.tag !== "string" && typeof vnode.state.onbeforeupdate === "function") {
-			forceComponentUpdate = callHook.call(vnode.state.onbeforeupdate, vnode, old)
+				var force = callHook.call(vnode.state.onbeforeupdate, vnode, old)
+				if (force !== undefined && !force) break
 			}
-		if (!(forceVnodeUpdate === undefined && forceComponentUpdate === undefined) && !forceVnodeUpdate && !forceComponentUpdate) {
+			return false
+		} while (false); // eslint-disable-line no-constant-condition
 		vnode.dom = old.dom
 		vnode.domSize = old.domSize
 		vnode.instance = old.instance
 		return true
 	}
-		return false
-	}
 
 	function render(dom, vnodes) {
 		if (!dom) throw new Error("Ensure the DOM element being passed to m.route/m.mount/m.render is not undefined.")

render/tests/test-component.js

@@ -699,13 +699,23 @@ o.spec("component", function() {
 						"onupdate", "onbeforeremove", "onremove"
 					]
 					hooks.forEach(function(hook) {
-						// the `attrs` hooks are called before  the component ones
+						if (hook === "onbeforeupdate") {
+							// the component's `onbeforeupdate` is called after the `attrs`' one
 							attrs[hook] = o.spy(function() {
-							o(attrs[hook].callCount).equals(methods[hook].callCount + 1)
+								o(attrs[hook].callCount).equals(methods[hook].callCount + 1)(hook)
 							})
 							methods[hook] = o.spy(function() {
-							o(attrs[hook].callCount).equals(methods[hook].callCount)
+								o(attrs[hook].callCount).equals(methods[hook].callCount)(hook)
 							})
+						} else {
+							// the other component hooks are called before the `attrs` ones
+							methods[hook] = o.spy(function() {
+								o(attrs[hook].callCount).equals(methods[hook].callCount - 1)(hook)
+							})
+							attrs[hook] = o.spy(function() {
+								o(attrs[hook].callCount).equals(methods[hook].callCount)(hook)
+							})
+						}
 					})
 
 					var component = createComponent(methods)

render/tests/test-onbeforeupdate.js

@@ -186,7 +186,7 @@ o.spec("onbeforeupdate", function() {
 				o(root.firstChild.attributes["id"].value).equals("b")
 			})
 
-			o("does not prevent update if returning false in component but true in vnode", function() {
+			o("prevents update if returning false in component but true in vnode", function() {
 				var component = createComponent({
 					onbeforeupdate: function() {return false},
 					view: function(vnode) {
@@ -199,10 +199,10 @@ o.spec("onbeforeupdate", function() {
 				render(root, [vnode])
 				render(root, [updated])
 
-				o(root.firstChild.attributes["id"].value).equals("b")
+				o(root.firstChild.attributes["id"].value).equals("a")
 			})
 
-			o("does not prevent update if returning true in component but false in vnode", function() {
+			o("prevents update if returning true in component but false in vnode", function() {
 				var component = createComponent({
 					onbeforeupdate: function() {return true},
 					view: function(vnode) {
@@ -215,7 +215,7 @@ o.spec("onbeforeupdate", function() {
 				render(root, [vnode])
 				render(root, [updated])
 
-				o(root.firstChild.attributes["id"].value).equals("b")
+				o(root.firstChild.attributes["id"].value).equals("a")
 			})
 
 			o("does not prevent update if returning true from component", function() {