feat: add SVGO optimization support for SVG assets#13880
feat: add SVGO optimization support for SVG assets#13880florian-lefebvre merged 19 commits intowithastro:mainfrom
Conversation
CodSpeed Performance ReportMerging #13880 will not alter performanceComparing Summary
Footnotes |
🦋 Changeset detectedLatest commit: 3df9ea6 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks! This is a new feature, so we will need to discuss it a bit before merging, and it will need to wait for a minor release. There is a chance we will want to put this behind an experimental flag at first too. In the meantime, could you add a changeset? It's used to generate the changelog, so make sure you include enough detail to explain what it is and how it works for new users. Think of it as the first bit of docs. |
delucis
left a comment
There was a problem hiding this comment.
Thanks for the PR @azat-io! I think this could be a nice feature.
Left a couple of questions based on a quick look.
Also pinging @natemoo-re and @stramel as our resident SVG experts who might have good insights here!
|
Thanks for the PR! I do have some initial thoughts on this. This approach is similar to what we did in astro-icon which had some downsides and issues. This might also conflict with an idea @natemoo-re had about shifting the icons into a content-collection instead. I was also thinking about alternatives to this approach that doesn't tie us so tightly to svgo. Similar to the image optimization setup or offering a optimize function/file. Sharp offers SVG optimization as well. Another thought was allowing opt-in per SVG though that can get a bit hairy. Let me know if you have any deeper thoughts. I'll keep thinking about it as well. Interested to hear @natemoo-re 's opinions as well |
175772e to
f7104bc
Compare
|
Made fixes based on comments:
|
|
Should I create a pull request in the documentation? |
|
Rebased. |
|
Any feedback? |
|
Ping. |
1 similar comment
|
Ping. |
|
Hey @azat-io! We’re waiting on getting some more feedback from our SVG experts — apologies for the delay. In the meantime, it would be good to have an answer to my question here: #13880 (comment) |
446db9a to
8a6dd52
Compare
|
Rebased. |
|
Amazing stuff! I was looking for how to use svgo with astro today and came across this PR. Native support for svgo with a customizable svgo config would be extremely helpful. |
|
Hi @delucis, @stramel, @natemoo-re, Checking in - it's been 4 months since the last comment. All feedback has been addressed and the PR is ready for review. What's the status on this? |
Princesseuh
left a comment
There was a problem hiding this comment.
Overall looks good to me, thank you for contributing!
|
I am personally of the opinion that tying this to SVGO is good. SVGO has been the reference in SVG optimisation for honestly forever. Some other libraries do offer that feature too, but it's typically worse than SVGO so why bother. Additionally, I think it's not really worth the complexity added by supporting image service-like for this. |
|
I've made fixes based on your comments, rebased, and resolved conflicts. I also opened a new pull request for documentation: |
There was a problem hiding this comment.
Some comments on behalf of docs here, and thank you for the docs PR @azat-io !
-
Quick question (for everyone) on the experimental flag name.
svgis pretty general, and could mean anything about SVGs. (And, could create confusion if there's another experimental feature involving SVGs at the same time.) So just want to make sure this shouldn't be something likeexperimental.svgOptimizationfor clarity, and to let the user know exactly what they're signing up for! -
Also, wondering about the
optimizeproperty which as far as I can tell is basically just setting the flag true? This doesn't seem to match our other patterns that can be just enabled, or passed extra config, like CSP for example. You either set the experimental flag true to enable the basic behaviour, or you pass a config object (but you don't otherwise have a property that retains "turning on the flag" behaviour):
export default defineConfig({
experimental: {
csp: {
directives: [
"default-src 'self'",
"img-src 'self' https://images.cdn.example.com"
]
}
}
});
It feels weird to document "optimize" when it really just means "yes, use this flag" and is required for any of the other options to be set. (Which otherwise are just one object that could even be experimental.svg options directly?) Maybe this is future proofing for when it's not experimental and these will be higher-level options? But it feels clunkier than other patterns we've used as I'm looking at documenting it, so I'm just pointing it out because I'm noticing it!
Like, maybe there's a world where it's similarly just (i.e. the SVGO configuration object just is the config object):
export default defineConfig({
experimental: {
svgOptimize: {
plugins: [
'preset-default',
],
floatPrecision: 2,
}
}
})
Anyway, more pointing out this feels different, and want to make sure everything done here is intentional and a conscious choice! And of course, I am not the subject matter expert here, so maybe what I'm saying doesn't even make sense in this context.
|
Thanks for another iteration of the review. What has been done:
|
sarah11918
left a comment
There was a problem hiding this comment.
Changeset looks great! Thanks for this lovely addition! A few minor editing polishes from me, and we can continue the discussions about the other usage examples in the docs PR!
Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
|
I confirm we prefer throwing an error than logging a warning |
Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
Changes
svg.optimizeandsvg.svgoConfigconfiguration optionsTesting
Added unit tests.
Benefits
Astro planned to add SVGO earlier:
#12067
Docs
I'll create another PR to docs.