Test mathjax #1991
Test mathjax #1991
Conversation
| @@ -107,7 +107,7 @@ Plotly.plot = function(gd, data, layout, config) { | |||
| // any part of the plotting code can push to gd._promises, then | |||
| // before we move to the next step, we check that they're all | |||
| // complete, and empty out the promise list again. | |||
| gd._promises = []; | |||
| if(!Array.isArray(gd._promises)) gd._promises = []; | |||
There was a problem hiding this comment.
This I didn't write a test for... but I made this change so that if you make a plot, then change it before its promises have finished, you should still wait for them (via previousPromises) so you don't get a race condition between them and the new plot (with the old ones potentially overwriting elements of the new plot you made).
I haven't actually managed to cause any ill effects without this change in manual testing, but I think it's safer this way.
| // even if everything we did was synchronous, return a promise | ||
| // so that the caller doesn't care which route we took | ||
| return Promise.all(gd._promises).then(function() { |
There was a problem hiding this comment.
This was the crux of the issue: if you have more than one async item in seq, only the promises added to gd._promises by the first one will exist when this Promise.all is called (at the synchronous return of Lib.syncOrAsync), so any added after that will not be awaited. But Plots.previousPromises (added to the end of seq above) looks at gd._promises when it's actually invoked so it gets them all.
There was a problem hiding this comment.
Solid investigation work 🔬 🎉 🥇
Is this enough to close #76 you think?
There was a problem hiding this comment.
Is this enough to close #76 you think?
I think so!
|
I tested this locally using the old (i.e. current aka To make sure (sure), I'll manually rebuild this branch on CI a few times. |
|
Moreover, I'm thinking that we could start testing |
One unrelated error, but the mathjax part succeeded 4/4 times on circle and all the times we've done it locally - looks pretty solid to me, ready to go? |
I want 5/5; so one more run -> https://circleci.com/gh/plotly/plotly.js/4955 |
|
It's green ✅ 💃 💃 💃 |
$is replaced byHin the mock where it wouldn't work correctly)Plotly.plotpromise chain that meant multiple MathJax items in different parts of the plot would render but the plot would report being done before it really had finished. Prior to this change, the mock fails.cc @etpinard - hopefully this will fix our MathJax image export issues.