Add key to rendered components, so React doesn't optimize

[valentijnnieman]

This is an interesting fix, where we're now passing a key to the rendered components, so that React doesn't try to optimize and recognizes that the components are different. This is an issue for having multiple Graph components for example, where if there's no key, React will try to optimize and will use the same state for both Graphs. This is what's happening in dcc #379, and this PR will fix that.

Link to the React docs: https://reactjs.org/docs/lists-and-keys.html#keys

[chriddyp]

Can we add an integration test?

[valentijnnieman]

@chriddyp I didn't add any integration tests here because these are React internals - what should it test? Should it test that the React component's constructor() is called for each new Graph component? Or test that React passes a key prop correctly to the components? I guess I can add a test that tests if click data is being passed through to a callback, when there's multiple Graph components, but that feels more like a DCC test to me.

[chriddyp]

I guess I can add a test that tests if click data is being passed through to a callback, when there's multiple Graph components, but that feels more like a DCC test to me.

That's what I'd recommend testing. Basically, we have an example that is broken right now (tabs in a graph) because of dash-renderer. This PR proposes that it fixes this example, so we should add a test that mimics that example to "prove" that it indeed fixes the issue.

[chriddyp]

Or test that React passes a key prop correctly to the components?

This in and of itself isn't important for me to test as we have to infer that adding a key will prevent something from using the same instance which will prevent odd event handling bug code like this. So, I'd rather test a step further back with the actual tabs and graphs.

[chriddyp]

Could we reorder the commits so that the test is first? That way we can make sure that the test is failing on CircleCI and then is subsequently fixed with the next commit. Once my comments are considered, 💃

[valentijnnieman]

Thought I'd mention that I also considered passing in the key prop in the component that NotifyObservers spits out - via extraProps here that fixes the bug too, and puts the key right on the component instead of on it's parent.

[chriddyp]

Since this is taking a little longer to get through review, could we create a pre-release so that any community members or customers that are affected by this bug can try out the prerelease?

[Marc-Andre-Rivet]

Internally this is added -- externally this is a bug that's been fixed. Change to ```### Fixed`

[Marc-Andre-Rivet]

Could we reorder the commits so that the test is first? That way we can make sure that the test is failing on CircleCI and then is subsequently fixed with the next commit.

I agree we should see this test failing without the fix before concluding the fix is correct -- the test is very remote from the code fix and we have to infer a lot. Running local CircleCI CLI with and w/o fix shows that the fix allows the test to pass. Manually tested the test, this look good. 💃

In the future, we should consider creating test components that live inside of the dash repo to test for specific scenarios like this. These components will provide stable, no-surprise, no-dependency implementations for specific test cases (for example this could be a simple Dash Component that renders state.value which is copied from props.value in the ctor, guaranteeing the keys have the expected behavior)