Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix 2128 #2130

Merged
merged 8 commits into from Apr 23, 2018
Merged

Fix 2128 #2130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6cb4b04
Disentangle keyed and unkeyed diff
pygy Apr 15, 2018
0756add
[DOM Mock] Fix insertBefore when the reference node follows the movin…
pygy Apr 20, 2018
0450129
tests for #2128
pygy Apr 20, 2018
33f1c94
[render/render] Simplify updateNodes, fix #2128
pygy Apr 20, 2018
b0652b7
tests for #2132
pygy Apr 21, 2018
761e2f0
[render/render] move keyed cached nodes when needed, fix diff semanti…
pygy Apr 21, 2018
c8b3395
Cleanup, comments, and optimize getKeyMap
pygy Apr 22, 2018
df4c34b
[render/render] Misc comment fixes
pygy Apr 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 14 additions & 12 deletions docs/change-log.md
View file
Open in desktop
Expand Up @@ -36,18 +36,20 @@
#### 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](https://github.com/MithrilJS/mithril.js/pull/2130))
- render/core: fix cached nodes behavior in some keyed diff scenarios [#2132](https://github.com/MithrilJS/mithril.js/issues/2132) ([#2130](https://github.com/MithrilJS/mithril.js/pull/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
Expand Down
177 changes: 103 additions & 74 deletions render/render.js
View file
Open in desktop
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion one way or another, but why'd this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The individual createX methods used to return a DOM node that was used in createComponent() and updateNodes() (in the map-based diff section).

This isn't needed anymore (and wasn't necessary in the first place AFAICT, since insertNode() in createComponent() was redundant and we can get v.dom in updateNodes()).

So I made it explicit that createNode() wasn't returning anything.

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) {
Expand All @@ -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()
Expand All @@ -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
Expand Down Expand Up @@ -132,7 +129,6 @@ module.exports = function($window) {
setLateAttrs(vnode)
}
}
return element
}
function initComponent(vnode, hooks) {
var sentinel
Expand All @@ -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
}
}

