rkit / react-select2-wrapper

Wrapper for Select2
MIT License
163 stars 97 forks source link

Option elements shouldn't have keys, based on index #48

Closed Boorj closed 8 years ago

Boorj commented 8 years ago

Keys in <option/> ekenebts shouldn't depend on key index here : https://github.com/rkit/react-select2-wrapper/blob/master/src/components/Select2.js#L159

/* This */
return (<option key={`option-${k}`} value={id} {...itemParams}>{text}</option>);
/* should be changed to this: */
return (<option key={`option-${id}`} value={id} {...itemParams}>{text}</option>);

Otherwise it doesn't re-render options, when options list is changed.

Also, probably <optgroup> keys could have been made of their children's keys, but that seemed overhead to me.

Boorj commented 8 years ago

No, i have a problem with this. And solved that exactly the way how it's described in PR. In react docs it's told that you better use id instead of index for child.

So i have async app, and it loads one list (of users) when it loads page, then it performs a request, and puts the response to select2. So list is changed. But react doesn't want to re-render list item, thinking that in child with key="2" nothing has changed. So changing that to key="2-user123" we make keys in proper way.

rkit commented 8 years ago

Sorry, but I still not understand you.

In react docs it's told that you better use id instead of index for child

Yes, I also suggest to useid instead of key + id Keys should be predictable, and unique. According to this PR, if add an element to the beginning of the list, all keys will be changed, as they unstable.

https://facebook.github.io/react/docs/reconciliation.html

Keys should be stable, predictable, and unique. Unstable keys (like those produced by Math.random()) will cause many nodes to be unnecessarily re-created, which can cause performance degradation and lost state in child components.

Boorj commented 8 years ago

Indeed, but if you reorder the list in model - dropdown also should be re-rendered. key part is responsible for reordering in model. id part is responsible for adding/removing and normal work in general. So that's why id+key solution is not the same as

(like those produced by Math.random())

and totally based on model and desired representation, not on "unstable" values.

Boorj commented 8 years ago

So, do you think we don't need to re-render dropdown, if the order is changed? How reactjs will react on that, if we just add an element to the beginning? Will it re-draw everything properly ?

rkit commented 8 years ago

if the order is changed? if we just add an element to the beginning?

The dropdown will be changed.

Will it re-draw everything properly ?

Yes, this code works right:

// use only `id`
return (<option key={`option-${id}`} value={id} {...itemParams}>{text}</option>);

// initial state
this.state = {
  data: [
    { text: 'bug', id: 1 },
    { text: 'feature', id: 2 },
    { text: 'documents', id: 3 },
  ],
};

// in render()
<Select2 data={this.state.data} />
<button onClick={() => this.setState({
  data: [
    { text: 'bug', id: 1 },
    { text: 'documents', id: 3 },
    { text: 'feature', id: 2 },
  ],
})}
>
  update
</button>

if we also add an element to the beginning, it will re-render. Everything is fine. Can you show full example code for reproduce your case?

Boorj commented 8 years ago

I think if we don't need to re-render all nodes of dropdown if order is changed and if your example works fine, we can leave option-${id} and it will be fine.

rkit commented 8 years ago

I released a new version, thanks

Boorj commented 8 years ago

Wonderful) thanks)