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

chore: load or create svgo.config.js #10211

Closed
wants to merge 2 commits into from
Closed

Conversation

SethFalco
Copy link
Contributor

@SethFalco SethFalco commented Jun 10, 2024

Note: This is a draft! It does work, and all tests are passing, but I'm not very familiar with the repository and wary that I haven't handled loading files and applying them to the webpack configuration correctly. (Or if you're even ok with that approach!)

This is mostly intended to be a starting point, any feedback or mentorship would be appreciated! Once we're happy with the implementation and interface, I can introduce new tests and documentation.

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • [TODO] If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • 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

There have been discussions about introducing an easier way for users to configure SVGO. This is an attempt to expose it similarly to how SVGO itself and SVGR does.

  • SVGO looks for a svgo.config.js (or .mjs/.cjs) file.
  • SVGR supports and recommends using a svgo.config.js file, though… based on the docs I don't know how. 😓

This PR makes it so if a svgo.config.js file is present in the site directory, it loads it instead of using what Docusaurus defines. If it is not present, Docusaurus uses a default config which is the same as before.

Changes

  • loadFreshModule is no longer async as it never called await internally anyway, this allowed me to use it in getFileLoaderUtils without much fuss.
  • when Docusaurus processes files, writes svgo.config.js to the generated files
  • when invoking getFileLoaderUtils, reads svgo.config.js from generated files and passes that config to SVGO.

Test Plan

Test links

Before

This is how the dogfooding page looked originally, and still looks when no svgo.config.js file is present. Docusaurus defines a default config if one isn't defined otherwise.

After

I now created a svgo.config.js file with the following content:

website/svgo.config.js

const path = require('path');

module.exports = {
  plugins: [
    {
      name: 'preset-default',
      params: {
        overrides: {
          removeTitle: false,
          removeViewBox: false,
        },
      },
    },
    {
      name: 'prefixIds',
      params: {
        delim: '',
        prefix: (_, info) => path.parse(info.path).name,
      },
    },
  ],
};

This is how the dogfooding page looks with the config applied. We load the config from the generated files, which is written to while building:

Deploy preview: https://deploy-preview-10211--docusaurus-2.netlify.app/tests/docs/tests/svgs/

Related issues/PRs


Edit: I also just noticed #9192. No promises, but I'll give it a peek and see if this is something I can handle in the near future.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 10, 2024
Copy link

netlify bot commented Jun 10, 2024

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 6398ae8
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/66677a38a437600008bf777d
😎 Deploy Preview https://deploy-preview-10211--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 site configuration.

Copy link

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 67 🟢 98 🟢 96 🟢 100 🟠 88 Report
/docs/installation 🟠 65 🟢 96 🟢 100 🟢 100 🟠 88 Report
/docs/category/getting-started 🟠 74 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog 🟠 70 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 64 🟢 96 🟢 100 🟢 100 🟠 88 Report
/blog/tags/release 🟠 67 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog/tags 🟠 73 🟢 100 🟢 100 🟢 90 🟠 88 Report

@slorber
Copy link
Collaborator

slorber commented Nov 14, 2024

Thanks for your PR @SethFalco and really sorry for the review delay, was busy on something else.

I've decided to not merge it because the technical solution is not ideal.

There's a much cleaner solution: create a first-class SVGR plugin, implemented in #10677.

When SVGR is passed svgoConfig: undefined, it will look for a .svgo.config.js file on its own, so this gives us maximum flexibility. We keep retro-compatibility but let users opt-in for SVGR/SVGO config files if they want to, through preset options.

I've used your dogfood page/svg to ensure that it works and we are able to resolve the conflicts on our own site.

@SethFalco
Copy link
Contributor Author

Yeah, no problem! I agree your solution is much better.
Happy to close this! 👍🏽

@slorber
Copy link
Collaborator

slorber commented Nov 14, 2024

Thanks!

BTW I created a dedicated issue, if you want to help us improve our default SVGO config, let's discuss it here: #10679

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple inline SVGs on a page can clash
3 participants