Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Remove references to plotly.js CDN and requirejs #4762

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

Closed
marthacryan wants to merge 6 commits into master from remove-plotlyjs-cdn

Conversation

@marthacryan
Copy link
Collaborator

@marthacryan marthacryan commented Sep 11, 2024
edited
Loading

This removes references in the base renderers to the CDN and requirejs.

  • Latest plotly.js used to be on a CDN, but it isn't anymore (we decided it was too much maintenance years ago)
  • 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.
  • Removes setting window._Plotly because that appears to be unused

This should fix #4336.

archmoj reacted with hooray emoji archmoj reacted with heart emoji archmoj reacted with rocket emoji
@marthacryan marthacryan changed the title (削除) Remove references to plotly.js CDN (削除ここまで) (追記) Remove references to plotly.js CDN and requirejs (追記ここまで) Sep 11, 2024
@marthacryan marthacryan marked this pull request as ready for review September 11, 2024 16:27
If 'cdn', a script tag that references the plotly.js CDN is included
in the output. The url used is versioned to match the bundled plotly.js.
HTML files generated with this option are about 3MB smaller than those
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep cdn option as it could help reduce smaller HTML files.
But for loading that we shouldn't use requirejs.

Copy link
Collaborator Author

@marthacryan marthacryan Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok - but we don't have a cdn currently right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we publish every version to CDN.
It's only the -latest version is no longer updated (plotly/plotly.js#5697) since plotly.js v2 and stayed as latest v1 version i.e. 1.58.5.
So we should be able to continue loading from CDN by specifying the exact version.
To achieve that you could add a separate script tag instead of requirejs to load e.g.

<script src="https://cdn.plot.ly/plotly-2.35.1.min.js" charset="utf-8"></script>

Or alternatively for async loading

<script type="module">
 import "https://cdn.plot.ly/plotly-2.35.2.min.js"
 Plotly.newPlot("gd", [{ y: [1, 2, 3] }])
</script>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcjohnson alexcjohnson Awaiting requested review from alexcjohnson

1 more reviewer

@archmoj archmoj archmoj left review comments

Reviewers whose approvals may not affect merge requirements

Labels

fix fixes something broken P2 considered for next cycle

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Latex not working in jupyter notebook v7.

AltStyle によって変換されたページ (->オリジナル) /