Fix 2128 #2130

pygy · 2018-04-20T21:42:12Z

This fixes #2128, a DOM mocks bug that I discovered while fixing #2128, and removes quite a bit of code.

Change log item coming as soon as I have the PR number.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation change

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
I have updated docs/change-log.md

pygy · 2018-04-20T22:13:09Z

render/render.js

-							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)


Just below, replacing o.skip = true with old[oldIndex] = null works as well, and it would save us a vnode slot.

Is there a reason not to mutate the old children list?

save us a vnode slot

Not sure I follow...

Sorry, that wasn't clear. If we do that, we can remove the .skip field on vnodes, improving their memory footprint.

tivac · 2018-04-21T05:41:57Z

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)


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

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.

tivac · 2018-04-21T05:44:28Z

This seems... reasonable. This is definitely the parts of mithril I understand the least though so I'm hesitant to actually post a review.

pygy · 2018-04-21T06:46:17Z

The main change here is the simplification to get the nextSibling.

When processing both lists downwards (unkeyed diff, first part of the keyed diff), the bottom part of the list hasn't been updated yet, and we need to look for the nextSibling in the old list using getNextSibling().

When processing the new list upwards (after the patch, the other three sections of the keyed list algo), the DOM nodes below end have already been updated, and we can set nextSibling to v.dom if it isn't null.

I've changed the list reversal part of the diff so that it processes the old list top-down and the new list bottom up to take advantage of that.

I've also moved the insertNode() calls before updateNode() where the nodes actually move, to take advantage of the cached nextSibling.

All in all, It saves us many getNextSibling() calls. I should probably update the comment about this to make it more clear.

…g child in a nodeList

pygy · 2018-04-21T07:36:33Z

@tivac I just pushed another commit that fully disentangles keyed and unkeyed diff.

With that, the pool stuff removed and the bug fixes, updateNodes() is at its simplest ever (the parts that I had a hard time understanding were actually buggy). It may be a good time to re-read it top-down.

…cs for cached nodes, fix MithrilJS#2132

tivac · 2018-04-22T07:30:48Z

I'll take a look tomorrow

pygy · 2018-04-22T11:08:56Z

Ok, I've cleaned it up some more, and added some comments :-)

Edit: some more... I've also updated the large comment that explains updateNodes() in plain English.

pygy · 2018-04-22T17:44:06Z

@isiahmeadows do you want to be removed from the owners file as well (that was an automatic review request)?

dead-claudia · 2018-04-22T22:30:54Z

@pygy I already took care of that.

pygy · 2018-04-22T22:43:35Z

👍 I had missed it.

tivac · 2018-04-23T06:10:51Z

render/render.js

-			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 isfull of null nodes, it has fewer branches


isfull

"is full"

tivac

Still not convinced I 100% understand this, but the logic all seems reasonable to me!

pygy · 2018-04-23T11:52:14Z

Here we go!

JacksonJN · 2018-04-23T14:15:40Z

Thanks @pygy. Any updates on when these changes and the other fixes to updateNodes can be released?

pygy · 2018-04-23T20:34:44Z

I think most of the updateNodes fixes could be backported, I'll have to review the patches (I'm quite busy at the moment though so I can't promise it'll be quick).

JacksonJN · 2018-04-23T21:22:33Z

I appreciate it, let me know if there is something I can do to help

pygy · 2018-04-24T16:06:26Z

@JacksonJN actually there are some changes in the next branch that are backwards incompatible (state/hooks-related), and disentangling them is more trouble than I can deal with now, so this will go in v2.

JacksonJN · 2018-04-24T16:27:29Z

@pygy Is v2 currently planned or is there an ETA?

pygy · 2018-04-24T20:17:54Z

We're actively working on it (there was a hiatus).

JacksonJN · 2018-04-24T20:52:09Z

Thanks for the info.

pygy requested a review from dead-claudia as a code owner April 20, 2018 21:42

pygy mentioned this pull request Apr 20, 2018

Error when inserting node during render: "Failed to execute 'insertBefore' on 'Node'" #2128

Closed

pygy force-pushed the fix-2128 branch from ef9dc28 to 27a9f52 Compare April 20, 2018 22:06

pygy requested a review from tivac as a code owner April 20, 2018 22:06

pygy commented Apr 20, 2018

View reviewed changes

pygy force-pushed the fix-2128 branch from 27a9f52 to 24f28df Compare April 20, 2018 22:19

tivac reviewed Apr 21, 2018

View reviewed changes

pygy added 4 commits April 21, 2018 09:23

Disentangle keyed and unkeyed diff

6cb4b04

[DOM Mock] Fix insertBefore when the reference node follows the movin…

0756add

…g child in a nodeList

tests for MithrilJS#2128

0450129

[render/render] Simplify updateNodes, fix MithrilJS#2128

33f1c94

pygy force-pushed the fix-2128 branch from 24f28df to 33f1c94 Compare April 21, 2018 07:27

pygy mentioned this pull request Apr 21, 2018

Cached nodes are not handled properly in some branches of the keyed diff algo. #2132

Closed

pygy added 2 commits April 21, 2018 22:15

tests for MithrilJS#2132

b0652b7

[render/render] move keyed cached nodes when needed, fix diff semanti…

761e2f0

…cs for cached nodes, fix MithrilJS#2132

pygy force-pushed the fix-2128 branch from df76451 to 761e2f0 Compare April 21, 2018 20:28

pygy force-pushed the fix-2128 branch 3 times, most recently from dacefe5 to 795a41f Compare April 22, 2018 12:04

Cleanup, comments, and optimize getKeyMap

c8b3395

pygy force-pushed the fix-2128 branch from 795a41f to c8b3395 Compare April 22, 2018 13:56

dead-claudia removed their request for review April 22, 2018 16:54

tivac reviewed Apr 23, 2018

View reviewed changes

tivac approved these changes Apr 23, 2018

View reviewed changes

[render/render] Misc comment fixes

df4c34b

pygy merged commit 44e165a into MithrilJS:next Apr 23, 2018

dead-claudia added the backport to v1 label Sep 1, 2018

dead-claudia mentioned this pull request Oct 9, 2018

Backport all the non-breaking bug fixes from v2 into v1.2 #2243

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 2128 #2130

Fix 2128 #2130

pygy commented Apr 20, 2018 •

edited

Loading

pygy Apr 20, 2018

tivac Apr 21, 2018

pygy Apr 21, 2018

tivac Apr 21, 2018

pygy Apr 21, 2018

tivac commented Apr 21, 2018

pygy commented Apr 21, 2018 •

edited

Loading

pygy commented Apr 21, 2018 •

edited

Loading

tivac commented Apr 22, 2018

pygy commented Apr 22, 2018 •

edited

Loading

pygy commented Apr 22, 2018

dead-claudia commented Apr 22, 2018

pygy commented Apr 22, 2018

tivac Apr 23, 2018

tivac left a comment

pygy commented Apr 23, 2018

JacksonJN commented Apr 23, 2018

pygy commented Apr 23, 2018

JacksonJN commented Apr 23, 2018

pygy commented Apr 24, 2018

JacksonJN commented Apr 24, 2018

pygy commented Apr 24, 2018

JacksonJN commented Apr 24, 2018

Fix 2128 #2130

Fix 2128 #2130

Conversation

pygy commented Apr 20, 2018 • edited Loading

Types of changes

Checklist:

pygy Apr 20, 2018

Choose a reason for hiding this comment

tivac Apr 21, 2018

Choose a reason for hiding this comment

pygy Apr 21, 2018

Choose a reason for hiding this comment

tivac Apr 21, 2018

Choose a reason for hiding this comment

pygy Apr 21, 2018

Choose a reason for hiding this comment

tivac commented Apr 21, 2018

pygy commented Apr 21, 2018 • edited Loading

pygy commented Apr 21, 2018 • edited Loading

tivac commented Apr 22, 2018

pygy commented Apr 22, 2018 • edited Loading

pygy commented Apr 22, 2018

dead-claudia commented Apr 22, 2018

pygy commented Apr 22, 2018

tivac Apr 23, 2018

Choose a reason for hiding this comment

tivac left a comment

Choose a reason for hiding this comment

pygy commented Apr 23, 2018

JacksonJN commented Apr 23, 2018

pygy commented Apr 23, 2018

JacksonJN commented Apr 23, 2018

pygy commented Apr 24, 2018

JacksonJN commented Apr 24, 2018

pygy commented Apr 24, 2018

JacksonJN commented Apr 24, 2018

pygy commented Apr 20, 2018 •

edited

Loading

pygy commented Apr 21, 2018 •

edited

Loading

pygy commented Apr 21, 2018 •

edited

Loading

pygy commented Apr 22, 2018 •

edited

Loading