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

Allow suppressing the warning for missing keys. #14374

Closed
flying-sheep opened this issue Dec 2, 2018 · 12 comments
Closed

Allow suppressing the warning for missing keys. #14374

flying-sheep opened this issue Dec 2, 2018 · 12 comments

Comments

@flying-sheep
Copy link

flying-sheep commented Dec 2, 2018

There are valid use cases for not wanting to use keys, if you e.g. render strongly differing trees (like e.g. blog posts) or have a function that returns always the same array of items.

When you don’t provide an explicit key, React will internally use the index as key and emit a warning – however you would want to silence the warning, as using the index here is perfectly valid.

This doesn’t currently seem to be possible however, which has the sad consequence that you either have to make your code more complex for no reason (manually adding useless keys) or you have to live with the warnings (which means that valid warnings might be lost among the noise).

The article quoted in the docs states the conditions in which using the default behavior is valid:

  1. the list and items are static–they are not computed and do not change;
  2. the items in the list have no ids;
  3. the list is never reordered or filtered.
@aweary
Copy link
Contributor

aweary commented Dec 10, 2018

using the index here is perfectly valid.

In that case why not just add the index as the key? I get that it's technically not required, but the fact that React uses the index as a fallback internally is an implementation detail that you shouldn't rely on. If the index is a valid key for the item, you should make that explicit.

Explicitly using the index in cases where its valid also makes it easier to refactor if, for some reason, the index isn't a valid key anymore. You used blog posts as an example. What if one day you added the ability to sort posts by date or title? In that case the index is no longer a valid key.

With an explicit key prop it's easy to identify this (especially for those who don't know that the index is used as a fallback internally) and refactor. If the warning was just suppressed it seems more likely that you'd miss this and introduce some tricky bugs.

This doesn’t currently seem to be possible however, which has the sad consequence that you either have to make your code more complex for no reason (manually adding useless keys)

What would be your proposed API for suppressing the warning? There's precedence with the supressContentEditableWarning prop, but having supressKeyWarning seems just as verbose and using the index as the key.

@flying-sheep
Copy link
Author

flying-sheep commented Dec 10, 2018

The semantics aren’t an implementation detail. The semantics are “If no key is specified, identity is defined by the order of the elements”, which is exactly the same as “…by the index into the array of elements”. Also the docs explicitly say “If you choose not to assign an explicit key to list items then React will default to using indexes as keys” which can therefore be considered stable behavior.

I think <parent suppressKeyWarning>{[<item/>, <item/>, …]}</parent> would be great. It’s also more explicit than silencing linters (that want us to not use indices as keys 🙄) and we pass around less data than by doing <parent>{[<item key={0}/>, <item key={1}/>, …]}</parent>

You used blog posts as an example.

Apologies – I meant the content of blog posts, if you e.g. convert markdown to a virtual DOM tree. Then you can e.g. use React to make the posts morph into each other via CSS transitions, but of course the posts are completely independent and similarities only arise through the structure (e.g. almost all of them start with a title and then paragraphs)

@aweary
Copy link
Contributor

aweary commented Dec 10, 2018

Also the docs explicitly say “If you choose not to assign an explicit key to list items then React will default to using indexes as keys” which can therefore be considered stable behavior.

Sorry, I missed that part. That would be considered stable behavior, though it's still implicit and the docs also state:

When you don’t have stable IDs for rendered items, you may use the item index as a key as a last resort:

Which implies that you should be explicitly using the index in those cases. We probably need to fix the documentation, because the messaging is somewhat confusing here.

I think <parent suppressKeyWarning>{[<item/>, <item/>, …]}</parent> would be great

Silencing the warning on the parent is problematic as you can have multiple lists with different key requirements.

