Move plotly.animate to Lib.syncOrAsync

[rreusser]

This PR moves the content of Plotly.animate into Lib.syncOrAsync so that it better fits into the plotly queue. The particular use-case is Plotly.restyle and Plotly.animate called in synchronous series (i.e. not promise-chained), though I'm currently unable to produce a situation that actually triggers any problems. I stumbled upon the possibility of a problem while working on the notifier, and solved things just by properly then-ing all the plotly API calls. That's good practice anyway, but this is also probably the right way for things to be.

I'm not quite sure how to produce a test case for this (short of inspecting the promise queue, except those are just anonymous callbacks, right?), but I'm glad to try if someone can come up with an idea that's not inherently fragile due to a race condition.

Additional note: there are only ~240 lines due to an indentation change. I've only changed this:

return new Promise(...)

into this

return Lib.syncOrAsync([
  function () {
    return new Promise(...)
  }
], gd);

/cc @alexcjohnson @etpinard

[rreusser]

Okay, it passes the tests. That's the first big step. That means it's worth inventing a test. I'll just be creative.

[rreusser]

I've added a test, but the conclusion is that most of the work was ending up on the main plotly queue anyway via Plots.transition so that I'm not 100% convinced this should be merged. I need to think about the extreme subtleties just a bit more.

Which is to say, what I thought I was implementing here was already happening correctly so that I'm not quite sure what this PR does do beyond adding some redundancy and moving a couple extra lines into the promise queue.

[rreusser]

Okay here's my conclusion on this PR:

I can't find a problem it solves. Plotly.animate currently does some setup work synchronously when called. This PR defers that work until syncOrAsync starts executing its callbacks, but since the frame modification functions do their work and resolve synchronously anyway, there's not really a way to get things out of sequence unless you misuse the API in ways that aren't really 'correct' anyway, i.e.:

Plotly.plot(...)
Plotly.animate(...)
Plotly.addFrames(...)

instead of:

Plotly.plot(...).then(() => {
  return Plotly.addFrames(...)
}).then(() => {
  return Plotly.animate(...)
})

So in the end I don't really have a strong preference on whether to merge this or not. I think it comes down to a matter of conventions instead of cases that succeed/fail. My question then is:

Should plotly.js API functions do any setup when invoked, or should everything be wrapped in Lib.syncOrAsync?

[alexcjohnson]

@rreusser there's not all that much that plotly.js does async, so it's not surprising that you can't pin this down. Mathjax rendering, geo and mapbox plots fetching data, fetching images... that might be it.

Note that Lib.syncOrAsync doesn't, on its own, wait for the existing promise queue to complete; it just ensures proper sequencing of items in its own arguments. It also doesn't necessarily do anything async or promise related, in fact it keeps things sync whenever possible.

If your goal is to wait for the plot to finish anything else it's been doing - which I think is a good goal, and probably important in some cases (a map or a plot with mathjax that animates on load?) then I think you just need to add Plots.previousPromises:

return Lib.syncOrAsync([
    Plots.previousPromises, // only async if there *are* promises in the queue
    function() {
        return new Promise(...)
    }
], gd); // gd is just passed along to previousPromises, it's not needed for your own fn

then if everything is sync, done will just be undefined, but since you're making a new promise, it will just be that promise (having chained it onto previous ones if necessary). In other places we've had to generate a promise to keep the API consistent, but you already know you have one.

[rreusser]

Thanks for the clarification, @alexcjohnson! Is there a rule of thumb on which API functions should wait for previousPromises to finish? Like it's possible to write a synchronous-looking API that queues everything internally and is actually asynchronous, but I'm not sure if that's a good or bad goal. My specific concern is that API commands would get interleaved inside the async parts of Plotly.plot (though as you pointed out, even though it's a sequence of promises, do they all resolve synchronously?) leading to nasty race conditions.

[alexcjohnson]

Is there a rule of thumb on which API functions should wait for previousPromises to finish?

Offhand I'd say anything that can modify the plot should wait for previousPromises and return a promise itself. which is... basically the whole top-level API? Below that level we're basically not exposing anything new anyway, and we'll probably de-expose all of those in v2, so nothing to worry about.

even though it's a sequence of promises, do they all resolve synchronously?

Are you talking about gd._promises or Lib.syncOrAsync? gd._promises is an array of whatever async stuff happened during plotting, so you could always call Promise.all on it if you need a promise, but Plots.previousPromises will return undefined if it's empty so that a syncOrAsync call will stay sync. syncOrAsync is called with a sequence, but it never creates promises, only waits for promises that its items return before continuing the sequence. If no items return a promise, the whole thing is sync, ie it's done before it returns and it does not return a promise.

[etpinard]

Mathjax rendering, geo and mapbox plots fetching data, fetching images... that might be it.

That is it, as far as I know. Shout-out to #76 🔈

Offhand I'd say anything that can modify the plot should wait for previousPromises and return a promise itself. which is... basically the whole top-level API?

Plotly.purge is an exception.

Below that level we're basically not exposing anything new anyway, and we'll probably de-expose all of those in v2, so nothing to worry about.

👍

[alexcjohnson]

Plotly.purge is an exception.

Makes sense, though... what would happen if you purge with promises still pending? I guess the hope would be that it just throws an error inside the callback, which doesn't affect anything in the wider app... or perhaps it can still act on the now-removed plot elements and doesn't even throw an error? I guess mostly it will be fine but perhaps there are cases this would pollute gd by reattaching some variables to it or something.

[etpinard]

Closing. It's probably best to find a scenario where not using Lib.syncOrAsync causes a bug before going through with this.

Now part #933