Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Plots.resize - additional check for gd.layout #2710

Merged
merged 2 commits into from Jun 18, 2018

Conversation

Projects
None yet
3 participants
@rafalbromirski
Copy link
Contributor

commented Jun 8, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2018

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

@rafalbromirski rafalbromirski force-pushed the rafalbromirski:plots-resize-layout-check branch from e1d47e3 to 3cbb60e Jun 14, 2018

@rafalbromirski

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2018

@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...

.then(done);
});

xit('should return a resolved promise and purge plotly attributes - tmp', function(done) {

This comment has been minimized.

Copy link
@rafalbromirski

rafalbromirski Jun 14, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Jun 15, 2018

Contributor

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 rafalbromirski force-pushed the rafalbromirski:plots-resize-layout-check branch from b4c5a1d to 002bb16 Jun 18, 2018

@rafalbromirski rafalbromirski force-pushed the rafalbromirski:plots-resize-layout-check branch from 002bb16 to 1d75ed1 Jun 18, 2018

@rafalbromirski

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2018

Beautiful, thanks @rafalbromirski! 💃

@alexcjohnson alexcjohnson merged commit e7896e9 into plotly:master Jun 18, 2018

6 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-image2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details
@etpinard

This comment has been minimized.

Copy link
Member

commented Jun 18, 2018

Lovely tests @rafalbromirski

Thanks very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.