-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Added promise returns to Plotly.plot #226
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
Conversation
@tchandler This looks good. Thank very much for doing this 🍻 We should add to test cases in https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/plot_promise_test.js before merging. I hope that you'll be up for the challenge 😏 But, if you don't feel like it (which totally understandable), me or @mdtusz will gladly add the test cases for you. Cheers. |
@etpinard I'm working through the unit tests and I noticed something, in #77 the Promise returns that were added are implemented like this: new Promise.resolve(gd);
//or
new Promise.reject(); But you can't use
I suspect the reason this doesn't break in production is because either the build fixes the error or the polyfill for Promises is implemented as a JS constructor function, which works just fine with that syntax:
I should have another commit for this in an hour or 2 that fixes my and the other uses of |
Good catch! Those were meant to be fixed in a PR that's been put on the backburner https://github.com/plotly/plotly.js/pull/149/files#diff-2941ab69a12080c0633ff4ac8ea3aa83R1654 We'll review what you've got once you've pushed up the next changes! |
Changes and tests look good to me so I'll merge it in. Thanks for the contribution! 💃 |
Added promise returns to Plotly.plot
Added promise rejections missed in #77 meant to resolve part of #76
All tests pass.