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

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

Merged
merged 4 commits into from
Oct 17, 2016
Merged

Conversation

Boorj
Copy link
Contributor

@Boorj Boorj commented Sep 11, 2016

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.

Keys in options shouldn't depend on key index here : https://github.com/rkit/react-select2-wrapper/blob/master/src/components/Select2.js#L159
```jsx
/* 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.
@@ -156,7 +156,8 @@ export default class Select2 extends Component {
makeOption(item, k) {
if (this.isObject(item)) {
const { id, text, ...itemParams } = item;
return (<option key={`option-${k}`} value={id} {...itemParams}>{text}</option>);
const sanitized_id = id + '';
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure that this is needed.
The key will be a string in any way…

Also we have to provide an ability use several options with same IDs, which is prohibited by react, but not strictly prohibited by select.
so, best solution seems to be
```js
      const key = 'option-' + k + '-' + id;
      return (<option key={key} value={id} {...itemParams}>{text}</option>);
```
And about sanitizing: was not sure, if react could use _any_ string as a key. Had to [check](https://facebook.github.io/react/docs/reconciliation.html#keys)
So now it seems to solve the problem
@@ -156,7 +156,8 @@ export default class Select2 extends Component {
makeOption(item, k) {
if (this.isObject(item)) {
const { id, text, ...itemParams } = item;
return (<option key={`option-${k}`} value={id} {...itemParams}>{text}</option>);
const key = 'option-' + k + '-' + id;
Copy link
Owner

@rkit rkit Sep 13, 2016

Choose a reason for hiding this comment

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

Please check js style

Is it better?
Copy link
Owner

@rkit rkit left a comment

Choose a reason for hiding this comment

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

If there is a "$k" in the key, the key will always be different. Isn't it?

@Boorj
Copy link
Contributor Author

Boorj commented Oct 2, 2016

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

rkit commented Oct 4, 2016

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

Boorj commented Oct 13, 2016

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

Boorj commented Oct 13, 2016

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

rkit commented Oct 13, 2016

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

Boorj commented Oct 16, 2016

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.

that should work fine
@rkit rkit merged commit dcbf07c into rkit:master Oct 17, 2016
@rkit
Copy link
Owner

rkit commented Oct 17, 2016

I released a new version, thanks

@Boorj
Copy link
Contributor Author

Boorj commented Oct 19, 2016

Wonderful) thanks)

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.

None yet

2 participants