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

Convert Slides from cdnjs.cloudflare.com to JSDelivr CDN #2513

Merged
merged 2 commits into from Dec 23, 2021

Conversation

IceCodeNew
Copy link
Contributor

Signed-off-by: IceCodeNew 32576256+IceCodeNew@users.noreply.github.com

Purpose

This commit is meant to complete #2512, I managed to completely remove the needs of loading scripts from Cloudflare by these two changes.
Not sure the root of the problem, but when I was testing my site, the script failed to load from Cloudflare under some circumstance (the debug info shows that's the CORS problem).
That's basically why I am so actively hunting and eliminating the scripts that loaded from cdnjs.cloudflare.com. And I do think proposing such a change will not affect others in bad means.

@gcushen gcushen changed the title Eliminate cdnjs.cloudflare.com Convert Slides from cdnjs.cloudflare.com to JSDelivr CDN Nov 1, 2021
@gcushen
Copy link
Collaborator

gcushen commented Nov 14, 2021

@IceCodeNew this PR is currently incomplete and results in JS errors as all the paths, filenames, and SRIs are still based on CDNJS.

You can view all the path/filename/SRI changes needed for JSDelivr at https://www.jsdelivr.com/package/npm/reveal.js

Please can you fix the URIs and SRIs for the Reveal, and its plugins and themes, and then test it?

Thanks

@IceCodeNew
Copy link
Contributor Author

@IceCodeNew this PR is currently incomplete and results in JS errors as all the paths, filenames, and SRIs are still based on CDNJS.

You can view all the path/filename/SRI changes needed for JSDelivr at https://www.jsdelivr.com/package/npm/reveal.js

Please can you fix the URIs and SRIs for the Reveal, and its plugins and themes, and then test it?

Thanks

Apologies for missing that, I will investigate it ASAP.

@IceCodeNew

This comment has been minimized.

@IceCodeNew
Copy link
Contributor Author

IceCodeNew commented Nov 17, 2021

@IceCodeNew this PR is currently incomplete and results in JS errors as all the paths, filenames, and SRIs are still based on CDNJS.

You can view all the path/filename/SRI changes needed for JSDelivr at https://www.jsdelivr.com/package/npm/reveal.js

Please can you fix the URIs and SRIs for the Reveal, and its plugins and themes, and then test it?

Thanks

The reason you see a problem recently is probably because this PR had not been rebased to the latest upstream (As you just merged my another PR).
This PR relies on that merged one to be fully functional. And I just rebased this PR and force pushed, you can test it again.

@gcushen

@gcushen
Copy link
Collaborator

gcushen commented Nov 21, 2021

@IceCodeNew the issues are mentioned here: #2513 (comment) , force pushing doesn't fix any of those issues

If we change $cdn_url_reveal, then we also need to change all the URI asset paths, filenames, and SRIs as they are all different on JSDelivr (see https://www.jsdelivr.com/package/npm/reveal.js ). If you test the PR, you should see all the errors in the browser console.

@IceCodeNew
Copy link
Contributor Author

@IceCodeNew the issues are mentioned here: #2513 (comment) , force pushing doesn't fix any of those issues

If we change $cdn_url_reveal, then we also need to change all the URI asset paths, filenames, and SRIs as they are all different on JSDelivr (see https://www.jsdelivr.com/package/npm/reveal.js ). If you test the PR, you should see all the errors in the browser console.

I did manually craft the complete URLs of the reveal plugin mentioned in this file to see whether the change is proper. Maybe there are some I missed in other files.
It would be helpful if you can provide the error message you see and the way you test this PR. As we may have different acknowledge about how to test properly.

@gcushen
Copy link
Collaborator

gcushen commented Nov 25, 2021

Thanks for addressing this.

I think the remaining change we need to make is that you are using the JSDelivr minify-on-demand feature to generate minified versions of the plugin files but JSDelivr advise not to use SRI on dynamically generated files: https://www.jsdelivr.com/using-sri-with-dynamic-files

So let's remove the SRI from the plugins if we are going to use the dynamically generated minified files...

@gcushen
Copy link
Collaborator

gcushen commented Nov 25, 2021

Also, let's take this opportunity to update to Reveal 4.2: https://github.com/hakimel/reveal.js/releases

@rodrigoalcarazdelaosa
Copy link
Contributor

Also, let's take this opportunity to update to Reveal 4.2: https://github.com/hakimel/reveal.js/releases

It'd be nice if now the default Math typesetter could be MathJax 3, the same as we have in the rest of the theme

@gcushen
Copy link
Collaborator

gcushen commented Nov 25, 2021

Also, let's take this opportunity to update to Reveal 4.2: https://github.com/hakimel/reveal.js/releases

It'd be nice if now the default Math typesetter could be MathJax 3, the same as we have in the rest of the theme

Good suggestion, Rodri!

Here's the reference on how to use Mathjax3 in Reveal 4.2+: https://revealjs.com/math/

And here's the place where the Reveal initialisation would need updating: https://github.com/wowchemy/wowchemy-hugo-themes/blob/d26f01d5d6c3f13b1daa629ba60bfc6a3e71b924/wowchemy/assets/js/wowchemy-slides.js#L10

We can change RevealMath to RevealMath.MathJax3

@IceCodeNew
Copy link
Contributor Author

Also, let's take this opportunity to update to Reveal 4.2: https://github.com/hakimel/reveal.js/releases

Also, let's take this opportunity to update to Reveal 4.2: https://github.com/hakimel/reveal.js/releases

It'd be nice if now the default Math typesetter could be MathJax 3, the same as we have in the rest of the theme

I would like to do those in separate PRs.


So let's remove the SRI from the plugins if we are going to use the dynamically generated minified files...

Thanks for pointing it out. I will check every change in this PR, as JSdelivr will show a warning on the dynamically generated minified files. Where the warning didn't sound, the change there stays the same.

@gcushen
Copy link
Collaborator

gcushen commented Dec 21, 2021

@IceCodeNew any update on the remaining changes?

remove the SRI from the plugins if we are going to use the dynamically generated minified files

@IceCodeNew
Copy link
Contributor Author

IceCodeNew commented Dec 22, 2021

// Do NOT use SRI with dynamically generated files!
L27: https://cdn.jsdelivr.net/npm/reveal.js@4.1.0/dist/reveal.min.css

// Do NOT use SRI with dynamically generated files!
L29: https://cdn.jsdelivr.net/npm/reveal.js@4.1.0/dist/theme/black.min.css

// Do NOT use SRI with dynamically generated files!
L42: https://cdn.jsdelivr.net/npm/reveal.js@4.1.0/dist/reveal.min.js

// Do NOT use SRI with dynamically generated files!
L43: https://cdn.jsdelivr.net/npm/reveal.js@4.1.0/plugin/markdown/markdown.min.js

// Do NOT use SRI with dynamically generated files!
L44: https://cdn.jsdelivr.net/npm/reveal.js@4.1.0/plugin/highlight/highlight.min.js

// Do NOT use SRI with dynamically generated files!
L45: https://cdn.jsdelivr.net/npm/reveal.js@4.1.0/plugin/notes/notes.min.js

// Do NOT use SRI with dynamically generated files!
L46: https://cdn.jsdelivr.net/npm/reveal.js@4.1.0/plugin/search/search.min.js

// Do NOT use SRI with dynamically generated files!
L47: https://cdn.jsdelivr.net/npm/reveal.js@4.1.0/plugin/math/math.min.js

// Do NOT use SRI with dynamically generated files!
L48: https://cdn.jsdelivr.net/npm/reveal.js@4.1.0/plugin/zoom/zoom.min.js

// no warning
L52: https://cdn.jsdelivr.net/npm/reveal.js-menu@2.1.0/plugin.js

// no warning
L53: https://cdn.jsdelivr.net/npm/reveal.js-menu@2.1.0/menu.js

// no warning
L54: https://cdn.jsdelivr.net/npm/reveal.js-menu@2.1.0/menu.css

@IceCodeNew
Copy link
Contributor Author

@IceCodeNew any update on the remaining changes?

remove the SRI from the plugins if we are going to use the dynamically generated minified files

The remaining changes are just done ;-)

Signed-off-by: IceCodeNew <32576256+IceCodeNew@users.noreply.github.com>
Signed-off-by: IceCodeNew <32576256+IceCodeNew@users.noreply.github.com>
@gcushen
Copy link
Collaborator

gcushen commented Dec 23, 2021

@IceCodeNew 谢谢

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.

None yet

3 participants