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

Don't warn on undefined key when using arrays #2816

Closed
mlmorg opened this issue Jan 7, 2015 · 22 comments
Closed

Don't warn on undefined key when using arrays #2816

mlmorg opened this issue Jan 7, 2015 · 22 comments

Comments

@mlmorg
Copy link

mlmorg commented Jan 7, 2015

Using React.createElement('div', null, someArrayOfChildren) will always result in a warning when in development mode. I understand the reasoning when using JSX as you are more aware when the user may be doing something wrong. When using the javascript api, however, it seems odd because arrays are useful to work with and can be more performant than dealing with arguments.

Would it be possible to provide the ability to turn off this warning?

@gaearon
Copy link
Collaborator

gaearon commented Jan 7, 2015

I don't think this has anything to do with JS / JSX differences. You can use arrays in both.

But if you don't specify the keys, React won't be able to efficiently tell if you moved an item or changed it so this may result in wasted time updating.

So, whether in JS or JSX, specifying key on each child will help React update DOM more efficiently. To specify key in plan JS, just pass it to children's createElement as if it were a prop.

@mlmorg
Copy link
Author

mlmorg commented Jan 7, 2015

This error doesn't show if you use arguments instead of arrays:

React.createElement('div', null, oneChild, TwoChild, RedChild, BlueChild)

But there's nothing to say I couldn't put those in an array. The only reason I see for the warning in one case and not the other is because when you use JSX an array means you're printing out a list of items and it's more likely that is something that should be keyed instead of simply basic child DOM elements.

@gaearon
Copy link
Collaborator

gaearon commented Jan 7, 2015

When you use arguments, React implicitly uses their order as keys because it is unlikely their order could change with elements staying the same.

In JSX, this just corresponds to having multiple explicitly defined children. In this case it would be silly having to specify keys.

I guess this was what you said in last comment—I just can't see any reason why there should be a difference in plain JS and JSX behavior.

@mlmorg
Copy link
Author

mlmorg commented Jan 7, 2015

The following examples are exactly the same but one produces a warning while the other does not:

React.createElement('div', null, someArrayOfChildren); // logs warning
React.createElement.apply(React, ['div', null].concat(someArrayOfChildren)); // no warning

I understand why React has chosen to warn when using arrays — because arrays might mean that the order may change — but this is obviously not always the case and probably less likely when working with the javascript api. Arrays are a useful structure to work with and are much more performant to pass around than concatenating and applying arguments (like in the example above). So, why not provide the ability to simply turn off this warning than always assume that an engineer using arrays without keys is doing something wrong?

@gaearon
Copy link
Collaborator

gaearon commented Jan 7, 2015

probably less likely when working with the javascript api

I wouldn't be so sure, IMO maps would be just as common in plain JS code. It would be a shame to miss these cases just because somebody doesn't use JSX.

On the other hand, you do have a way to suppress this warning if you're just passing stuff around, and you found it yourself: use arguments.

You can spread the array for cleaner syntax (assuming you use an ES6 transpiler):

React.createElement('div', null, ...someArrayOfChildren); // no warning

@syranide
Copy link
Contributor

syranide commented Jan 7, 2015

@mlmorg While it's off-topic, createElement is primarily intended for JSX AFAIK, using React.DOM.* and React.createFactory exists for the non-JSX crowd.

However, I'm curious if you may be doing it "wrong" in a sense if you're seeing this issue a lot. Dynamically constructing a non-keyed array of items shouldn't really be done, the implicit key system used by React work best when all elements have fixed indices. I.e. it's better to construct React.createElement('div', null, X ? A : null, X ? null : B) than React.createElement('div', null, A) or React.createElement('div', null, B), maintaing the holes like that helps the reconciliation without having to be overzealous with keying.

Again, off-topic and probably doesn't apply to you... but now you know if you didn't. :)

What might interest you though is using React.createElement('div', {children: [...]}) instead in places where this is an issue... I believe doing that is still "officially OK", and would give you the result you want.

@gaearon
Copy link
Collaborator

gaearon commented Jan 7, 2015

I believe doing that is still "officially OK", and would give you the result you want.

#2809 (comment)

Oh wait, you're not talking about objects, pardon.

@sebmarkbage
Copy link
Collaborator

It's a heuristic. People gets the keys wrong all the time, even with the warning in place. You're very likely to forget a key on a dynamic array.

Don't think about the implicit key as the default and we ask you to opt-in to keys. You should really always provide an explicit key because even with a static set you might switch between two different static sets. (We should really be warning on children and any other property too.)

The extra children arguments is really a special cased way to avoid providing implicit keys by indicating that it's a static set of items.

You can certainly trick the system by using .apply(...) or children: [...]. However, it's more idiomatic to use a static notation for var args (a, b, c) which would clearly indicate that this is not a dynamic set. The spread operator makes it a lot easier to use arrays as rest arguments which is kind of unfortunate.

At the end of the day it's not there to prevent you from shooting yourself in the foot. It's just a heuristic that we can use to avoid a common source of errors. If we can come up with a foolproof system or different heuristic, by all means. Let's add that instead.

I'm closing this out since there is currently a way to avoid this as @syranide pointed out. I would not recommend making it a common practice though, since lack of keys can cause severe bugs.

@theREALdebater
Copy link

This issue needs to be re-opened and taken seriously. I think it is a major deficiency in React! There are cases where the elements of an array cannot sensibly be assessed for re-rendering individually. This occurs when a change of props or state is more likely to cause elements of the array to be inserted, omitted, or re-ordered (instead of just being changed in place). In these cases, it is more appropriate to simply regenerate the whole array on each update, and compare it to the corresponding sub-tree in the DOM: if there is a difference, change the whole sub-tree. Of course this is to be avoided whenever possible, but sometimes it just cannot be sensibly avoided. In these cases, it makes no sense to have (or require or expect) keys for the elements of the array. My recommendation is that, assuming the array will be wrapped in some outer element, allow a prop such as 'atomic' (bool) on the outer element to indicate whole element - children and all - must be updated as a whole (if any part of the sub-tree has changed), and by implication the children do not require keys. Often many of the elements of the array will be just text; it is a major pain to have to wrap each text item in an element just to put a key on it (and the keys are, in reality, arbitrary and useless). The facility I recommend would allow React to optimise such sequences of text items, by concatenating them before putting them into the DOM (as a single text node).

@gaearon
Copy link
Collaborator

gaearon commented Nov 13, 2017

I’m not sure I understand what you’re saying.
An example would be helpful.

This occurs when a change of props or state is more likely to cause elements of the array to be inserted, omitted, or re-ordered (instead of just being changed in place).

This is exactly the case where specifying keys is important. Keys let React figure out what was inserted/moved/deleted instead of trying to update everything in place.

@theREALdebater
Copy link

My apologies, I think I didn't explain it very well. Supposing my SPA is an ELIZA-like program (a trivial psychologist simulation). There are two areas on the page: one is a text box that allows the user to enter a message; the second is a paragraph containing ELIZA's reply. The reply is a sequence of words forming a plain English sentence, but some of the words are to be in bold. I want to declare a function that will return the reply, given the user's message. Since some words in the reply are to be in bold, I want to be able wrap some bits of text in the reply in a span element (with a class). An example of a return value might be: ["Why","is","your",<span className="refword">dog</span>,"afraid","of",<span className="refword">spiders</span>,"?"]. If this array ended up in a variable a, we might wrap it <p className="reply">{a}</p> thus, to get the paragraph to be rendered. But then React complains that the array elements (?) must have keys. I think it would be nonsense to try to put keys on everything in the array in this case. The next time there is an update - presumably because the user has typed in a new message - the reply will be a completely different sequence of words. I feel there isn't any point in React trying to identify which ones have changed or not changed. I think this is a case where it is better to just update the (real) DOM with the re-rendered p - with all its children (from the array) - without wasting time and memory trying to work out what has changed. I am suggesting I could use <p atomic={true} className="reply">{a}</p> to get this effect.

@aweary
Copy link
Contributor

aweary commented Nov 14, 2017

I think it would be nonsense to try to put keys on everything in the array in this case.

@theREALdebater React doesn't require you to key text nodes, so you only need to add keys to the span elements in that example. If the array of elements is totally static you can just set the key to a random value or use a simple counter.

Alternatively, there is an API that will automatically key an array of elements. React.Children.toArray can be used to map an array of elements to an array of elements with keys. That's not the primary purpose of the API but it does work. With that in mind you can create an AtomicText component that does what you want:

