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

Avoid exernal assets #2249

Merged
merged 38 commits into from
May 28, 2024
Merged

Avoid exernal assets #2249

merged 38 commits into from
May 28, 2024

Conversation

salim-b
Copy link
Collaborator

@salim-b salim-b commented Dec 5, 2022

Directly includes all external assets (JS, CSS, fonts) from first-party plus switches to the minified version of fuse.js. Only implemented for BS5 (old BS3 is left untouched). All external assets are centrally defined in new YAML file inst/BS5/assets_external.yaml which should make future asset maintenance easy.

Direct asset inclusion might be relevant to people trying to strictly adhere to privacy regulations like GDPR, since otherwise IP addresses of site visitors are sent to the asset CDN operators (based in the US) on page load.

Note that the newly imported openssl package used to validate downloaded file hashes was already an indirect mandatory dependence due to the httr import.

Successor to #1541. Fixes #2099.

This is configurable via YAML option `template.params.external_assets`.
(`fs::path_norm()` unfortunately squishes consecutive slashes)
@salim-b
Copy link
Collaborator Author

salim-b commented Dec 5, 2022

Next step could be asset bundling (and minification) into a single CSS and JS file each to reduce the number of requests on page load. Given there's some adequate toolchain available for this in R (which I don't know of)...

Question: Is there a specific reason why currently the unminified version of fuse.js is used in pkgdown? There is a minified version available on the same CDN... Also switched to minified version in 810dd6f.

@salim-b salim-b mentioned this pull request Jan 25, 2023
NEWS.md Outdated Show resolved Hide resolved
R/theme.R Outdated Show resolved Hide resolved
@hadley
Copy link
Member

hadley commented Apr 16, 2024

@salim-b I'm planning to merge this into pkgdown 2.10, which I'll start working on soon. Thanks for continuing to keep this PR up-to-date!

@salim-b
Copy link
Collaborator Author

salim-b commented Apr 16, 2024

I'm planning to merge this into pkgdown 2.10, which I'll start working on soon. Thanks for continuing to keep this PR up-to-date!

Cool!

Let me know if you want me to continue working on the refinements we discussed above.

@jayhesselberth
Copy link
Collaborator

@salim-b can you bump the version of clipboard.js as in #2468

@salim-b
Copy link
Collaborator Author

salim-b commented Apr 19, 2024

@salim-b can you bump the version of clipboard.js as in #2468

Sure, done.

@hadley
Copy link
Member

hadley commented May 28, 2024

@salim-b I started from your PR, but it's diverged quite a lot 😄 I think there are two key ideas here now:

  • Cache the files in the user's cache directory; in hindsight this seems like the obvious place to do the caching. This should make it fast for interactive exploration, and shouldn't have much of an impact on CI since downloading from a CDN should be fast.
  • Create htmlDependencies() so we can use all the existing code for handling the bslib html dependencies (in particularly this now has some nice code to report which files have changed).

@hadley
Copy link
Member

hadley commented May 28, 2024

I also ended up converting the YAML file to R code; in the end I think this is pretty much a net neutral but at various points in my refactoring it seems to make more sense. And there's something nice to keeping all of the dependency code in R.

@salim-b
Copy link
Collaborator Author

salim-b commented May 28, 2024

@hadley This looks great!

  • Caching files in user-cache dir seems fine to me 👍
  • Leveraging the existing htmltools:: dependency handling is more elegant 👍
  • Specifying the assets in R code instead of YAML data is fine with me (I don't really care)

What came to mind though is that if we want to support asset URLs that always point to the latest release of an asset like the ones Pandoc (3.2+) uses for MathJax 31 and KaTeX2, we might want to set integrity = NA_character for them in external_dependencies() and skip check_integrity(). But that's something for a separate PR, I guess.

Footnotes

  1. https://cdn.jsdelivr.net/npm/mathjax@3/es5/tex-mml-chtml.js

  2. https://cdn.jsdelivr.net/npm/katex@latest/dist/katex.min.js and https://cdn.jsdelivr.net/npm/katex@latest/dist/katex.min.css

@hadley
Copy link
Member

hadley commented May 28, 2024

@salim-b yeah, I was thinking we'd need to do some extra work for the math handling, but with this in place it'll be easier enough to do in separate PR.

@hadley hadley merged commit 3751893 into r-lib:main May 28, 2024
13 checks passed
@salim-b salim-b deleted the incl-assets branch May 28, 2024 18:36
SebKrantz pushed a commit to SebKrantz/pkgdown that referenced this pull request Jun 1, 2024
Now turn them into HTML dependencies objects, caching them in the user cache directory. Fixes r-lib#2099.
---------

Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
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.

Embed all external resources locally instead of using a CDN
3 participants