Expand Down Expand Up @@ -194,10 +187,8 @@ module.exports = function($window) {
//
// The updateNodes() function:
// - deals with trivial cases
// - determines whether the lists are keyed or unkeyed
// (Currently we look for the first pair of non-null nodes and deem the lists unkeyed
// if both nodes are unkeyed. TODO (v2) We may later take advantage of the fact that
// mixed diff is not supported and settle on the keyedness of the first vnode we find)
// - determines whether the lists are keyed or unkeyed based on the first non-null node
// of each list.
// - diffs them and patches the DOM if needed (that's the brunt of the code)
// - manages the leftovers: after diffing, are there:
// - old nodes left to remove?
Expand All @@ -209,13 +200,14 @@ module.exports = function($window) {

// ## Diffing
//
// There's first a simple diff for unkeyed lists of equal length.
// If one list is keyed and the other is unkeyed, the old is removed, and the new one is
// inserted (since the keys are guaranteed to differ).
//
// Then comes the main diff algorithm that is split in four parts (simplifying a bit).
// Then comes the unkeyed diff algo, and at last, the keyed diff algorithm that is split
// in four parts (simplifying a bit).
//
// The first part goes through both lists top-down as long as the nodes at each level have
// the same key. This is always true for unkeyed lists that are entirely processed by this
// step.
// the same key.
//
// The second part deals with lists reversals, and traverses one list top-down and the other
// bottom-up (as long as the keys match).
Expand All @@ -241,10 +233,25 @@ 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.
//
// 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).
// `updateNode()` and `createNode()` expect a nextSibling parameter to perform DOM operations.
// 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
// as the next sibling (cached in the `nextSibling` variable).


// ## DOM node moves
//
// In most scenarios `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). 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
Expand All @@ -256,84 +263,106 @@ module.exports = function($window) {
else if (old == null) createNodes(parent, vnodes, 0, vnodes.length, hooks, nextSibling, ns)
else if (vnodes == null) removeNodes(old, 0, old.length)
else {
var start = 0, commonLength = Math.min(old.length, vnodes.length), isUnkeyed = false
for(; start < commonLength; start++) {
if (old[start] != null && vnodes[start] != null) {
if (old[start].key == null && vnodes[start].key == null) isUnkeyed = true
// default to keyed because, when either list is full of null nodes, it has fewer branches
var start = 0, oldStart = 0, isOldKeyed = true, isKeyed = true
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
}
}
if (isUnkeyed && old.length === vnodes.length) {
for (start = 0; start < vnodes.length; start++) {
if (old[start] === vnodes[start] || old[start] == null && vnodes[start] == null) continue
else if (old[start] == null) createNode(parent, vnodes[start], hooks, ns, getNextSibling(old, start + 1, nextSibling))
else if (vnodes[start] == null) removeNodes(old, start, start + 1)
else updateNode(parent, old[start], vnodes[start], hooks, getNextSibling(old, start + 1, nextSibling), ns)
if (isOldKeyed !== isKeyed) {
removeNodes(old, oldStart, old.length)
createNodes(parent, vnodes, start, vnodes.length, hooks, nextSibling, ns)
return
}
if (!isKeyed) {
// Don't index past the end of either list (causes deopts).
var commonLength = old.length < vnodes.length ? old.length : vnodes.length
// Rewind if necessary to the first non-null index on either side.
// We could alternatively either explicitly create or remove nodes when `start !== oldStart`
// but that would be optimizing for sparse lists which are more rare than dense ones.
start = start < oldStart ? start : oldStart
for (; start < commonLength; start++) {
o = old[start]
v = vnodes[start]
if (o === v || o == null && v == null) continue
else if (o == null) createNode(parent, v, hooks, ns, getNextSibling(old, start + 1, nextSibling))
else if (v == null) removeNode(o)
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)
return
}
var oldStart = start = 0, oldEnd = old.length - 1, end = vnodes.length - 1, map, o, v
// keyed diff
var oldEnd = old.length - 1, end = vnodes.length - 1, map, o, v

while (oldEnd >= oldStart && end >= start) {
// both top-down
o = old[oldStart]
v = vnodes[start]
if (o === v || o == null && v == null) oldStart++, start++
else if (o == null) {
if (isUnkeyed || v.key == null) {
createNode(parent, vnodes[start], hooks, ns, getNextSibling(old, ++start, nextSibling))
}
oldStart++
} else if (v == null) {
if (isUnkeyed || o.key == null) {
removeNodes(old, start, start + 1)
oldStart++
}
start++
} else if (o.key === v.key) {
if (o == null) oldStart++
else if (v == null) start++
else if (o.key === v.key) {
oldStart++, start++
updateNode(parent, o, v, hooks, getNextSibling(old, oldStart, nextSibling), ns)
if (o !== v) 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++
// reversal: old top-down, new bottom-up
v = vnodes[end]
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)
if (o !== v) updateNode(parent, o, v, hooks, nextSibling, ns)
if (v.dom != null) nextSibling = v.dom
}
else break
}
}
while (oldEnd >= oldStart && end >= start) {
// both bottom-up
o = old[oldEnd]
v = vnodes[end]
if (o === v) oldEnd--, end--
else if (o == null) oldEnd--
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
if (o !== v) updateNode(parent, o, v, hooks, nextSibling, ns)
if (v.dom != null) nextSibling = v.dom
oldEnd--, end--
} else {
if (!map) map = getKeyMap(old, oldEnd)
// old map-based, new bottom-up
if (map == null) {
// the last node can be left out of the map because it will be caught by the
// bottom-up part of the diff loop. If we were to refactor this to use distinct
// loops, we'd have to pass `oldEnd + 1` (or change `start < end` to `<=` in getKeyMap).
map = getKeyMap(old, oldStart, oldEnd)
}
if (v != null) {
var oldIndex = map[v.key]
if (oldIndex != null) {
if (oldIndex == null) {
createNode(parent, v, hooks, ns, nextSibling)
if (v.dom != null) nextSibling = v.dom
} else {
o = old[oldIndex]
updateNode(parent, o, v, hooks, getNextSibling(old, oldEnd + 1, nextSibling), ns)
insertNode(parent, toFragment(v), nextSibling)
insertNode(parent, toFragment(o), nextSibling)
if (o !== v) updateNode(parent, o, v, hooks, nextSibling, ns)
o.skip = true
if (o.dom != null) nextSibling = o.dom
} else {
var dom = createNode(parent, v, hooks, ns, nextSibling)
nextSibling = dom
if (v.dom != null) nextSibling = v.dom
}
}
end--
}
if (end < start) break
}
// deal with the leftovers.
createNodes(parent, vnodes, start, end + 1, hooks, nextSibling, ns)
removeNodes(old, oldStart, oldEnd + 1)
}
Expand Down Expand Up @@ -435,13 +464,13 @@ module.exports = function($window) {
vnode.domSize = old.domSize
}
}
function getKeyMap(vnodes, end) {
var map = {}, i = 0
for (var i = 0; i < end; i++) {
var vnode = vnodes[i]
function getKeyMap(vnodes, start, end) {
var map = {}
for (; start < end; start++) {
var vnode = vnodes[start]
if (vnode != null) {
var key = vnode.key
if (key != null) map[key] = i
if (key != null) map[key] = start
}
}
return map
Expand All @@ -467,7 +496,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)
}

Expand Down