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

remove child in an array from beginning, each child's state is not woking as expected #120

Closed
eisneim opened this issue Apr 11, 2016 · 4 comments

Comments

@eisneim
Copy link

eisneim commented Apr 11, 2016

jsfiddle https://jsfiddle.net/eisneim/rrqtpj72/1/

handleClick(){
/*
  state = {
    childData: ['a','b','c','d','e']
  }
 remove child from let to right, but vdom node not update correctly, 
 the "key" prop is not working neither
 */
  var newChildData = this.state.childData.slice(1)
  console.log(newChildData)
  this.setState({childData: newChildData })
}

it seems that the props.key is not working, and because of the "component-recycler", the state was not reseted when a child is collected, this will cause issue when child is reused, the previous state object is still there

@eisneim eisneim changed the title remove child at beginning not working remove child in an array from beginning, each child's state is not woking as expected Apr 11, 2016
@eisneim
Copy link
Author

eisneim commented Apr 11, 2016

additional to that, this will also cause componentWillMount, componentDidMount and other life cycle hook invoked on incorrect child component

@developit
Copy link
Member

@eisneim Great work, thanks for investigating this. I think we're going to need to have the component recycling invoke the constructor again when re-using a collected component:
https://github.com/developit/preact/blob/master/src/vdom/component-recycler.js#L24

This will probably cause some slight performance disadvantages, sadly, since it is now a common pattern to use class instance properties to mimic lexically scoped methods. However, I think it's best to make the recycler as transparent as possible, and this is one case where it currently isn't.

Are you able to elaborate on the lifecycle issue you mentioned in your second comment?

developit added a commit that referenced this issue Apr 12, 2016
…d fail in some cases, causing components not to be recycled. Relates to #98 and the second issue noted in #120.
developit added a commit that referenced this issue Apr 12, 2016
…oking component constructors when re-using components from the recycler.
@ouzhenkun
Copy link
Contributor

@developit @eisneim

https://jsfiddle.net/ryan_ou/uLnspcp7/2/

I wrote a jsfiddle for the lifecycle issue:
i have 3 items [ 'a', 'b', 'c' ], if i remove the first item a, this will cause componentWillUnmount and other life cycle hook(DidUnmount, DidUpdate...) invoked on incorrect child component.

ps.
add the key props for each <Item /> will fix this.

@eisneim
Copy link
Author

eisneim commented Apr 12, 2016

@zhenkunou nice work!
i would suggest adding a warning message if array of child don't have "key" prop, just like React

@eisneim eisneim closed this as completed Apr 12, 2016
developit added a commit that referenced this issue Apr 13, 2016
…not a great idea. Babel's transpiled constructors (and basically anything else) re-assign to the prototype, which is slow and a fairly bad idea. Also, and more importantly, it was assigning `this.base` to `null`, completely ignoring the main reason for caching components (caching their generated DOM using the component itself as a cache key). This commit changes the behavior to discard the cached component instance itself, re-using only the base by copying it to a fresh instance. Best of both worlds! /cc @eisneim @zhenkunou
marvinhagemeister added a commit that referenced this issue Mar 15, 2022
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

3 participants