Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Fix 'child in a list should have a unique key prop' #690

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

chabb
Copy link

@chabb chabb commented Feb 14, 2020

  • Each element of a react list should have a unique key prop. This
    is not the case here, as the key of a cell is generated from its
    column index. To have unique key, we should use the row and column
    index.

  • The ./demo folder, should use the development version of react
    (at least when you are developing ), otherwise React will NOT
    show errors/warnings. This is very confusing, as the ./demo folder
    is used for development purposes. The only way to see the react
    warning was to change the script tags to point to the development
    bundle of react. We should point to the correct bundle (e.g, if
    NODE_ENV is set up to DEVELOPMENT, load the DEV bundle, if NODE_ENV
    is set up to PRODUCTION, load the PRODUCTION bundle )

  • I still get this error in the demo, but not in my component. I
    guess there are some places where the keys are still not unique.
    I was not able to find them in the dev tools and did not dig
    thru the code too much

- Each element of a react list should have a unique key prop. This
is not the case here, as the key of a cell is generated from its
column index. To have unique key, we should use the row and column
index.

- The ./demo folder, should use the development version of react
(at least when you are developing ), otherwise React will NOT
show errors/warnings. This is very confusing, as the ./demo folder
is used for development purposes. The only way to see the react
warning was to change the script tags to point to the development
bundle of react. We should point to the correct bundle (e.g, if
NODE_ENV is set up to DEVELOPMENT, load the DEV bundle, if NODE_ENV
is set up to PRODUCTION, load the PRODUCTION bundle )

- I still get this error in the demo, but not in my component. I
guess there are some places where the keys are still not unique.
I was not able to find them in the dev tools and did not dig
thru the code too much
@alexcjohnson
Copy link
Collaborator

Thanks for the contribution @chabb - this is certainly annoying. But I'd like to figure out exactly where the problem is and just fix that rather than sprinkling more complex keys all over. I've only ever seen this message (from the table) coming from <th> elements, and I think the way cells are keyed just by column makes sense, because they're inside a <tr> for each row, so only column is needed to disambiguate.

Also what can we do to test this fix? Use debug mode for some of the tests and verify that we don't see any console warnings, is that enough? Perhaps this should happen in some of the tests @Marc-Andre-Rivet is porting to Selenium in #696.

@chabb
Copy link
Author

chabb commented Feb 28, 2020

Thanks @alexcjohnson for taking care of it. I've tried to fix this issue, but I realized that it was not as straightforward as I thought it would be, especially if we want to have a good/consistent fix.

I think using the dev version of react for your tests, and failing tests on console errors would be the right good thing to do ( though it can potentially makes your tests slower and generates a lot of noise); but in the end, you do not want developers to see errors in their chrome/safari/firefox/(IE) dev console

@simonhammes
Copy link

Thanks for the contribution @chabb - this is certainly annoying. But I'd like to figure out exactly where the problem is and just fix that rather than sprinkling more complex keys all over. I've only ever seen this message (from the table) coming from <th> elements.

I am currently trying to fix this exact problem; what is the suggested solution?
The weird thing is that I don't even see the th components in the React Dev Tools, only the actual table rows.

Thanks in advance!

@blanning19
Copy link

Please let me know when this has been fixed so I can pull.
Thanks

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants