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

A few more things to clean up in the promise land #76

Closed
1 of 2 tasks
alexcjohnson opened this issue Dec 4, 2015 · 10 comments
Closed
1 of 2 tasks

A few more things to clean up in the promise land #76

alexcjohnson opened this issue Dec 4, 2015 · 10 comments

Comments

@alexcjohnson
Copy link
Collaborator

  • Check that we're doing things as synchronously as possible (see comment on 70 )
  • Make sure all plot_api calls return promises. Looks like at present plot, newPlot, redraw, restyle, and relayout do, but not extendTraces, moveTraces etc.
@alexcjohnson
Copy link
Collaborator Author

From a separate convo

@etpinard

Plotly.plot's promise story is still a bit of a mystery to me
One thing I do know: we can't test Plotly.plot in jasmine without handling the then() regardless of the plot type

@alexcjohnson

huh, OK, it seems weird to me. We do always return a promise, but everything should be synchronous up to that point in the normal case, so by the time the function exits it should be complete.
might be interesting to watch donePlotting
and see if we are really making everything async for some dumb reason...

It's not the end of the world if plotting is less synchronous than it could be, but the experience is better for the viewer (at the very least fewer flashes of half-built plots, and likely faster display overall) if it's sync. For the developer using plotly.js it's probably no different, they should get used to expecting Plotly.plot and Plotly.newPlot (and everything else in plot_api but there's some more for us to clean up there) to return a promise.

@mdtusz
Copy link
Contributor

mdtusz commented Dec 4, 2015

@etpinard @alexcjohnson I see that the promise resolve isn't passed an argument - should we pass in the gd? That's what I would expect to be handed when using the library, and is trivial to implement because the gd is always in scope when Promise.resolve is called.

@etpinard
Copy link
Contributor

etpinard commented Dec 4, 2015

@mdtusz I vote for passing the gd

@alexcjohnson
Copy link
Collaborator Author

@mdtusz sure, if you think it would be useful. Generally I don't see why it would, as all these functions take gd as an arg so the caller already has it. But I guess there could be cases, like if you bundle a bunch of plot promises together...

@mdtusz
Copy link
Contributor

mdtusz commented Dec 4, 2015

@alexcjohnson my main use-case is if someone does something like this:

// In some place, dynamically adding plot(s)
var myPlot = Plotly.plot('someDiv', data, layout);
var plots = [myPlot];


// Somewhere else in the code, maybe another file
plots.forEach(function(plot){
    plot.then(function(gd){
         gd.className = "visible";
    });
});

It's a bit of a contrived example, but the expected behaviour of promises is generally to receive some success data, or an error. Doesn't hurt to pass it along in any case!

@alexcjohnson
Copy link
Collaborator Author

Doesn't hurt to pass it along in any case!

👍 yup, that was basically the kind of case I was imagining, go for it.

@mdtusz
Copy link
Contributor

mdtusz commented Dec 7, 2015

Added promises to all the Plotly.____ methods here.

@mdtusz
Copy link
Contributor

mdtusz commented Dec 14, 2015

@alexcjohnson @etpinard Can we close this?

@alexcjohnson
Copy link
Collaborator Author

Ideally it would be nice to do a little exploration of whether our async behavior really is what we think it is, ie only MathJax (and maybe gl?) make plotting be async. Mainly to make it easier to write tests around plotting.

Also when it is async, it looks to me as though we need cleanUp to return gd as well so that the resolve arg is consistent?

@alexcjohnson
Copy link
Collaborator Author

considering this closed by #1991 and many before it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants