search

plotly / dash-renderer

OBSOLETE has been merged into dash
https://github.com/plotly/dash
MIT License
97 stars 32 forks source link

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

Closed valentijnnieman closed 5 years ago

valentijnnieman commented 5 years ago

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 commented 5 years ago

Can we add an integration test?

valentijnnieman commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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?