-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Use modern native ES6 import to load plotly.js bundle instead of requirejs which is no longer under active development
#4763
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
Great PR.
Please add a changelog also please mention if it is fixing #4336?
Note: the orca tests are also failing on master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃 I'm on board - not aware of any good use cases for requirejs here. Maybe there were in the past, but mostly those use cases would have applied to direct plotly.js use rather than plotly.py anyway.
Please update the description of the PR and include links & info about requirejs.
native ES6 import to load plotly.js bundle instead of requirejs which is no longer under active development (追記ここまで)
Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR 🏆
Nicely completed.
💃
@LiamConnors do you have permission to merge this?
@marthacryan and @archmoj, when building Plotly.py using this PR and the master branch of Plotly.js, figures don't load in a classic notebook and the following error is in the console.
image
@archmoj @marthacryan please sync with @LiamConnors about the issue he flagged (previous comment in this thread)
Regarding the error saw in my previous comment. It only is an issue with notebook<7
Is there any blocker for merging this PR?
Is there any blocker for merging this PR?
@archmoj @LiamConnors @gvwilson I believe there isn't anything blocking merging since Liam was able to use this with Notebook 7.
💃
merge it!
Uh oh!
There was an error while loading. Please reload this page.
Note: this PR breaks support for notebook < 7. #4805 is already working on removing that code and adding a warning.
This removes references in the base renderers to requirejs. Requirejs is essentially unmaintained and newer versions of jupyter lab and jupyter notebook don't support it anymore, so this fixes some of the notebook 7 and lab 4 issues we've been seeing. Replaces requirejs support with native javascript modules: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules
Alternative to #4762 that doesn't remove the CDN.
Fixes #4336.