Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

data-table: fixes warning about missing key prop #1778

Merged
merged 5 commits into from
Sep 30, 2021
Merged

Conversation

antoinerg
Copy link
Contributor

@antoinerg antoinerg commented Sep 24, 2021

Closes plotly/dash-table#860 and potentially #1433

As pointed out in plotly/dash-table#690 (comment), in order to see the warnings on the demo page, we need to use React's dev bundles (see 3eac50a). I also changed the version of the React bundle to match the one used in package-lock.json. I don't know if this should stay in the PR but @chabb made a good argument for it to stay in.

The fix itself can be found in a25cca9. It adds the one missing key prop. After this commit, the missing key warning disappears in the demo page!

cc @alexcjohnson

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • task 1
    • task 2
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

@alexcjohnson
Copy link
Collaborator

Nice! Is there a way for our tests to catch this warning so we can ensure it doesn't pop up again as we change things?

I also changed the version of the React bundle to match the one used in package-lock.json. I don't know if this should stay in the PR but @chabb made a good argument for it to stay in.

Agreed - though at some point soon we will want to make react 17 an option you can enable for any given Dash app, at first just for our own testing purposes.

@antoinerg
Copy link
Contributor Author

Nice! Is there a way for our tests to catch this warning so we can ensure it doesn't pop up again as we change things?

Via a Selenium test, it would probably be possible to check console logs and assert they're empty.

@antoinerg
Copy link
Contributor Author

Via a Selenium test, it would probably be possible to check console logs and assert they're empty.

Actually, for this to work, we also need to use a development bundle of React in the tests. I'm not sure what's the best way to do that.

cc @alexcjohnson

@alexcjohnson
Copy link
Collaborator

We serve the dev version of react etc when you're in debug mode. In tests we typically disable hot reloading when we turn on debug, like:

dash_duo.start_server(
app,
debug=True,
use_reloader=False,
use_debugger=True,
dev_tools_hot_reload=False,
)

@antoinerg
Copy link
Contributor Author

Nice! Is there a way for our tests to catch this warning so we can ensure it doesn't pop up again as we change things?

Done in 50f8c04

Here's proof that this test detects the warning: https://app.circleci.com/pipelines/github/plotly/dash/2751/workflows/f3be563b-0da9-44e2-a0e1-4f0ad7796b2b/jobs/45091

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it, thanks for taking care of this! Please add a quick changelog entry, then 💃

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

Successfully merging this pull request may close these issues.

React Warning "each child should have a unique "key" prop
2 participants