Using Index as a key is an anti-pattern in React #4495
Conversation
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit dcf911b:
|
|
Looks good and makes sense to me though I know this exists in a few other places in Select.js |
| @@ -1463,7 +1463,7 @@ export default class Select extends Component<Props, State> { | |||
| }} | |||
| isFocused={isOptionFocused} | |||
| isDisabled={isDisabled} | |||
| key={`${this.getOptionValue(opt)}${index}`} | |||
| key={this.getOptionValue(opt)} | |||
There was a problem hiding this comment.
For reference, this was added to fix #4154.
|
@Rall3n , maybe additional prop ? duplicateValues or maybe pass func for build key ? |
| /> | ||
| )) | ||
| selectValue.map(opt => { | ||
| const value = this.getOptionValue(opt); |
There was a problem hiding this comment.
This value might not be unique at all for key.
Make this customizable by user instead by some callback, like this.getReactKey(opt,i), because I as a user know how to specify unique id for my data.
There was a problem hiding this comment.
@Rall3n Is "value" of option required to be unique or not ? I assume it should be ... but I found no information about that in docs.
There was a problem hiding this comment.
@NullPainter2 it's likely assumed, but not sure that it's a stated requirement. Perhaps something to consider. I wonder if something like ${label}_${value} would be acceptable as I can't think of a compelling reason why or how a user would have duplicate derived value and label values. Perhaps in different option groups, but they would have a different parent node so the keys I believe wouldn't need to be unique.
|
Greetings @vadimka123 , Thank you for putting this together. We discussed this with Jed and the rest of the team, and this is something we would certainly like to address, but given that there is no requirement for values to be unique, this could potentially be a breaking change. We would likely would want a more complete solution perhaps as @NullPainter2 had suggested with a new built-in prop for more advanced use cases like this. We want to address this, but are currently in the middle of a total rewrite to TypeScript. This will be something that we will revisit once it's complete, |
…es-as-a-key-is-an-anti-pattern
|
One issue got raised today around this I think. |
|
@vadimka123 Can you solve it, please? |
|
@vadimka123 what? |
Fixed issue when on D&D new MultiValue component rendered instead of update exists components
Also in general it's anti-pattern