Graph overdrawing & failed cleanup

[alexcjohnson]

1. Graph is not removing its event listeners when unmounting. This is a simple problem that I have a fix already done for.
2. Graph with no id replots itself multiple times during startup, with different auto id each time and in a new DOM element each time. Event listeners are only attached to the first one so can never be removed after that one is discarded. Do we even need these auto ids anymore? Can we just get the element from a ref and forget about generating id (and having to query the DOM every time we want to address the graph)? Also can we avoid this multiple redraw thing?

Discovered while investigating https://community.plot.ly/t/why-would-an-app-completely-freeze-if-i-press-on-ctrl-and-throw-an-error-about-a-graph-which-was-perfectly-loaded-and-displayed-on-the-previous-page-5-sec-earlier/27035/5

[wbrgss]

Thanks for taking a look @alexcjohnson! I think these fixes would be really helpful for Dash and related products.

Graph is not removing its event listeners when unmounting.

So would Plotly.purge(gd) replace eventEmitter.removeAllListeners(); in componentWillUnmount()? I still don't understand what eventEmitter is or where it's instantiated. I think it's part of plotly.js but the this in this.eventEmitter refers to the PlotlyGraph class. So has this ever been defined?

Graph with no id replots itself multiple times during startup, with different auto id each time and in a new DOM element each time.

Yeah, I think this is possibly part of the reason ids were out of sync with their respective graphs in #463 #563 — a id reference would still exist on a graph (the first one (?), with the listeners attached), but when the event fired (in this case a resize), the id and/or the graph has been re-generated.

Can we just get the element from a ref and forget about generating id

👍 Generating a random id string feels a bit hacky to me. I think generating a ref should be pretty straightforward. I've used callback refs but we should be able to just use React.createRef() as of React 16+

Can we avoid this multiple redraw thing?

I think this might really help performance in Dash 👍

[alexcjohnson]

AFAICT this.eventEmitter wasn't defined anywhere, must have been a holdover from some old way we were doing things. In principle I think Plotly.purge(gd) is sufficient but in #604 I also included gd.removeAllListeners();

The multiple redraw issue I think only matters when there's no ID, which hopefully doesn't happen all that much (can't happen if the user is attaching callbacks to the graph anyway) but yes, in that case it should be much faster now.

React.createRef is exactly what I used in #604 - I suspect part of the reason we had this ID madness before was renderer-level problems with rendering elements with no ID, but these have been resolved for a while now.