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

Fetch all resources from a single CDN #893

Merged
merged 10 commits into from
Nov 20, 2018
Merged

Fetch all resources from a single CDN #893

merged 10 commits into from
Nov 20, 2018

Conversation

Bisaloo
Copy link
Contributor

@Bisaloo Bisaloo commented Nov 17, 2018

It should be more efficient to load all resources from a single CDN because DNS resolution, TLS handshake, etc. only need to be performed once.

I picked cloudflare because it looks like it was already providing the largest number of external resources.

Also, as cool side effects:

  • SRI hashes has been added in places they were previously missing, thus increasing security
  • Font Awesome has been updated from 4.6.3 to 4.7.0. Font Awesome 5.0+ should also be soon available and I can submit a PR with this change later.
  • Docsearch resources have been updated as well.

I tested the changes locally and everything seems to work fine

TODO:

  • Check if more libraries can be seamlessly updated
  • Update bootstrap

@Bisaloo Bisaloo changed the title Fetch all resources from a single CDN [WIP] Fetch all resources from a single CDN and update bootstrap (fix #884) Nov 17, 2018
@jayhesselberth
Copy link
Collaborator

The update to BS4 will be problematic - the layout changed substantially and required a lot of fidding to get it right. Probably better as a separate PR.

@Bisaloo
Copy link
Contributor Author

Bisaloo commented Nov 17, 2018

Yeah, you're right. I moved all changes related to bootstrap to a new branch (PR #894). This should be ready for review if you wish.

@Bisaloo Bisaloo changed the title [WIP] Fetch all resources from a single CDN and update bootstrap (fix #884) Fetch all resources from a single CDN Nov 17, 2018
@Bisaloo
Copy link
Contributor Author

Bisaloo commented Nov 17, 2018

Actually, there could be a base for keeping multiple CDN. If one resource is overwhelmingly fetched for one place (say for example that 90% of websites fetch bootstrap from maxcdn), most users should have the library cached.

But even in that case, the URLs should be updated to the canonical CDN for each library.

From what I have seen in my tests, this PR should still result in a speed up because the majority of the time for page load is spent in the DNS resolution and TLS negociation. That's just n = 1 but I'm not really sure how to collect better data to test this.

@hadley
Copy link
Member

hadley commented Nov 19, 2018

I think using the same CDN makes sense to me.

@dongzhuoer, @BruceZhaoR, @yiluheihei: I know CDN choice can make a difference for Chinese users. Do you have any concerns about switching all links to use cloudflare?

@dongzhuoer
Copy link

I haven't noticed the CDN. Maybe I need to use it for a while to see the performance.

@Bisaloo
Copy link
Contributor Author

Bisaloo commented Nov 20, 2018

Another possible criterion to choose the CDN: some users may use the browser extension decentraleyes, which locally mirrors popular CDN. It would probably be good to use on from its list of mirrored CDN.

@BruceZhaoR
Copy link

@hadley cloudflare works fine in China.
@Bisaloo There is an error in Chrome Console: TeX-AMS-MML_HTMLorMML.js:15 Uncaught ReferenceError: MathJax is not defined at TeX-AMS-MML_HTMLorMML.js:15 while the origin link
https://cdnjs.cloudflare.com/ajax/libs/mathjax/2.7.5/MathJax.js?config=TeX-AMS-MML_HTMLorMML works fine

I'm going to use deploy_site_github() and test it online.

@Bisaloo
Copy link
Contributor Author

Bisaloo commented Nov 20, 2018

@BruceZhaoR, good catch, thank you! It should be fixed now.

I also added a test page for mathjax to ensure that future changes don't break its functionality.

@jayhesselberth
Copy link
Collaborator

This all looks good. Adding a mathjax test vignette is a good idea.

@hadley
Copy link
Member

hadley commented Nov 20, 2018

Can you please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber).

@jayhesselberth jayhesselberth merged commit 59d3618 into r-lib:master Nov 20, 2018
@Bisaloo Bisaloo deleted the sri-hashes branch November 20, 2018 13:36
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.

5 participants