feat: update jt to add keys to react elements #237
Conversation
|
|
||
| export const addReactKeysWhereNeeded = (item, idx) => { | ||
| if (isValidElement(item) && !item.key) { | ||
| return cloneElement(item, { key: idx.toString() }); |
There was a problem hiding this comment.
https://react.dev/learn/rendering-lists do not use array index as key
There was a problem hiding this comment.
You know that react.dev wasn't even around when this PR was opened right? I don't disagree with what you're saying, and I regularly point to the same warning, but we should remember context.
Is this still an issue with ttag, I haven't used it in years? If it is, then did you have an alternative suggestion for the key? One thought that comes to mind is the actual text content of the element, but there is no way to guarantee those will be unique on the same page. You could also combine the text content with the array index.
However, I would argue that in this case there is probably nothing wrong with using the array index, as React itself uses that when no explicit key is provided. It mainly becomes an issue if you are going to sort the items, or delete/insert items, which usually isn't a concern for translation of strings for the purpose of i18n.
There was a problem hiding this comment.
No idea if this is an issue, just stumbled upon the PR and out of curiosity looked at the content. Didn't notice the PR was that old, and honestly don't have a better idea. But hey, isn't pointing at peoples misstakes the whole point with the internet!?! 😄
btw, it's been in the docs for a while https://17.reactjs.org/docs/lists-and-keys.html#keys 😜
There was a problem hiding this comment.
Yes, I've read this on the old docs too. I still feel like when it comes to translation strings which usually are not dynamic in nature, so the reordering concern doesn't really apply, an index is an acceptable key in this context.
I'm more interested in whether this is still an issue with ttag. I know when I has to use this lib for i18n due to work requirements, it was a real pain to see these errors come up in the console, it lowered the signal-to-noise ratio. Maybe I'll rebase with master and see if this is still a thing. Looks like the original issue is still open.
1 participant
Addresses #166.
Adds an optional peer dependency on
reactand usesisValidElementandcloneElementto add keys tojtexpressions that are React elements and do not already have a key defined.Chose to bump the major version because this would be a breaking change for current users of
jtwho did not previously havereactas a dependency.Now you can safely use
jtwith JSX expressions without manually adding akeyprop (which is an uncommon pattern when defining JSX outside of.map).