Skip to content

serve Splide and Three.js locally instead of via CDN #16557

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

AritraDey-Dev
Copy link
Member

Description

Serve Splide and Three.js locally via static directory to replace external CDN usage.
Fixes #9723

Command run to download the css and js file -

curl -L -o static/js/three.min.js https://cdnjs.cloudflare.com/ajax/libs/three.js/r68/three.min.js
curl -L -o static/js/splide.min.js https://cdn.jsdelivr.net/npm/@splidejs/splide@latest/dist/js/splide.min.js && curl -L -o static/css/splide.min.css https://cdn.jsdelivr.net/npm/@splidejs/splide@latest/dist/css/splide.min.css

Reviewers

  • Ambient
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Extensions and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure
  • Localization/Translation

@AritraDey-Dev AritraDey-Dev requested review from a team as code owners June 5, 2025 21:56
@istio-testing istio-testing added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 5, 2025
@istio-testing
Copy link
Contributor

Hi @AritraDey-Dev. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@AritraDey-Dev AritraDey-Dev marked this pull request as draft June 5, 2025 22:34
@istio-testing istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 5, 2025
@craigbox
Copy link
Contributor

craigbox commented Jun 6, 2025

It's been a few years since I raised the request. Does this actually make sense?

Pros of CDN: loads quicker in parallel?
Cons: external dependency, versions can change upstream and break site (unless we pin them?)

@AritraDey-Dev
Copy link
Member Author

Yes, definitely makes sense. The concern you mentioned—external changes breaking the site—is actually what's causing issues when the scripts are loaded from the static folder. That’s why I’ve marked this PR as a draft for now. There are a few UI breaking changes I’m still working through.

Do you recommend I continue refining the PR and eventually open it for review?

Copy link
Member

@Arhell Arhell left a comment

Choose a reason for hiding this comment

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

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Jun 7, 2025
@craigbox
Copy link
Contributor

Sure, go ahead.

@AritraDey-Dev AritraDey-Dev marked this pull request as ready for review June 12, 2025 22:34
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jun 12, 2025
@AritraDey-Dev
Copy link
Member Author

Up for review!

@craigbox
Copy link
Contributor

craigbox commented Jun 17, 2025

Are these files going to be bundled/minified?
Should they be?

(And if so, should we hold off on this until the esbuild PR has gone through?)

@AritraDey-Dev
Copy link
Member Author

These files are already minified,look at the extension min.js, min.css
I believe they should definitely be minified, as that reduces size and boosts performance.

(And if so, should we hold off on this until the esbuild PR has gone through?)

I believe we don't need to hold off until the esbuild PR goes through because these Splide files are used directly by HTML files and the esbuild PR is focused on changing js files, so they are independent changes.

@AritraDey-Dev AritraDey-Dev requested a review from craigbox June 19, 2025 21:07
@craigbox
Copy link
Contributor

craigbox commented Jun 23, 2025

It's still JavaScript that we serve, and if we are going to have a single minified JS file which has its tree shaken, then we should have it only serve the code used by splide.

Should we have some sort of npm pulling splide situation like we have for the others? By making this change we move from latest to pinning the version at this exact point in time.

(That's not to say it's not worth doing, but we should look to do it properly if we are making any change.)

@AritraDey-Dev
Copy link
Member Author

makes sense! I think we can add the particular versions of Splide (4.1.4) and Three.js (0.162.0) to the netlify_install target, then in gen_site.sh we can copy over those files if available from node_modules/ or fall back to existing static JS files,
and if there's a change we can figure that out easily through version pinning

@AritraDey-Dev AritraDey-Dev force-pushed the feat/local-splide-threejs branch from 7fd9aa2 to 8c91bfd Compare July 12, 2025 07:41
@craigbox
Copy link
Contributor

I don't see Three.js here?
And I still think these should be included in our esbuild bundle?

@AritraDey-Dev
Copy link
Member Author

I was thinking of adding Three.js via Docker tools and pinning the version that way. Would that be a better approach instead of adding three.module.js directly to the repository? This way, updating the Three.js version would be easier. If we include three.module.js in the repo, it could become difficult to update it every time.

@AritraDey-Dev AritraDey-Dev requested a review from a team as a code owner July 21, 2025 14:21
@istio-testing istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 21, 2025
@AritraDey-Dev AritraDey-Dev force-pushed the feat/local-splide-threejs branch from 22a8f51 to 594e49d Compare July 21, 2025 14:47
@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR needs to be rebased before being merged size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 21, 2025
Signed-off-by: Aritra Dey <[email protected]>
@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 21, 2025
Signed-off-by: Aritra Dey <[email protected]>
@AritraDey-Dev
Copy link
Member Author

@craigbox I've added three.js in netlify_install. If this approach looks better, I will go ahead and add it to the tools Dockerfile. Please let me know.

@istio-testing
Copy link
Contributor

istio-testing commented Jul 21, 2025

@AritraDey-Dev: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
doc.test.multicluster_istio.io 22a8f51 link true /test doc.test.multicluster
doc.test.profile-minimal_istio.io 22a8f51 link true /test doc.test.profile-minimal
doc.test.profile-none_istio.io 22a8f51 link true /test doc.test.profile-none
doc.test.profile-default_istio.io 22a8f51 link true /test doc.test.profile-default
doc.test.dualstack_istio.io 22a8f51 link true /test doc.test.dualstack
doc.test.profile-demo_istio.io 22a8f51 link true /test doc.test.profile-demo
doc.test.profile-ambient_istio.io 22a8f51 link true /test doc.test.profile-ambient
lint_istio.io 722b28d link true /test lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test and release ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bring splide and three.js into local build
5 participants