function AtomicText({ text }) {
  return <div>{React.Children.toArray(text)}</div>
}

Then just pass it the array of unkeyed elements as a prop (not as children, otherwise the keys get checked)

<AtomicText text={["Hello", <span>world</span>]}/>

Here's a demo of this. I hope that helps with your use case!

Edit: don't use React.Children.toArray like that, it's a bug

@theREALdebater
Copy link

I very much appreciate the responses. I like the idea of the AtomicText component, and I'll probably use this technique.

I would suggest, however, that it doesn't resolve the fundamental issue, that sometimes there is a component for which it is inappropriate for React to be checking individually on the need to update the (real) DOM for each of its sub-components. Instead, it makes more sense to update the whole sub-tree if any part of it has changed. I suspect that this could save significant CPU time, since React no longer has to make updates for individual sub-components. React would only have to test if any of them has changed (or any have been added or lost), and if they have then replace the entire sub-tree.

I suggested a prop named 'atomic', but It might make more sense to provide a new React class called AtomicComponent, which I think could have the same API as a Component, but would have the behaviour I describe (if it or any of its sub-components has changed, replace the whole sub-tree in the DOM).

Like all suggestions, I guess this one goes on the end of the list, and will be prioritised as appropriate :-)

Again, thanks for your attention.

@syranide
Copy link
Contributor

I suggested a prop named 'atomic', but It might make more sense to provide a new React class called AtomicComponent, which I think could have the same API as a Component, but would have the behaviour I describe (if it or any of its sub-components has changed, replace the whole sub-tree in the DOM).

If you key the outer component and update the key on every change, then you would get this behavior.

@clemmy
Copy link
Contributor

clemmy commented Nov 14, 2017

@aweary I thought it was a bug that the elements passed in aren't validated? Maybe I'm misunderstanding something.

@aweary
Copy link
Contributor

aweary commented Nov 14, 2017

@clemmy yes we discussed this after I made that comment and came to the conclusion that the behavior of React.Children.toArray skipping key validation is a bug, so @theREALdebater I don't recommend using that approach anymore.

Keying the parent component to force reconciliation as @syranide seems like the simplest solution to what you want in general @theREALdebater. For the array/key issue I would just suggest adding keys with arbitrary values to any elements in the array.

React.Fragment will be available at some point, but AFAIK you still need to key elements if you're using an array right @clemmy?

@clemmy
Copy link
Contributor

clemmy commented Nov 14, 2017

Ah, got it 🙂

You'd be able to do something like:

<React.Fragment key="foo">
  Hello
  my
  <h2>name</h2>,
  is
  <h2>{this.props.name}</h2>
</React.Fragment>

but I don't think that solves the problem of wanting to use an array without keying.

@gaearon
Copy link
Collaborator

gaearon commented Nov 28, 2017

@aweary
Copy link
Contributor

aweary commented Nov 28, 2017

To clarify, React.Fragment will still require you to key elements if you are using a dynamic array instead of defining them statically.

@gaearon
Copy link
Collaborator

gaearon commented Nov 28, 2017

Oh yeah sorry I didn't make it clear. I was referring specifically to this comment: #2816 (comment).

@theREALdebater
Copy link

I think the release of v16.2 with fragments is excellent, and I am pretty sure I will be making use of them, but they don't quite resolve this issue: there is still no way to prevent React's warning on elements in an array (or iterator) that do not have a key; there is still no way to indicate to React that the nodes in an array (or a component's sub-tree) can be assumed to all change together (and so do not need to be individually replaced in the DOM).

I don't think it would be difficult for me to invent a function that would take an array of nodes, and add a key to all the elements that didn't already have a key. I could invent a class AtomicComponent, derived from React.Component, that automatically did this to this.props.children to create a new prop (e.g. keyedChildren). Alternatively, I could create a function KeyedChildren that called React.Children but added the key where necessary.

Can anyone see a problem with any of these techniques? Any recommendations? I think it would be nice if React got one of these features added; it would establish an interface, and the implementation could subsequently be optimised at leisure.

@gaearon
Copy link
Collaborator

gaearon commented Nov 29, 2017

I think it's reasonable for your use case to do this:

Alternatively, I could create a function KeyedChildren that called React.Children but added the key where necessary.

It's uncommon enough that I don't think we'll be directly supporting it. Especially since it's doable in userland.

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

7 participants