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

caching (locally) npm pkgs' dist at docs build stage #150

Closed
2bndy5 opened this issue Aug 18, 2022 · 1 comment · Fixed by #231
Closed

caching (locally) npm pkgs' dist at docs build stage #150

2bndy5 opened this issue Aug 18, 2022 · 1 comment · Fixed by #231

Comments

@2bndy5
Copy link
Collaborator

2bndy5 commented Aug 18, 2022

This may be off topic, but I figured I'd mention it now that this aims to solve #111. I think the fetching of mermaid.js should be added to external_resource_cache. Currently, the sphinx_immaterial.mermaid_diagrams ext is on by default because the theme src seems to be fetching the mermaid.js resource regardless.

Agreed, and also for MathJax. But that could be deferred for a separate PR.

Originally posted by @jbms in #144 (comment)

@2bndy5 2bndy5 changed the title caching (locally) npm pkgs at docs build stage caching (locally) npm pkgs' dist at docs build stage Aug 18, 2022
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 2, 2023

About mermaid.js

I found this upstream comment that explains how the mermaid.js lib is used in the theme's JS bundle.

Note
This following ideas imply that the mermaid ext in our theme becomes optional instead of always enabled.

So, I can add the mermaid.js lib to this theme's external resources ext (on build-init if the mermaid ext is enabled), but I'm not sure this fits into the consolidated bundle because the bundled JS is written to load the script from a separate file (only when there are <div class="mermaid"> elements present).

? watchScript("https://unpkg.com/mermaid@9.1.7/dist/mermaid.min.js")

What I think I need to do is somehow read the contents of the mermaid.min.js dist and create a <script> element with the contents as children -- this would be extra bloat if not using mermaid. I'm not sure if there is a TS function in the inherited codebase to do that because the watchScript() function is only designed to create <script src="path/to/min.js"></script> elements.

Alternatively, I can just add the mermaid.js dist from cache to built docs' _static folder and simply change the path specified in the watchScript() arg. In this scenario, we would have to exclude the mermaid.min.js contents from the bundled JS though.

I have yet to look into how we could exclude the theme's mermaid-related JS code from the bundle. It definitely won't be needed when the mermaid ext is not enabled. This bit seems like a separate issue because there is other JS code that could be conditionally be excluded (eg not using all available html_theme_options["features"]).

About mathjax

This should be much easier since the upstream implementation is basically just telling the user to add the mathjax lib themselves. Sphinx already ships with mathjax support, so we may just need to document how the user can build docs that self-host the mathjax dist.

TBH, I'm not really sure there's anything in the inherited codebase that is specific to mathjax (aside from some CSS). So, I don't think mathjax support in our external resources ext is needed, rather it may be better managed by the doc project's author(s).

2bndy5 added a commit that referenced this issue Mar 2, 2023
In reference to #150, this uses the `external_resources` ext to cache the mermaid.js dist.

Users can still use `html_static_path` if downloading the dist at build time proves problematic.
2bndy5 added a commit that referenced this issue Mar 2, 2023
In reference to #150, this uses the `external_resources` ext to cache the mermaid.js dist.

Users can use `html_static_path` if downloading the dist at build time proves problematic.

BREAKING CHANGE: The `sphinx_immaterial.mermaid_diagrams` ext is now optional and must be enabled in conf.py to use the `md-mermaid` directive.
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 a pull request may close this issue.

1 participant