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

add mermaid.js and mathjax to npm deps #231

Merged
merged 12 commits into from
May 9, 2023
Merged

add mermaid.js and mathjax to npm deps #231

merged 12 commits into from
May 9, 2023

Conversation

2bndy5
Copy link
Collaborator

@2bndy5 2bndy5 commented Mar 2, 2023

  • mermaid dist is copied to built docs' static files.
  • mathjax dist is copied to built docs' static files if the sphinx.ext.mathjax is enabled.
    mathjax script tag attributes intentionally exclude async.

resolves #150

@jbms
Copy link
Owner

jbms commented Mar 3, 2023

I think rather than download mermaid.js when the docs are built, we could just add it as a dependency to packages.json and then include it in the built package / wheel with the other javascript bundles (but not bundle it). We can do the same thing for mathjax.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 3, 2023

I had considered that as well, but I wasn't sure if changing the package.json was a welcome change. I'll rework this and get back to you...

I'm still not convinced that the mathjax lib needs to be handled by this theme. Sphinx' mathjax_path can be used by users to do the same thing that was suggested. Maybe I'm missing something.

@2bndy5 2bndy5 marked this pull request as draft March 3, 2023 04:39
@jbms
Copy link
Owner

jbms commented Mar 3, 2023

It is true that mathjax isn't really related to this theme. Perhaps it would be better if Sphinx itself included a version of mathjax so that there is no dependency on external resources by default. But I'm not sure how likely that is to go through...

Particularly since we may soon provide a way to use mathml instead, I think it is reasonable to just ignore mathjax for now...

But in general providing a way to avoid any dependence on external resources is a useful feature, and one that can be a bit annoying for users to implement themselves.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 3, 2023

Perhaps it would be better if Sphinx itself included a version of mathjax so that there is no dependency on external resources by default

Oh, you're focused on the default behavior. 🤦🏼 That would be so easy to just change the default value of mathjax_path if the config has the attr defined... I'm not sure where we'd document this adjustment. Maybe it can be mentioned with the solution for #233 .

@2bndy5 2bndy5 changed the title use mermaid.min.js from local build add mermaid.js and mathjax to npm deps Mar 3, 2023
@2bndy5 2bndy5 marked this pull request as ready for review March 3, 2023 10:21
@2bndy5 2bndy5 requested a review from jbms April 21, 2023 17:50
@2bndy5 2bndy5 force-pushed the cache-mermaid.js branch 2 times, most recently from d5f38b8 to 19cc0ab Compare April 26, 2023 06:09
@2bndy5 2bndy5 force-pushed the cache-mermaid.js branch 2 times, most recently from f1d18e7 to 7ae47f8 Compare April 27, 2023 07:21
@jbms
Copy link
Owner

jbms commented Apr 28, 2023

Regarding mathjax, now that all major browsers support MathML, it may make sense to explore whether we can just recommend using mathml instead of mathjax, since if it works, it would require no additional libraries and would render without a delay.

This comment describes how it might simply require modifying the docutils.conf file in the doc root directory:

sphinx-doc/sphinx#6092 (comment)

Not sure if anyone has actually tested that, though.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Apr 28, 2023

I was going to approach the #233 separately. The newer MathML feature does look much more appealing, but this work is more like retrofitting when it comes to mathjax use. The sphinx dev cycle isn't too slow (for the most part), so hopefully there will be more development/testing with MathML in Sphinx by the time we get around to #233 .

@jbms
Copy link
Owner

jbms commented May 7, 2023

Can you also ensure that the licenses for these two packages are added to the generated license file (see tools/buil.d/transform/index.ts),; the existing logic only includes packages that are bundled by esbuild.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented May 7, 2023

Can you also ensure that the licenses for these two packages are added to the generated license file

The mermaid dist includes a copy of the License alongside the mermaid.min.js* files. Did you want to keep that in the bundles folder as well? Or is it sufficient to have it only in the generated LICENSE file?

@jbms
Copy link
Owner

jbms commented May 7, 2023

Can you also ensure that the licenses for these two packages are added to the generated license file

The mermaid dist includes a copy of the License alongside the mermaid.min.js* files. Did you want to keep that in the bundles folder as well? Or is it sufficient to have it only in the generated LICENSE file?

Keeping a copy of the license there as well makes sense.

Copy link
Owner

@jbms jbms left a comment

Choose a reason for hiding this comment

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

Thanks!

@2bndy5 2bndy5 merged commit 951e384 into main May 9, 2023
@2bndy5 2bndy5 deleted the cache-mermaid.js branch May 9, 2023 23:58
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.

caching (locally) npm pkgs' dist at docs build stage
2 participants