Plots.resize: promises should always be resolved

[antoinerg]

@Marc-Andre-Rivet pointed out to me that the promises returned by Plotly.Plots.resize would sometimes never complete. This PR fixes that by resolving all promises. If many calls are made one after the other, they will all resolve when the last resize operation is done!

[etpinard]

Cool. Thanks for pointing this out. I'm wondering though, why should the second Plots.resize call result in a promise-reject? Could it resolve and maybe Lib.log resize in progress instead?

[antoinerg]

why should the second Plots.resize call result in a promise-reject? Could it resolve and maybe Lib.log resize in progress instead?

I thought that rejecting made more sense since the resize has not happened yet. The goal is to clearly tell the caller that the operation is not completed. Listening to the log is probably not ideal for this purpose.

Another possibility would be to resolve all pending promises once the resize operation is done. We could keep all the resolve functions from previous promises in an array and simply call them all when resizing is done.

I'm not sure what's the best behavior to adopt here.

cc @Marc-Andre-Rivet @alexcjohnson

[Marc-Andre-Rivet]

I thought that rejecting made more sense since the resize has not happened yet.

As a customer of the promise both resolving or rejecting the promise can make sense. If resolving instead of rejecting, providing a state that makes it clear that a resize has occurred or not would be necessary to add value vs. rejection: callers don't need to keep track of the number of in-progress requests anymore, they only need to wait for a promise to resolve with a success state.

--Actually if resize can take longer than the debounce timeout, true/false state on resolve will not be of any use. Seems pretty clear this could happen, which means a counter from the caller's side is still the best bet.

[antoinerg]

Ok, so according to @Marc-Andre-Rivet, it seems like resolving/rejecting versus resolving true/ false is equivalent.

It is, therefore, a matter of preference/philosophy for @plotly/plotly_js. I don't have a strong preference as long as all promises are fulfilled or rejected. Just let me know what you would prefer.

[alexcjohnson]

A caller probably doesn't actually care that another resize delayed the redraw, they just want to know when the resize is really truly done, even if it takes 10 sec because someone keeps spamming resize calls. So what if instead we resolve all of these promises at the same time, whenever the redraw really happens?

If I'm understanding this correctly, we can do that by simply resolving the first promise with the new promise we've just made - ie store it in a var rather than just return new Promise(...) (sorry if the below is obvious to you, I had to try it to make sure it worked as I thought it should)

> p=new Promise((resolve, reject) => {window.res=resolve})
> p.then(v=>console.log('p: ', v))
> p2=new Promise((resolve, reject) => {window.res2=resolve})
> p2.then(v=>console.log('p2: ', v))
> res(p2) // this doesn't resolve p, because we're resolving with another promise
> p
< Promise {<pending>}
> res2(42) // this resolves p2 AND p
    p2:  42
    p:  42
> p
< Promise {<resolved>: 42}

[Marc-Andre-Rivet]

@alexcjohnson Nice! I was not aware of that impl detail for resolve. Just to be certain, checked that the behavior is not a Chrome idiosyncracy and is implemented the same way in the promise-polyfill. My concern was about book keeping but with this approach the gd can keep track by itself.

Here's a scenario where we may need to do more than wait for a single resize operation to be triggered and completed in order to resolve the promise? Doing away with counters on the caller's side entirely.

Given that resize debounces/trails if called within 100ms and that it's possible for a resize operation to take >100ms, assuming the resize operation is at least partially async and not debounced inside

plotly.js/src/plots/plots.js

Line 105 in deb32ff

Registry.call('relayout', gd, {autosize: true}).then(function() {

and that a resizes on the same gd keep order:

[0ms] resize request No.1
[90ms] resize request No.2 within 100ms of No.1, debounces No.1
[190ms] 100ms passed, resize calc starts for No.2, this calc will take 250ms
[200ms] resize request No.3 110ms later not debouncing No.2
[300ms] 100ms passed, resize calc starts for No.3, this call will take 250ms
(speculative implementation)
[440ms] resize completes for call No.2; promise resolves to No.3 as another resize is in progress
[550ms] resize completes for call No.3; promise resolves as no other resize is in progress
[550ms] caller's resize promises for requests No.1, No.2, No.3 resolve

[alexcjohnson]

Good call @Marc-Andre-Rivet - this is a really helpful scenario.

There's not very much that happens async in plotly.js, and in fact in a resize I'd think the vast majority of these would already be done. For the most part it's MathJax and map data, and once you've drawn the graph once, on a resize you already have the correct data so these operations should complete synchronously. But there may be some edge cases where it's still async...

Then I think the important thing is that any new call to Plotly.plot (the underlying routine that resize, relayout, react, newPlot etc all call out to) starts its async-friendly portion by waiting for any in-progress plotting promises to resolve. So I believe your request No.3 won't start its 250ms redraw until No.2 finishes its redraw - finishing at 440+250=690ms. At that point the question is, can we prevent No.2 from resolving until No.3 completes, and do we want to? I could see arguments either way - maybe you want to maintain a 1:1 correspondence between redraws and resolves, so that if there's some quick operation to do elsewhere on the page to keep visually in sync with the graph, you can have that happen each time... on the other hand it seems like it would simplify external handling a lot if all overlapping requests resolve simultaneously, as soon as you know that every queued redraw is really truly done. I guess the latter sounds better to me, a single final resolve of everything, unless anyone can think of a good argument for the former.

[antoinerg]

Commit 040d6ac resolves all promises instead of rejecting them.

About overlapping requests:

At that point the question is, can we prevent No.2 from resolving until No.3 completes

I am not familiar with that part of the code so I can't say how much work it would involve. Maybe @etpinard could give us an idea.

cc @alexcjohnson @Marc-Andre-Rivet

[alexcjohnson]

It should all be manageable from within plots.resize. Seems to me we'd just have to track the Promise we're creating (attaching it to gd?), and if you're in the relayout.then(...) and another resize promise has already been started (gd._resizePromise !== myResizePromise or something?), resolve with that one rather than gd?

[antoinerg]

I think the following is simpler and does the right thing: 5ee431e

See the pattern in action at https://codepen.io/antoinerg/pen/YzPXyJv?editors=1002

[alexcjohnson]

Nice! Took me a while and drawing some timing diagrams to be sure I agreed with this, but I think you've got it 🏆

Only thing I might suggest is when we finally get to this resolve(gd), should we also delete gd._resolveResize? Otherwise the next time someone resizes 10 minutes later, there will still be a resolveLastResize(p) call. In principle it's OK to resolve the same promise more than once, but it feels nicer not to, also then we don't need to worry about it during purge.

[antoinerg]

The old promise resolver is now deleted in 5ee431e !

[alexcjohnson]

Lovely, nice work. 💃