<div suppressKeyWarning>
   {/* No keys required here... */}
   {[<div>first</div>]}
    {/* But keys are required here, and the warning was suppressed! */}
    {this.state.items.map(item => <div>{item}</div>}
</div>

we pass around less data than by doing <parent>{[<item key={0}/>, <item key={1}/>, …]}</parent>

It's less data than adding a key to every item in the array separately, like this example. In most cases you'd be mapping through a set of items

<parent>{items.map(data => <item {...data} />}</parent>

In which case you'd be declaring the key once in the source

<parent>{items.map((data, i) => <item key={i} {...data} />}</parent>

If you have a static list of items where you are explicitly adding key to every item you should use Fragments or remove the wrapping array all together when possible.

@flying-sheep
Copy link
Author

flying-sheep commented Dec 11, 2018

Which implies that you should be explicitly using the index in those cases.

And this is how I understand it, but I would prefer to use the simplest possible code for the simplest possible case. Why do something manually, noising up my code, if it’s done automatically anyway?

Silencing the warning on the parent is problematic as you can have multiple lists with different key requirements.

You’re right. Thinking of it, the property “keys are not needed” pertains to the array. So maybe it should be a function that adds a custom property to an array that marks it as identity safe?

<div>
    {/* No keys necessary */}
    {React.suppressKeyWarning([<div>first</div>])}
    {/* Warning thrown here */}
    {this.state.items.map(item => <div>{item}</div>}
</div>

Of course it could also be named nokeys or indexIsKey or whatever 😄 I’d give it a nice alias and wrap all those arrays.

It's less data than adding a key to every item in the array separately, like this example. In most cases you'd be mapping through a set of items

Exactly. And my code gets a bit more complex (I now have to use and pass on the index), and every item ends up with a useless key property that I’ll see in the debugger. That’s what I meant with data.

@hamlim
Copy link
Contributor

hamlim commented Dec 11, 2018

This seems like it is already supported with the current React API:

render(
  <ul>
    {React.Children.toArray([1, 2, 3].map(item => <li>{item}</li>))}
  </ul>
);

React.Children.toArray will already implicitly assign a key to each child it's provided: https://reactjs.org/docs/react-api.html#reactchildrentoarray.

@flying-sheep
Copy link
Author

flying-sheep commented Dec 11, 2018

Thank you! If it doesn’t do too much unnecessary work and impacts performance, that’s almost what we’re searching for. Of course not actually adding the keys but simply marking the array as “no keys necessary” would be better for performance.

If there’s no performance hit and this information would end up being referenced in the keys section of the docs, I’d be happy.

@aweary
Copy link
Contributor

aweary commented Dec 11, 2018

@hamlim @flying-sheep we've discussed this before and concluded that toArray not warning for missing keys is a bug, see #11549. So don't rely on that behavior.

Why do something manually, noising up my code, if it’s done automatically anyway?

For the reasons mentioned above, I think relying on implicit keys is actually more complex than explicitly using keys because it relies on behavior that isn't obvious.

Exactly. And my code gets a bit more complex (I now have to use and pass on the index), and every item ends up with a useless key property that I’ll see in the debugger. That’s what I meant with data.

I would argue that a new API like React.supressKeyWarning introduces more complexity than adding a key, but feel free to open an RFC for the idea! It's probably better to call it something like React.StaticList so it's clear it's not just suppressing a warning, but removing the requirements for keys altogether.

@hamlim
Copy link
Contributor

hamlim commented Dec 12, 2018

Interesting, should the docs be updated to not mention the fact that the elements in toArray get assigned keys since it should be considered an implementation detail and not necessary "stable" for this use case?

@jdfm
Copy link

jdfm commented Jan 17, 2019

I was going to post this on #2816, but I noticed this open issue, so, here it goes...

What if we use a structure like:

import React from 'react'

const myData = [
  Something,
  Or,
  Whatever
]

const ListItems =({ items }) => Object.freeze(
  items.map(
    item => <li>{item}</li>
  )
)

const List = () => <ul><ListItems items={myData} /></ul>

In my head, ListItems can satisfy a number of these properties:

  • it can change between builds of the application (codebase changes over time)
  • it may use dynamic contents to be created the first time, but it's never mutated dynamically during runtime from that point on
  • it can be arbitrarily large, so, you might not want to keep all of the contents inline with the component that will contain it, which may look like a dynamic array when transpiled, but is in fact static

By Object.freezeing it you should be able to key off some JS properties to determine whether an array can in fact be used without the key prop on the elements inside of the array.

@gaearon
Copy link
Collaborator

gaearon commented Feb 8, 2019

Technically you can always do what JSX does — createElement(type, props, ...children). We won't warn for missing keys on those children.

Of course it's error-prone if children are dynamic so we don't advertise this.

@gaearon gaearon closed this as completed Feb 8, 2019
@flying-sheep
Copy link
Author

So basically

const StaticList = (...children) => React.createElement(React.Fragment, {}, ...children)

@geekbleek
Copy link

For those interested, a solution that worked for me is posted here: resend/react-email#1150 (comment)

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

No branches or pull requests

6 participants