Skip to content
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

Use modern native ES6 import to load plotly.js bundle instead of requirejs which is no longer under active development #4763

Merged
merged 10 commits into from
Oct 21, 2024

Conversation

marthacryan
Copy link
Collaborator

@marthacryan marthacryan commented Sep 11, 2024

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.

@archmoj archmoj marked this pull request as ready for review September 11, 2024 20:04
@archmoj
Copy link
Contributor

archmoj commented Sep 11, 2024

Great PR.
Please add a changelog also please mention if it is fixing #4336?

@marthacryan
Copy link
Collaborator Author

Note: the orca tests are also failing on master

@gvwilson gvwilson added feature something new P1 needed for current cycle dependencies Pull requests that update a dependency file labels Sep 12, 2024
Copy link
Collaborator

@alexcjohnson alexcjohnson left a 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.

@marthacryan marthacryan reopened this Sep 17, 2024
@archmoj
Copy link
Contributor

archmoj commented Sep 17, 2024

Please update the description of the PR and include links & info about requirejs.

CHANGELOG.md Outdated Show resolved Hide resolved
@archmoj archmoj changed the title Remove requirejs Use modern native ES6 import to load plotly.js bundle instead of requirejs which is no longer under active development Sep 18, 2024
marthacryan and others added 2 commits September 18, 2024 21:38
Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>
Copy link
Contributor

@archmoj archmoj left a 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.
💃

@archmoj
Copy link
Contributor

archmoj commented Sep 23, 2024

@LiamConnors do you have permission to merge this?

@LiamConnors
Copy link
Member

@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

@gvwilson
Copy link
Contributor

@archmoj @marthacryan please sync with @LiamConnors about the issue he flagged (previous comment in this thread)

@LiamConnors
Copy link
Member

Regarding the error saw in my previous comment. It only is an issue with notebook<7

@archmoj
Copy link
Contributor

archmoj commented Oct 21, 2024

Is there any blocker for merging this PR?

@marthacryan
Copy link
Collaborator Author

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.

@archmoj
Copy link
Contributor

archmoj commented Oct 21, 2024

💃

@gvwilson
Copy link
Contributor

merge it!

@archmoj archmoj merged commit ae32a70 into master Oct 21, 2024
2 of 4 checks passed
@archmoj archmoj deleted the remove-requirejs branch October 21, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file feature something new P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Latex not working in jupyter notebook v7.
5 participants