From a8473e63c95b3b513a067aa05497c923c72593c4 Mon Sep 17 00:00:00 2001 From: Isiah Meadows Date: Tue, 27 Nov 2018 18:02:48 -0500 Subject: [PATCH] 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 | 1 + render/render.js | 45 +++++++++++++++-------------- render/tests/test-component.js | 24 ++++++++++----- render/tests/test-onbeforeupdate.js | 8 ++--- 4 files changed, 45 insertions(+), 33 deletions(-) diff --git a/docs/change-log.md b/docs/change-log.md index 57640721..47525c41 100644 --- a/docs/change-log.md +++ b/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 diff --git a/render/render.js b/render/render.js index d197a0dd..69c74d3b 100644 --- a/render/render.js +++ b/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 @@ -255,7 +255,7 @@ module.exports = function($window) { // When the list is being traversed top-down, at any index, the DOM nodes up to the previous // vnode reflect the content of the new list, whereas the rest of the DOM nodes reflect the old // list. The next sibling must be looked for in the old list using `getNextSibling(... oldStart + 1 ...)`. - // + // // In the other scenarios (swaps, upwards traversal, map-based diff), // the new vnodes list is traversed upwards. The DOM nodes at the bottom of the list reflect the // bottom part of the new vnodes list, and we can use the `v.dom` value of the previous node @@ -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") { - var result = callHook.call(vnode.attrs.onbeforeremove, vnode) + if (typeof vnode.tag !== "string" && typeof vnode.state.onbeforeremove === "function") { + 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") { - var result = callHook.call(vnode.state.onbeforeremove, vnode) + if (vnode.attrs && typeof vnode.attrs.onbeforeremove === "function") { + 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,20 +863,21 @@ module.exports = function($window) { if (typeof source.onupdate === "function") hooks.push(callHook.bind(source.onupdate, vnode)) } function shouldNotUpdate(vnode, old) { - var forceVnodeUpdate, forceComponentUpdate - if (vnode.attrs != null && typeof vnode.attrs.onbeforeupdate === "function") { - forceVnodeUpdate = callHook.call(vnode.attrs.onbeforeupdate, vnode, old) - } - if (typeof vnode.tag !== "string" && typeof vnode.state.onbeforeupdate === "function") { - forceComponentUpdate = callHook.call(vnode.state.onbeforeupdate, vnode, old) - } - if (!(forceVnodeUpdate === undefined && forceComponentUpdate === undefined) && !forceVnodeUpdate && !forceComponentUpdate) { - vnode.dom = old.dom - vnode.domSize = old.domSize - vnode.instance = old.instance - return true - } - return false + do { + if (vnode.attrs != null && typeof vnode.attrs.onbeforeupdate === "function") { + var force = callHook.call(vnode.attrs.onbeforeupdate, vnode, old) + if (force !== undefined && !force) break + } + if (typeof vnode.tag !== "string" && typeof vnode.state.onbeforeupdate === "function") { + var force = callHook.call(vnode.state.onbeforeupdate, vnode, old) + if (force !== undefined && !force) break + } + return false + } while (false); // eslint-disable-line no-constant-condition + vnode.dom = old.dom + vnode.domSize = old.domSize + vnode.instance = old.instance + return true } function render(dom, vnodes) { diff --git a/render/tests/test-component.js b/render/tests/test-component.js index dd27ef10..29c04a44 100644 --- a/render/tests/test-component.js +++ b/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 - attrs[hook] = o.spy(function() { - o(attrs[hook].callCount).equals(methods[hook].callCount + 1) - }) - methods[hook] = o.spy(function() { - o(attrs[hook].callCount).equals(methods[hook].callCount) - }) + 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)(hook) + }) + methods[hook] = o.spy(function() { + 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) diff --git a/render/tests/test-onbeforeupdate.js b/render/tests/test-onbeforeupdate.js index d0b1ecab..9ff828b6 100644 --- a/render/tests/test-onbeforeupdate.js +++ b/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() {