facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
229.28k stars 46.96k forks source link

Consider a more specific warning for key={undefined} #11935

Open gaearon opened 6 years ago

gaearon commented 6 years ago

Proposed in this comment:

I had changed the casing of "ID" in the response, but forgot to commit it aaaaaand I ended up with it happening.

Basically I was doing key={undefined}. Could React warn user when this happens, something like "Looks like you tried to supply a key, but the value supplied is undefined. Check the render..." and so on?

I think it might make sense to give a more specific warning in this case. Open to suggestions about specific wording and in which case it would be used.

onstash commented 6 years ago

Hi @gaearon. I would like to take this issue as my first pull request if it is not too complex 😅. I will wait* until discussions about specific wording have come to a conclusion.

gaearon commented 6 years ago

Feel free to propose the behavior you think would be reasonable.

onstash commented 6 years ago

Here is my approach:

This warning

warning(
  false,
  'Each child in an array or iterator should have a unique "key" prop.' +
    '%s%s See https://fb.me/react-warning-keys for more information.%s',
  currentComponentErrorInfo,
  childOwner,
  getStackAddendum(),
);

could be modified as

warning(
  false,
  'Each child in an array or iterator should have a unique "key" prop that is not "undefined".' +
    '%s%s See https://fb.me/react-warning-keys for more information.%s',
  currentComponentErrorInfo,
  childOwner,
  getStackAddendum(),
);

if element.key is present but undefined.

I am not sure if this is a foolproof solution or a hack. Please let me know your thoughts about this. 😅

k1sul1 commented 6 years ago

How about the other values, that are simply wrong: objects, empty strings, booleans, null, undefined.

Did I miss any?

onstash commented 6 years ago

Ah. I didn't think of other invalid values. Thanks for pointing that out @k1sul1. I'll wait for @gaearon to reply to our comments.

SadPandaBear commented 6 years ago

@k1sul1 function!

gaearon commented 6 years ago

Maybe:

Each child in an array or iterator should have a unique "key" prop. Received: (put actual value here).

SadPandaBear commented 6 years ago

Hey, I'm working on this just to see which tests you'd have to enhace too.

So far:

ReactStatelessComponent-test:361

ReactChildren-test:959

ReactChildren-test:980

ReactElementValidator-test:67

ReactElementValidator-test:85

ReactElementValidator-test:97

ReactElementValidator-test:118

ReactJSXElementValidator-test:72

Well, I realised that all of those tests are considering only "null" keys. Should we proceed this way or test for undefined (and eventually other values) too?

Perhaps I'll open a PR later and you guys can give me your thoughts.

danrot commented 4 years ago

I was just wondering: Wouldn't it be ok, if only one of the passed values is undefined?

tito300 commented 2 years ago

In my case I use the key prop sometimes for reseting a component (not in a list). In that case, it would be undefined in some cases (while fetching data or key comes from a selected item and the user selects none). I think this should be fine unless there is another way to handle this use case.

brendenehlers commented 11 months ago

Any updates on this? I'm happy to pick it up if there's a consensus on what the error content should be.

It's been a few years and this would be a helpful change to have when working on React projects.