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

onremove fails with nested component children when the view return false #1921

Closed
octavore opened this Issue Jul 31, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@octavore
Copy link

octavore commented Jul 31, 2017

When a component receives class components as children but returns nothing in its view, onremove will try to call onremove on the children but fails because the children were never initialized. This does not happen if the component returns a vnode because in this case there is a check for vnode.instance that avoids the loop.

Expected Behavior

When navigating, onremove should not try to invoke the onremove handler of undefined _state.

Current Behavior

Appears to silently fail to change the dom.

Possible Solution

Maybe add a check that vnode._state is defined here?

Steps to Reproduce

// component which fails to be removed
class GrandchildComponent {
  view() {
    return m('div', 'hello')
  }
}

// if this component returns a vnode, the bug does not happen
class ChildComponent {
  view(v) {
    return;
  }
}

// setup for testing: click the link to trigger navigation, which fails due to the bug
class ParentComponent {
  view() {
    return [
      m('div', m('a', { href: '/b', oncreate: m.route.link }, 'next')),
      m(ChildComponent, {}, m('div', m(GrandchildComponent)))
    ]
  }
}

var root = document.getElementById("app");
m.route(root, '/', {
  '/': ParentComponent,
  '/b': { view: () => m('div', 'world')}
})

Context

The situation where I encountered this bug was with a Popover component where the parent could specify the visibility (as a boolean attribute) and children of the Popover, and if the visibility was false the view would simply return nothing. However, child components which were classes would subsequently fail onremove when navigating to a different URL.

Your Environment

  • Version used: 1.1.3 and 1da69da
  • Browser Name and version: Safari 10.1.1, Chrome 60
  • Operating System and version (desktop or mobile): MacOS 10.12.5
  • Link to your project: https://github.com/ketchuphq/ketchup

@pygy pygy added the bug label Jul 31, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Jul 31, 2017

@octavore thanks for the report, you're right, we should not iterate over the children of components, not matter what the view returns

@pygy pygy changed the title onremove fails with nested class component children onremove fails with nested component children when the view return false Jul 31, 2017

@pygy pygy referenced this issue Jul 31, 2017

Merged

Fix #1921 #1922

6 of 10 tasks complete

@pygy pygy closed this in c96e085 Jul 31, 2017

pygy added a commit that referenced this issue Jul 31, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Jul 31, 2017

It was merged into next ideally it should also be cherry picked for a v1.1.x release

pygy added a commit to pygy/mithril.js that referenced this issue Jul 31, 2017

pygy added a commit that referenced this issue Jul 31, 2017

Merge pull request #1923 from pygy/fix-change-log
Move the #1921/#1922 line into the v1.1.4 change set

pygy added a commit that referenced this issue Nov 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment