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

Component ignores the "key" attribute when it is the only child of another Component #254

Closed
gnaeus opened this issue Aug 3, 2016 · 6 comments

Comments

@gnaeus
Copy link

gnaeus commented Aug 3, 2016

When a component returns in render() methon only one of its chilren, for example:

class Router extends Component {
  render({ url, children }) {
    return children.find(c => c.attributes.path === url);
  }
}

it ignores the key attributes from component's children. And then children of same type are patched in place by virtual dom algorithm instead of mounting / unmounting. Like if there are no key attributes at all.
I now that Preact recycles DOM nodes and even components instead of render them again. But if we modify code like that:

class Router extends Component {
  render({ url, children }) {
    return (
      <div>
        { children.find(c => c.attributes.path === url) }
      </div>
    );
  }
}

then everethyng works fine and components are unmounted. Fore details please see JSFiddle

And it seems that in React this bug is not present. See JSFiddle

Why it is matter for me? Because I want to port my application from KnockoutJS to Preact.
Also I need some routing setup. And now I end up with followig code:

import { h, Component, render } from "preact";
import { Router } from "preact-router";

class KnockoutPage extends Component {
  shouldComponentUpdate = () => false;
  componentDidMount() {
    // initialize Knockout component
  }
  componentWillReceiveProps({ url, matches }}) {
    // transfer props to Knockout component
  }
  componentWillUnmount() {
    // dispose Knockout component
  }
  render({ name }) {
    let binding = `component: { name: '${ name }', params: $rawData }`
    return <div data-bind={ binding }></div>
  }
}

route((
  <Router>
    <KnockoutPage key="1" path="/foo" name="foo-page-component" />
    <KnockoutPage key="2" path="/bar" name="bar-page-component" />
  </Router>
), document.body);

P.S. Sorry for my poor English

@gnaeus
Copy link
Author

gnaeus commented Aug 3, 2016

A possible fix in my PR #255
Please review it because it breaks CI. And I'm not sure that it will not break something else.

@developit
Copy link
Member

@gnaeus Great catch! This is a good use of keys, it seems I have a need for this myself.

It looks like the CI breakage in #255 is just saucelabs failing to connect - the tests are actually passing just fine 👍

If you don't mind, I'm going to convert your JSFiddle into a test for the PR when I merge. I will try to get this done tonight 😃

@gnaeus
Copy link
Author

gnaeus commented Aug 3, 2016

Ok, thanks a lot!

@developit developit added this to the 5.7 milestone Aug 3, 2016
@developit developit modified the milestones: 5.8, 5.7 Aug 22, 2016
@developit
Copy link
Member

@gnaeus I think the SSR fix in 6 bypassed this change. Re-opening to look into it.

@developit developit reopened this Sep 7, 2016
sskoopa added a commit to sskoopa/preact that referenced this issue Nov 4, 2016
@developit developit modified the milestones: 7.1.0, 5.8 Nov 28, 2016
@developit
Copy link
Member

Merged for release in 7.1.0 :D

@developit
Copy link
Member

developit commented Dec 2, 2016

Released the fix! http://jsfiddle.net/developit/zckmzj51/5/

Sorry about the long wait on this one @gnaeus and thanks for your patience!

developit pushed a commit that referenced this issue Dec 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants