Skip to content

chore: minor reduction to inline svg/js code #11317

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

Merged
merged 2 commits into from
Jul 15, 2025
Merged

Conversation

SethFalco
Copy link
Contributor

@SethFalco SethFalco commented Jul 10, 2025

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • N/A If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

I've been looking at the output of SVGO.dev and just trying to see where we could reduce the bundles further. (Switching to Preact, writing PostCSS plugins, hand-editing our SVGs, etc.)

These are two things I noticed during my review that were upstream. The changes are petty at best, but do reduce the inlined SVG/JS code by 0.25 KB and reduce the amount of JavaScript that is executed on the client. It's not much, but at least it's a quick win. ^-^'

Redundant XMLNS in SVG

Removes xmlns from the inlined sprite sheet icon.

There's no need to specify the XMLNS when inlining an SVG into an HTML document. HTML rules apply rather than XML rules! I believe this is a safe change, inline SVGs have worked without xmlns all the way back with IE 9! The SVGs in this screenshot don't have xmlns.

The HTML syntax does not support namespace declarations, even in foreign elements.)

For instance, consider the following HTML fragment:

<svg>
  <metadata>
  <!-- this is invalid -->
  <cdr:license xmlns:cdr="https://www.example.com/cdr/metadata" name="MIT"/>
 </metadata>
</svg>

The innermost element, cdr:license, is actually in the SVG namespace, as the "xmlns:cdr" attribute has no effect…

HTML Standard, also observe that they omit xmlns in the <svg> tag.

When using the HTML syntax, the namespace is provided automatically by the HTML parser.

Document Structure — SVG 2

  <body>
    <svg viewBox="0 0 100 100" preserveAspectRatio="xMidYMid slice" role="img">

SVG in HTML introduction - SVG | MDN, in MDN's documentation, they also omit the xmlns tag when talking about SVGs in the context of HTML.

Before vs. After in the Build Output (⬇️ 0.035 KB)

- <svg xmlns="http://www.w3.org/2000/svg" style="display: none;"><defs>
- <symbol id="theme-svg-external-link" viewBox="0 0 24 24"><path fill="currentColor" d="M21 13v10h-21v-19h12v2h-10v15h17v-8h2zm3-12h-10.988l4.035 4-6.977 7.07 2.828 2.828 6.977-7.07 4.125 4.172v-11z"/></symbol>
- </defs></svg>

+ <svg style="display: none;"><defs>
+ <symbol id="theme-svg-external-link" viewBox="0 0 24 24"><path fill="currentColor" d="M21 13v10h-21v-19h12v2h-10v15h17v-8h2zm3-12h-10.988l4.035 4-6.977 7.07 2.828 2.828 6.977-7.07 4.125 4.172v-11z"/></symbol>
+ </defs></svg>

As an aside, these are the same sources you can reference for the rationale behind enabling removeXMLNS by default in the issue for better SVGO defaults.

Reduce Theming JavaScript

This makes two changes:

  • The Docusaurus config is known at build time. So we should be able to evaluate some or most of these variables at build time. The implementation before inlined the config values in the client, then the client would calculate them instead.
  • If disableSwitch is enabled, there's no need to even inline the functions to read the theme setting from the URL or localStorage. The option to set it shouldn't be there anyway!

⚠️ This has one subtle change in behavior. Before, even if disableSwitch was enabled, it was still possible to force light mode through ?docusaurus-theme=light. The disableSwitch option would now disable both the button and the query parameter.

Before vs. After in the Build Output (⬇️ 0.206~ KB)

- function(){var t=function(){try{return new URLSearchParams(window.location.search).get("docusaurus-theme")}catch(t){}}()||function(){try{return window.localStorage.getItem("theme")}catch(t){}}();document.documentElement.setAttribute("data-theme",t||(window.matchMedia("(prefers-color-scheme: dark)").matches?"dark":"light")),document.documentElement.setAttribute("data-theme-choice",t||"system")}()
+ document.documentElement.setAttribute("data-theme",window.matchMedia("(prefers-color-scheme: dark)").matches?"dark":"light"),document.documentElement.setAttribute("data-theme-choice","system")

This may require consideration as it could be a breaking change. Depends on if the following is a supported scenario. A user sets disableSwitch to true, because they want to implement their own switch without swizzling, but they still wants Docusaurus to be responsible for the data-theme attribute. 🤔

Test Plan

Test links

  • XMLNS change, I tested it in SVGO.dev (locally). Not sure what would be the best automated test for it.
  • Color scheme change, I wrote snapshot tests to confirm that the output JavaScript makes sense, and to make it easy to check multiple states between changes. I've also applied this change in SVGO.dev (locally) and it works as intended. (Including changing OS theme while on the site, etc.)

Deploy preview: https://deploy-preview-11317--docusaurus-2.netlify.app/

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jul 10, 2025
Copy link

netlify bot commented Jul 10, 2025

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit f0e3abe
🔍 Latest deploy log https://app.netlify.com/projects/docusaurus-2/deploys/68712c4454aa150008830384
😎 Deploy Preview https://deploy-preview-11317--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@SethFalco SethFalco changed the title Chores chore: minor reduction to inline svg/js code Jul 10, 2025
Copy link

github-actions bot commented Jul 10, 2025

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO Report
/ 🔴 46 🟢 98 🟢 100 🟢 100 Report
/docs/installation 🟠 56 🟢 97 🟢 100 🟢 100 Report
/docs/category/getting-started 🟠 71 🟢 100 🟢 100 🟠 86 Report
/blog 🟠 58 🟢 96 🟢 100 🟠 86 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 59 🟢 92 🟢 100 🟢 100 Report
/blog/tags/release 🟠 60 🟢 96 🟢 100 🟠 86 Report
/blog/tags 🟠 70 🟢 100 🟢 100 🟠 86 Report

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Thanks

I think we can consider it's not a breaking change

Comment on lines 7 to 10
}


function getQueryStringTheme() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we reduce line breaks and useless indentation?

This doesn't matter much (if minimization is enabled), but it looks a bit messy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Let me know if you're fine with that!

I did it this way before as my aim was to keep the source clean rather than the output. Now that we've prioritized a clean output, I feel the source looks a bit messy now, though.

I'm starting to see the appeal to tc39/proposal-string-dedent.

@SethFalco SethFalco force-pushed the chores branch 4 times, most recently from 30de303 to 362c71a Compare July 11, 2025 15:21
@SethFalco SethFalco requested a review from slorber July 12, 2025 17:08
@slorber slorber added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Jul 15, 2025
@slorber
Copy link
Collaborator

slorber commented Jul 15, 2025

Thanks 👍

@slorber slorber merged commit 2adbc0d into facebook:main Jul 15, 2025
43 checks passed
@SethFalco SethFalco deleted the chores branch July 15, 2025 13:05
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: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants