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.

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

Fix 2128 #2130

Merged
merged 8 commits into from Apr 23, 2018
Merged

Fix 2128 #2130

merged 8 commits into from Apr 23, 2018

Conversation

pygy
Copy link
Member

@pygy pygy commented Apr 20, 2018

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

render/render.js Outdated
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)
Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

save us a vnode slot

Not sure I follow...

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -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.

@tivac
Copy link
Contributor

tivac commented Apr 21, 2018

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
Copy link
Member Author

pygy commented Apr 21, 2018

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.

@pygy
Copy link
Member Author

pygy commented Apr 21, 2018

@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.

@tivac
Copy link
Contributor

tivac commented Apr 22, 2018

I'll take a look tomorrow

@pygy
Copy link
Member Author

pygy commented Apr 22, 2018

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 pygy force-pushed the fix-2128 branch 3 times, most recently from dacefe5 to 795a41f Compare April 22, 2018 12:04
@pygy
Copy link
Member Author

pygy commented Apr 22, 2018

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

@dead-claudia
Copy link
Member

@pygy
Copy link
Member Author

pygy commented Apr 22, 2018

👍 I had missed it.

render/render.js Outdated
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
Copy link
Contributor

Choose a reason for hiding this comment

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

isfull

"is full"

Copy link
Contributor

@tivac tivac left a comment

Choose a reason for hiding this comment

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

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

@pygy pygy merged commit 44e165a into MithrilJS:next Apr 23, 2018
@pygy
Copy link
Member Author

pygy commented Apr 23, 2018

Here we go!

@JacksonJN
Copy link

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

@pygy
Copy link
Member Author

pygy commented Apr 23, 2018

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
Copy link

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

@pygy
Copy link
Member Author

pygy commented Apr 24, 2018

@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
Copy link

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

@pygy
Copy link
Member Author

pygy commented Apr 24, 2018

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

@JacksonJN
Copy link

Thanks for the info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when inserting node during render: "Failed to execute 'insertBefore' on 'Node'"
4 participants