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

fix(gatsby): postcss-svgo - remove plugin removeAttrs #33266

Merged
merged 2 commits into from
Oct 4, 2021

Conversation

samouss
Copy link
Contributor

@samouss samouss commented Sep 21, 2021

This PR removes the plugin removeAttrs for postcss-svgo. The main reason behind this change is to remove warnings while building a Gatsby site with SVGs (see below). The removeAttrs plugin expect a pattern to remove matching attributes from the SVG, if none is provided the plugin is useless (see svg/svgo#1582).

With the current version of svgo:

$ gatsby build
[...]
warn Css Minimizer Plugin: postcss-svgo:
/Users/samuel.vaillant/Work/gatsby-svgo/styles.d07168f9038428354f1a.css:2:3: TypeError:
Cannot read properties of undefined (reading 'includes')
[...]
✨  Done in 19.87s.

With the "next" version (if the PR is merged):

$ gatsby build
[...]
Warning: The plugin "removeAttrs" requires the "attrs" parameter.
It should have a pattern to remove, otherwise the plugin is a noop.
Config example:

plugins: [
  {
    name: "removeAttrs",
    params: {
      attrs: "(fill|stroke)"
    }
  }
]
[...]
✨  Done in 22.70s.

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 21, 2021
@LekoArts LekoArts added topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Sep 21, 2021
@LekoArts LekoArts changed the title refactor(gatsby): webpack: postcss-svgo: remove plugin removeAttrs fix(gatsby): postcss-svgo - remove plugin removeAttrs Sep 21, 2021
@samouss
Copy link
Contributor Author

samouss commented Sep 21, 2021

I don't think the CI failure is related to this PR though... It looks like other PRs have the exact same error.

@TrySound
Copy link

Better use preset-default instead of long list

@natapg
Copy link

natapg commented Sep 24, 2021

I am having this problem in my project after updating to gatsby 3, is there a way to fix it momentarily or should I wait for these changes to be released? Thanks

8:26:20 PM: warning Css Minimizer Plugin: postcss-svgo: /opt/build/repo/styles.dbf98c38294bf5d76d0e.css:9:4512: TypeError: Cannot read property 'includes' of undefined
8:26:20 PM: warning Css Minimizer Plugin: postcss-svgo: /opt/build/repo/styles.dbf98c38294bf5d76d0e.css:9:4975: TypeError: Cannot read property 'includes' of undefined

@samouss
Copy link
Contributor Author

samouss commented Sep 24, 2021

The package svgo is used by Gatsby as a dependency of another dependency of another dependency...

$ yarn why svgo
yarn why v1.22.5
[1/4] 🤔  Why do we have the module "svgo"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "svgo@2.6.1"
info Reasons this module exists
   - "gatsby#css-minimizer-webpack-plugin#cssnano#cssnano-preset-default#postcss-svgo" depends on it
   - Hoisted from "gatsby#css-minimizer-webpack-plugin#cssnano#cssnano-preset-default#postcss-svgo#svgo"
info Disk size without dependencies: "1.27MB"
info Disk size with unique dependencies: "3.72MB"
info Disk size with transitive dependencies: "5.64MB"
info Number of shared dependencies: 12
✨  Done in 0.63s.

The plugin is a noop without the attrs attribute so the error should be no arm. If you really want to remove the error from the build we have to wait for a new version of the svgo package AND manually update the package in the Gatsby project. Until the package is released we can use the version 2.4.0 of svgo. Here is how to do it with Yarn resolutions feature (don't forget to re-install the packages to pickup the changes):

{
  "resolutions": {
    "gatsby/**/svgo": "2.4.0"
  }
}

Unfortunately, with npm, it looks like a bit more complicated to do. There is a RFC for a similar feature than resolutions but it's not yet implemented AFAIK. You can probably edit the shrinkwrap to force a dedicated version (but I didn't test it). Hope that helps!

Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

Thanks!

@LekoArts LekoArts merged commit a1f35ca into gatsbyjs:master Oct 4, 2021
@samouss samouss deleted the refactor/postcss-remove-attrs branch October 4, 2021 12:19
vladar pushed a commit that referenced this pull request Oct 5, 2021
Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>
(cherry picked from commit a1f35ca)
vladar added a commit that referenced this pull request Oct 5, 2021
Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>
(cherry picked from commit a1f35ca)

Co-authored-by: Samuel Vaillant <samuel.vllnt@gmail.com>
Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com>
wardpeet pushed a commit to herecydev/gatsby that referenced this pull request Oct 29, 2021
Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>
pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants