From 9490950c30a6a16fa660949b6aabaa86f6b77954 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre-Yves=20G=C3=A9rardy?= Date: Fri, 20 Apr 2018 21:27:22 +0200 Subject: [PATCH] [render/render] Simplify updateNodes, fix #2128 --- docs/change-log.md | 25 +++++++++--------- render/render.js | 64 ++++++++++++++++++++++++---------------------- 2 files changed, 47 insertions(+), 42 deletions(-) diff --git a/docs/change-log.md b/docs/change-log.md index efd51c4b..e0278c04 100644 --- a/docs/change-log.md +++ b/docs/change-log.md @@ -36,18 +36,19 @@ #### Bug fixes - API: `m.route.set()` causes all mount points to be redrawn ([#1592](https://github.com/MithrilJS/mithril.js/pull/1592)) -- API: Using style objects in hyperscript calls will now properly diff style properties from one render to another as opposed to re-writing all element style properties every render. -- core: `addEventListener` and `removeEventListener` are always used to manage event subscriptions, preventing external interference. -- core: Event listeners allocate less memory, swap at low cost, and are properly diffed now when rendered via `m.mount()`/`m.redraw()`. -- core: `Object.prototype` properties can no longer interfere with event listener calls. -- API: Event handlers, when set to literally `undefined` (or any non-function), are now correctly removed. -- core: `xlink:href` attributes are now correctly removed -- render: fixed an ommission that caused `oninit` to be called unnecessarily in some cases [#1992](https://github.com/MithrilJS/mithril.js/issues/1992) -- render: Render state correctly on select change event [#1916](https://github.com/MithrilJS/mithril.js/issues/1916) ([#1918](https://github.com/MithrilJS/mithril.js/pull/1918) [@robinchew](https://github.com/robinchew), [#2052](https://github.com/MithrilJS/mithril.js/pull/2052)) -- render: fix various updateNodes/removeNodes issues when the pool and fragments are involved [#1990](https://github.com/MithrilJS/mithril.js/issues/1990), [#1991](https://github.com/MithrilJS/mithril.js/issues/1991), [#2003](https://github.com/MithrilJS/mithril.js/issues/2003), [#2021](https://github.com/MithrilJS/mithril.js/pull/2021) -- render: fix element value don't change if new valor is undefined [#2082](https://github.com/MithrilJS/mithril.js/issues/2082) -- render: fix typo, missing `)` - +- render/attrs: Using style objects in hyperscript calls will now properly diff style properties from one render to another as opposed to re-writing all element style properties every render. +- render/attrs: `xlink:href` attributes are now correctly removed +- render/attrs: fix element value don't change if new value is undefined [#2082](https://github.com/MithrilJS/mithril.js/issues/2082) +(https://github.com/MithrilJS/mithril.js/pull/2130) +- render/core: Render state correctly on select change event [#1916](https://github.com/MithrilJS/mithril.js/issues/1916) ([#1918](https://github.com/MithrilJS/mithril.js/pull/1918) [@robinchew](https://github.com/robinchew), [#2052](https://github.com/MithrilJS/mithril.js/pull/2052)) +- render/core: fix various updateNodes/removeNodes issues when the pool and fragments are involved [#1990](https://github.com/MithrilJS/mithril.js/issues/1990), [#1991](https://github.com/MithrilJS/mithril.js/issues/1991), [#2003](https://github.com/MithrilJS/mithril.js/issues/2003), [#2021](https://github.com/MithrilJS/mithril.js/pull/2021) +- render/core: fix crashes when the keyed vnodes with the same `key` had different `tag` values [#2128](https://github.com/MithrilJS/mithril.js/issues/2128) [@JacksonJN](https://github.com/JacksonJN) ([#2130] +- render/events: `addEventListener` and `removeEventListener` are always used to manage event subscriptions, preventing external interference. +- render/events: Event listeners allocate less memory, swap at low cost, and are properly diffed now when rendered via `m.mount()`/`m.redraw()`. +- render/events: `Object.prototype` properties can no longer interfere with event listener calls. +- render/events: Event handlers, when set to literally `undefined` (or any non-function), are now correctly removed. +- render/hooks: fixed an ommission that caused `oninit` to be called unnecessarily in some cases [#1992](https://github.com/MithrilJS/mithril.js/issues/1992) +- docs: fix typo ([#2104](https://github.com/MithrilJS/mithril.js/pull/2104) [@mikeyb](https://github.com/mikeyb)) --- ### v1.1.7 diff --git a/render/render.js b/render/render.js index 7e005290..72dba419 100644 --- a/render/render.js +++ b/render/render.js @@ -51,18 +51,17 @@ module.exports = function($window) { vnode.state = {} if (vnode.attrs != null) initLifecycle(vnode.attrs, vnode, hooks) switch (tag) { - case "#": return createText(parent, vnode, nextSibling) - case "<": return createHTML(parent, vnode, ns, nextSibling) - case "[": return createFragment(parent, vnode, hooks, ns, nextSibling) - default: return createElement(parent, vnode, hooks, ns, nextSibling) + case "#": createText(parent, vnode, nextSibling); break + case "<": createHTML(parent, vnode, ns, nextSibling); break + case "[": createFragment(parent, vnode, hooks, ns, nextSibling); break + default: createElement(parent, vnode, hooks, ns, nextSibling) } } - else return createComponent(parent, vnode, hooks, ns, nextSibling) + else createComponent(parent, vnode, hooks, ns, nextSibling) } function createText(parent, vnode, nextSibling) { vnode.dom = $doc.createTextNode(vnode.children) insertNode(parent, vnode.dom, nextSibling) - return vnode.dom } var possibleParents = {caption: "table", thead: "table", tbody: "table", tfoot: "table", tr: "tbody", th: "tr", td: "tr", colgroup: "table", col: "colgroup"} function createHTML(parent, vnode, ns, nextSibling) { @@ -87,7 +86,6 @@ module.exports = function($window) { fragment.appendChild(child) } insertNode(parent, fragment, nextSibling) - return fragment } function createFragment(parent, vnode, hooks, ns, nextSibling) { var fragment = $doc.createDocumentFragment() @@ -98,7 +96,6 @@ module.exports = function($window) { vnode.dom = fragment.firstChild vnode.domSize = fragment.childNodes.length insertNode(parent, fragment, nextSibling) - return fragment } function createElement(parent, vnode, hooks, ns, nextSibling) { var tag = vnode.tag @@ -132,7 +129,6 @@ module.exports = function($window) { setLateAttrs(vnode) } } - return element } function initComponent(vnode, hooks) { var sentinel @@ -157,15 +153,12 @@ module.exports = function($window) { function createComponent(parent, vnode, hooks, ns, nextSibling) { initComponent(vnode, hooks) if (vnode.instance != null) { - var element = createNode(parent, vnode.instance, hooks, ns, nextSibling) + createNode(parent, vnode.instance, hooks, ns, nextSibling) vnode.dom = vnode.instance.dom vnode.domSize = vnode.dom != null ? vnode.instance.domSize : 0 - insertNode(parent, element, nextSibling) - return element } else { vnode.domSize = 0 - return $emptyFragment } } @@ -241,10 +234,20 @@ module.exports = function($window) { // parts are actually implemented as only two loops, one for the first two parts, and one for // the other two. I'm not sure it wins us anything except maybe a few bytes of file size. - // ## DOM node operations + // ## Finding the next sibling. + // + // `updateNode()` and `createNode()` expect a nextSibling parameter. When the list is being + // traversed top-down, the next sibling must be looked for in the old list using + // `getNextSibling()`. In the other scenarios (swaps, upwards traversal, map-based diff), + // the new vnodes list is traversed upwards, which enables us to fetch the `nextSibling` + // parameter on the go (set to the last `v.dom` when present). + + // ## DOM node moves // // In most cases `updateNode()` and `createNode()` perform the DOM operations. However, - // this is not the case if the node moved (second and fourth part of the diff algo). + // this is not the case if the node moved (second and fourth part of the diff algo). We move + // the old DOM nodes before updateNode runs because it enables us to use the cached `nextSibling` + // variable rather than fetching it using `getNextSibling()`. // // The fourth part of the diff currently inserts nodes unconditionally, leading to issues // like #1791 and #1999. We need to be smarter about those situations where adjascent old @@ -300,14 +303,15 @@ module.exports = function($window) { oldStart++, start++ updateNode(parent, o, v, hooks, getNextSibling(old, oldStart, nextSibling), ns) } else { - o = old[oldEnd] - if (o === v) oldEnd--, start++ - else if (o == null) oldEnd-- - else if (v == null) start++ + v = vnodes[end] + if (o === v) oldStart++, end-- + else if (o == null) oldStart++ + else if (v == null) end-- else if (o.key === v.key) { - updateNode(parent, o, v, hooks, getNextSibling(old, oldEnd + 1, nextSibling), ns) - if (start < end) insertNode(parent, toFragment(v), getNextSibling(old, oldStart, nextSibling)) - oldEnd--, start++ + oldStart++ + if (start < end--) insertNode(parent, toFragment(o), nextSibling) + updateNode(parent, o, v, hooks, nextSibling, ns) + if(v.dom != null) nextSibling = v.dom } else break } @@ -319,8 +323,8 @@ module.exports = function($window) { else if (o == null) oldEnd-- else if (v == null) end-- else if (o.key === v.key) { - updateNode(parent, o, v, hooks, getNextSibling(old, oldEnd + 1, nextSibling), ns) - if (o.dom != null) nextSibling = o.dom + updateNode(parent, o, v, hooks, nextSibling, ns) + if (v.dom != null) nextSibling = v.dom oldEnd--, end-- } else { if (!map) map = getKeyMap(old, oldEnd) @@ -328,13 +332,13 @@ module.exports = function($window) { var oldIndex = map[v.key] if (oldIndex != null) { o = old[oldIndex] - updateNode(parent, o, v, hooks, getNextSibling(old, oldEnd + 1, nextSibling), ns) - insertNode(parent, toFragment(v), nextSibling) + insertNode(parent, toFragment(o), nextSibling) + updateNode(parent, o, v, hooks, nextSibling, ns) o.skip = true - if (o.dom != null) nextSibling = o.dom + if (v.dom != null) nextSibling = v.dom } else { - var dom = createNode(parent, v, hooks, ns, nextSibling) - nextSibling = dom + createNode(parent, v, hooks, ns, nextSibling) + if (v.dom != null) nextSibling = v.dom } } end-- @@ -474,7 +478,7 @@ module.exports = function($window) { } function insertNode(parent, dom, nextSibling) { - if (nextSibling) parent.insertBefore(dom, nextSibling) + if (nextSibling != null) parent.insertBefore(dom, nextSibling) else parent.appendChild(dom) }