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

refactor: self-host KaTeX assets #6429

Merged
merged 11 commits into from
Jan 27, 2022
Merged

refactor: self-host KaTeX assets #6429

merged 11 commits into from
Jan 27, 2022

Conversation

pranabdas
Copy link
Contributor

@pranabdas pranabdas commented Jan 21, 2022

Motivation

Self-host KaTeX stylesheet and fonts instead of using external CDN. Total asset size increases about 328 KB. Please see related discussions here.

CC: @slorber

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

Deploy preview.

Related PRs

#4821

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jan 21, 2022
@@ -73,11 +73,9 @@ const config = {
trailingSlash: isDeployPreview,
stylesheets: [
{
href: 'https://cdn.jsdelivr.net/npm/[email protected]/dist/katex.min.css',
// style lint fails for katex.min.css
Copy link
Collaborator

@Josh-Cena Josh-Cena Jan 21, 2022

Choose a reason for hiding this comment

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

If stylelint fails, add it to the ignore file and commit with --no-verify. I have no idea why stylelint isn't respecting the ignore option in priority to the files passed to it during precommit—eslint/prettier are able to do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Not much of a difference using the minified css though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we do get a size reduction? Even if it's just a few KBs? Since we never touch the CSS, no need to keep the original :P

@netlify
Copy link

netlify bot commented Jan 21, 2022

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 0c25d93

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61ecb9656a225d0007ffa609

😎 Browse the preview: https://deploy-preview-6429--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Jan 21, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 60
🟢 Accessibility 100
🟢 Best practices 93
🟢 SEO 100
🟢 PWA 92

Lighthouse ran on https://deploy-preview-6429--docusaurus-2.netlify.app/

@Josh-Cena
Copy link
Collaborator

Not sure if we need so many fonts. If we never use things like \mathfrak, maybe the font files can go, or be replaced with a placeholder file? Is it possible to trick the CSS? 🤔

@Josh-Cena
Copy link
Collaborator

Of course, I'm not in favor of this move for the reasons you outlined in the docs (cache, request load, our bundle size...) but since Sebastien wants it I'll respect that😄

@Josh-Cena Josh-Cena added the pr: documentation This PR works on the website or other text documents in the repo. label Jan 21, 2022
@pranabdas
Copy link
Contributor Author

I think without bundled katex fonts, they are replaced with user installed fonts. But sometimes equations/symbols does not look good without proper fonts (sometime they don't scale proportionately, spacing not exact etc.). To ensure full LaTeX compatibility, probably we need all fonts. But there are .ttf, .woff, and .woff2 versions. We can perhaps include .woff2 version only. I think old browsers may not support it, so .woff for legacy usage, and .ttf are not generally required for web usage.

@Josh-Cena
Copy link
Collaborator

We don't support old browsers anyways. Our website doesn't polyfill flatMap so it doesn't even work on Chrome <68. woff2 is compatible enough for the browsers we target.

As for font rendering, things like SansSerif and Fraktur are never used on our website so they are safe to remove.


Loading stylesheets, fonts, and javascript libraries from CDN sources is a good practice for popular libraries and assets. Provided such assets are widely used across various websites, those assets will not be downloaded every time we visit a new website that uses the same assets, rather they would be cached by the browser reducing network load. In case you prefer to self-host the `katex.min.css` (along with required KaTeX fonts), you can download the latest version from [KaTeX GitHub releases](https://github.com/KaTeX/KaTeX/releases). Extract and copy `katex.min.css` and `fonts` directory (only `.woff2` font types should be enough) to docusaurus `static` directory. And in `docusaurus.config.js`, replace the css `href` from CDN url to your local path (say, `/katex/katex.css`).

````js title="docusaurus.config.js"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrong number of ticks :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Thank you :D

@slorber
Copy link
Collaborator

slorber commented Jan 21, 2022

The preview does not seem to work, I think it's because of the .min.css

https://deploy-preview-6429--docusaurus-2.netlify.app/docs/markdown-features/math-equations/

Also there's a stylelint copyright issue, you should add this file in .stylelintignore or at the top of the CSS file:

/* stylelint-disable docusaurus/copyright-header */

.eslintignore Outdated
@@ -17,3 +17,4 @@ copyUntypedFiles.mjs

packages/create-docusaurus/lib/*
packages/create-docusaurus/templates/facebook/.eslintrc.js
website/static/katex/katex.min.css
Copy link
Collaborator

Choose a reason for hiding this comment

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

in .stylelintignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Full file path in .stylelintignore seems to be problematic.

@Josh-Cena
Copy link
Collaborator

https://www.stefanjudis.com/notes/say-goodbye-to-resource-caching-across-sites-and-domains/ Oh, okay... I feel less strongly about using CDNs now. Particularly if we can tailor the bundle ourselves to make it smaller.

@slorber
Copy link
Collaborator

slorber commented Jan 27, 2022

thanks, looks like it works fine 👍

@slorber slorber merged commit f415da1 into facebook:main Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: documentation This PR works on the website or other text documents in the repo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants