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

Remove CloudFlare CDN request #1256

Closed
wants to merge 5 commits into from

Conversation

shreyasminocha
Copy link

Similar in motivation to #847.

@shreyasminocha shreyasminocha changed the title Remove MathJax-related CF request Remove CloudFlare CDN request Jun 13, 2020
src/theme/index.hbs Outdated Show resolved Hide resolved
@Dylan-DPC-zz
Copy link

@shreyasminocha can you resolve the conflicts? thanks

@shreyasminocha
Copy link
Author

@Dylan-DPC done!

@ehuss
Copy link
Contributor

ehuss commented Jul 9, 2020

This doesn't seem to work. The file is not copied to the output. All theme files require special handling in code. Also, I would prefer if it wasn't copied if mathjax is disabled.

@shreyasminocha
Copy link
Author

I don't have much rust in me, but would duplicating and editing lines responsible for the other static files in src/theme/mod.rs and src/renderer/html_handlebars/hbs_renderer.rs work? @ehuss

@ehuss
Copy link
Contributor

ehuss commented Jul 9, 2020

Yea, I think that's roughly correct. In copy_static_files it should check if mathjax is enabled (I think there is a field in html_config).

@shreyasminocha
Copy link
Author

Thanks.

I ran it a couple of times locally and it seems to be working as expected now.

@ehuss
Copy link
Contributor

ehuss commented Sep 6, 2020

This still doesn't appear to work. The MathJax samples aren't rendering, and I see these errors on the console:

Loading failed for the <script> with source “http://localhost:3000/extensions/MathMenu.js?V=2.7.1”. mathjax.html:1:1
Loading failed for the <script> with source “http://localhost:3000/extensions/MathZoom.js?V=2.7.1”. mathjax.html:1:1

@unpavedmop
Copy link

i cant use mdbook until this is fixed. how did cf slip in after the google debacle?

those interested in this may also like:

rust-lang/rust#40552
rust-lang/rust#75263

@ssokolow
Copy link

ssokolow commented Feb 12, 2021

@unpavedmop Every fix takes them some time. (And, while I agree that I wouldn't want to be using any external resources, I don't use mdBook currently and, if I did, I have no need for MathJax, so I'm not going to be the one fixing it.)

You should be able to resolve the problem for yourself just by either turning off MathJax support for using the custom theme support to replace the built-in Handlebars template with one that uses a local copy of MathJax.

Heck, given how easy it is to build a Rust project from source, it should be trivial for you make a PR to fix the problem for all of us.

You got me curious, so I did a quick repository search for "cloudflare" using GitHub's search box. The offending line is inside an {{#if mathjax_support}} block in theme/index.hbs. Change that to use {{ path_to_root }} instead.

I then searched for something that already uses {{ path_to_root }}... in this case highlight.css, and it looks like you need to do the following to add another file to the bundled local resources:

  1. Add include_bytes! lines for the MathJax files to theme/mod.rs
  2. Add lines to copy_across_theme in book/init.rs to copy the data to output files.
  3. Add lines to copy_static_files in renderer/html_handlebars/hbs_renderer.rs to copy the data to output files. (I didn't dig deep enough to figure out why this has to be done in two places.)
  4. Add lines to the copy_theme test in tests/init.rs to test the functionality you added.
  5. Update format/theme/README.md to add the MathJax files to the list of bundled files that can be overridden.

As for the "may also like" links, I'm just going to pretend you didn't link those, because I have trouble finding a more favourable interpretation of that than "transparent high-school attempt to manipulate the developers into doing what you want".

@ssokolow
Copy link

ssokolow commented Feb 13, 2021

Oops. I didn't sleep well last night and thought this was an issue with someone asking for external MathJax support to be removed, not a PR providing a way to do it.

Given that I really don't like sites randomly relying on external domains (especially now that cache isolation makes external CDNs purely a liability) and it seems like a very ugly hack to make a custom theme that's identical to the default except for switching to local MathJax, could either this or a corresponding feature request be opened back up? (And, in the latter case, linked here so people can find the feature request more easily.)

If you're going to self-host the Google Fonts, it makes sense to be consistent and also self-host the MathJax.

@shreyasminocha
Copy link
Author

Yeah I don't think I'll personally get around to finishing this, but people are welcome to continue from here.

@stappersg
Copy link

stappersg commented Feb 13, 2021 via email

@ehuss ehuss added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Dec 20, 2021
@SeniorMars
Copy link

I just want to say thanks for working on this! I care a lot about my privacy, and I think it's important that we remove something like this.

@ehuss
Copy link
Contributor

ehuss commented Jan 15, 2023

I'm going to close since this has gone stale. If someone is willing to pick it back up, feel free to create a new PR.

@ehuss ehuss closed this Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants