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

Updated to SVGO 2 #565

Closed
wants to merge 1 commit into from
Closed

Conversation

underoot
Copy link

@underoot underoot commented Jun 8, 2021

Summary

Test plan

  • I ran tests (yarn test) after modifications and make changes according to changes in API of SVGO in cases where it was needed. Snapshots from tests stay almost untouched except of optimization of "Move to" operator in d attribute of path (d="M10 10 10 10" instead of d="M10 10L10 10")

@vercel
Copy link

vercel bot commented Jun 8, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/gregberge/svgr/HfGgtHN1Js67wpd4YUpVLs9oeMpX
✅ Preview: https://svgr-git-fork-underoot-update-to-svgo-2-gregberge.vercel.app

@codecov
Copy link

codecov bot commented Jun 8, 2021

Codecov Report

Merging #565 (2a8eb01) into main (b360117) will increase coverage by 1.90%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #565      +/-   ##
==========================================
+ Coverage   87.19%   89.09%   +1.90%     
==========================================
  Files          31       31              
  Lines         570      541      -29     
  Branches      160      151       -9     
==========================================
- Hits          497      482      -15     
+ Misses         58       48      -10     
+ Partials       15       11       -4     
Impacted Files Coverage Δ
packages/core/src/config.js 100.00% <ø> (ø)
packages/plugin-svgo/src/config.js 77.27% <100.00%> (+1.08%) ⬆️
packages/plugin-svgo/src/index.js 100.00% <100.00%> (+33.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b360117...2a8eb01. Read the comment docs.

return array
}, [])
return configs
.flatMap(extractPlugins)
Copy link

@r21meghashyam r21meghashyam Jun 15, 2021

Choose a reason for hiding this comment

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

Node 10 doesn't seem to support flatMap
Consider changing this to:
.reduce((configsArr,config)=>configsArr.concat(extractPlugins(config)),[])

Refference: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/flatMap

Else, you would need to remove compatibility to Node 10.

Copy link
Contributor

Choose a reason for hiding this comment

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

Else, you would need to remove compatibility to Node 10.

You need to do this anyway:

So while the 2.0 release in Feb 2021 mentions needing 10.13.x or newer, in this case to resolve this issue (as it appears to be NodeJS related not specifically svgo, you need NodeJS 11.x.x or newer

Otherwise users may risk breakage as the linked issue describes and my investigation revealed was due to issues with NodeJS 10.

@@ -28,6 +28,6 @@
"dependencies": {
"cosmiconfig": "^7.0.0",
"deepmerge": "^4.2.2",
"svgo": "^1.2.2"
"svgo": "^2.3.0"

Choose a reason for hiding this comment

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

Could you update this to ^2.3.1 so that it incorporates the css-select vulnerability fix?

@btd
Copy link
Contributor

btd commented Jun 30, 2021

You will need to update travis ci config to remove Node 10, also i would suggest to update everywhere engines in package.json. Anyway this will cause major version, so it makes sense to incorporate as much changes as possible.

@stale
Copy link

stale bot commented Aug 29, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Aug 29, 2021
@renchap
Copy link

renchap commented Aug 30, 2021

This should still be merged

@stale stale bot removed the wontfix label Aug 30, 2021
@underoot
Copy link
Author

Yep, I think that I'll find time on current week to deal with comments

@@ -13,7 +13,7 @@ export const DEFAULT_CONFIG = {
replaceAttrValues: null,
svgProps: null,
svgo: true,
svgoConfig: null,
svgoConfig: { plugins: [{ name: 'convertStyleToAttrs', active: true }]},
Copy link
Contributor

Choose a reason for hiding this comment

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

This plugin is error prone and was disabled because of it. I don't recommend to enable it.

@@ -1,3 +1,4 @@
import { extendDefaultPlugins } from 'svgo';
Copy link
Contributor

Choose a reason for hiding this comment

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

The latest svgo version provides preset-default plugin. extendDefaultPlugins is deprecated and will trigger warning.

@gregberge
Copy link
Owner

Fixed in #591

@gregberge gregberge closed this Sep 6, 2021
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.

Update SVGO to v2
8 participants