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

feat: use artifacts instead of PlotlyJS #4884

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pankgeorg
Copy link
Contributor

@pankgeorg pankgeorg commented Feb 2, 2024

Alternative to #4862, includes #4863

Replaces the download from CDN/scratchspace with an artifact

src/init.jl Show resolved Hide resolved
@fonsp
Copy link
Contributor

fonsp commented Feb 8, 2024

The advantage of this PR over #4862 is that this PR will always have access to the plotly.min.js asset, while #4862 only works when PlotlyJS is loaded by the user, right?

@pankgeorg
Copy link
Contributor Author

The advantage of this PR over #4862 is that this PR will always have access to the plotly.min.js asset, while #4862 only works when PlotlyJS is loaded by the user, right?

Yes, exactly. We also directly link to the Plotly release assets, which is also the disadvantage of this PR (the artifact being 110MB)

@disberd
Copy link
Member

disberd commented Mar 20, 2024

@pankgeorg I created a repository last week to experiment with javascript and bundling that might be relevant for this.
The repo is at https://github.com/disberd/PlotlyArtifactsESM and it automatically bundles and release in an artifact ready format the plotly library.
If you are still pursuing this you might have a look at it and let me know if it can be used or if you see issues with its implementation.

I am planning on using this in PlutoPlotly for the next release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants