Plots.resize - additional check for gd.layout

[rafalbromirski]

While working with plotly & react-plotly, from time to time I could notice minor error in my console:

Uncaught TypeError: Cannot read property 'width' of undefined

First, I thought the error is somewhere on my side but later it was clear it was inside plotly library. I backtracked the error and ended up here:

if(gd.layout.width && gd.layout.height)

=> https://github.com/plotly/plotly.js/blob/master/src/plots/plots.js#L75-L113

Reproducing it was possible by changing my routes faster than usual – not waiting for plotly to finish rendering charts – chaos monkey style. Apparently when you delete DOM node from your HTML, gd variables still holds the reference to the element which doesn't exist anymore. In that case if you try to call gd.layout.width/height it will throw an error because layout is undefined.

How to reproduce it:

• npm start
• open JS console and type:

gd.layout
=> {updatemenus: Array(18), xaxis: {…}, yaxis: {…}, height: 450, width: 1100, …}

• remove gd element:

gd.parentElement.removeChild(gd)

• try to call it again:

gd.layout
=> undefined

If you call gd, it still points to #graph:

gd
=> <div class="dashboard-plot" id="graph"></div>

[alexcjohnson]

Interesting, thanks for bringing this up @rafalbromirski! Seems like a reasonable check but the source must be a little different from what you've identified. The reason gd has no layout when you detach it in the test dashboard is that we recreate it if needed every second so you're not looking at the same gd. If before calling gd.parentElement.removeChild(gd) you say gd2 = gd, you can see that at the end gd2 is different from gd, and gd2 still has a layout.

However if you call Plotly.purge(gd) (possibly an unmount before this timer runs out?) we DO remove layout, and everything else we had attached to the <div>. So in that case the check you added makes a lot of sense, gd was valid at the time resize was called so we should not generate an error.

Can you add a test to the resize promise test suite mimicking this situation? I'm thinking:

var resizePromise = Plotly.Plots.resize(initialDiv);

// now purge and detach the element before the resize timer runs out
Plotly.purge(initialDiv);
initialDiv.parentElement.removeChild(initialDiv);

// then wait for the promise as usual, if it doesn't error the test passes
resizePromise.catch(failTest).then(done);

hmm, looks like that test file doesn't have our robust promise error handling (failTest) yet - needs it added like in this commit, otherwise a failure is just manifest as a timeout because done never gets called.

[rafalbromirski]

@alexcjohnson thanks for explaining! Will look into tests in my spare time.

[rafalbromirski]

@alexcjohnson I tried to follow your lead but I got an error:

Uncaught TypeError: Cannot convert undefined or null to object thrown

and it seems like this error comes from _redrawTimer method (https://github.com/plotly/plotly.js/blob/master/src/plots/plots.js#L91-L111)

Maybe because I'm calling purge before and almost all plotly attributes are gone?

I tried different solution and it seems like it's working fine. Could you take a look? Not sure if it's the right one though...

[rafalbromirski]

@alexcjohnson you would have to enable it to see the error

[alexcjohnson]

Playing with this a little... I think this version of the test is correct, and what's needed is just to interpret the comment in the code // return if there is nothing to resize a little more broadly - if the plot has been purged (in which case it has no layout) that's another "nothing to resize." So perhaps:

if(!gd.layout || (gd.layout.width && gd.layout.height)) {

Which, at least if I run it in the console (Plotly.Plots.resize(gd);Plotly.purge(gd);) successfully avoids throwing an error.

Hmm, it also occurs to me, looking at the code, we should bail out if isHidden(gd) as well, ie when resize was called the plot was visible, but before _redrawTimer fired it has been hidden/removed. That's yet another "nothing to resize." So I guess:

if(!gd.layout || (gd.layout.width && gd.layout.height) || isHidden(gd)) {

The only other test we do outside _redrawTimer is !gd, but I don't see any way gd could be truthy before and falsy 100ms later, it's still going to be a DOM element, the worst that can happen is it gets detached; so I think that should do it!

[rafalbromirski]

@alexcjohnson done. I added additional code which handles early return (it still returns resolved promise like it did before) but if somebody will change it in the future you will at least have tests for it.

[alexcjohnson]

Beautiful, thanks @rafalbromirski! 💃

[etpinard]

Lovely tests @rafalbromirski

Thanks very much!