feat: add SVGO optimization support for SVG assets #13880
Conversation
github-actions
bot
added
pkg: astro
CodSpeed Performance ReportMerging #13880 will not alter performanceComparing Summary
|
🦋 Changeset detectedLatest commit: 8986051 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. |
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!
| svg: z | ||
| .object({ | ||
| optimize: z.boolean().default(true), | ||
| svgoConfig: z.record(z.any()).optional(), |
There was a problem hiding this comment.
I think we should offer at least proper typing here even if we don’t do full schema validation. For example, if a type is available for the SVGO options, we could use something like:
svgoConfig: z.custom<SVGOOptions>((value) => value && typeof value === 'object').optional(),That way users should get a nice editor experience.
| * @description | ||
| * Whether to enable SVG optimization using SVGO during build time. | ||
| * | ||
| * When enabled, all imported SVG files will be optimized for smaller file sizes |
There was a problem hiding this comment.
all imported SVG files
Will this also apply to usage with Astro’s assets pipeline? For example usage like:
---
import { Image } from 'astro:assets';
import mySVG from './example.svg';
---
<Image src={mySVG} alt="Example SVG" />
|
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 |
azat-io
force-pushed
the
feat/svgo
branch
2 times, most recently
from
175772e to
f7104bc
Compare
|
Made fixes based on comments:
|
| svg?: { | ||
| /** | ||
| * @docs | ||
| * @name svg.optimize |
There was a problem hiding this comment.
| * @name svg.optimize | |
| * @name experimental.svg.optimize |
|
|
||
| /** | ||
| * @docs | ||
| * @name svg.svgoConfig |
There was a problem hiding this comment.
| * @name svg.svgoConfig | |
| * @name experimental.svg.svgoConfig |
|
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) |
azat-io
force-pushed
the
feat/svgo
branch
2 times, most recently
from
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? |
|
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.
| const result = optimize(contents, svgConfig.svgoConfig); | ||
| processedContents = result.data; | ||
| } catch (error) { | ||
| console.warn('SVGO optimization failed:', error); |
There was a problem hiding this comment.
Should we error with an AstroError instead of warning?
There was a problem hiding this comment.
Can you advise me on how to do this?
There was a problem hiding this comment.
Sure! You need to do 2 things basically:
- Create a new error data in
src/core/errors/error-data.ts(path might not be exact but pretty close). You can look atsrc/core/errors/README.mdfor guidance and at the rest of the TS file for practical examples - Then here you can throw an AstroError directly, something like this:
catch(cause) {
throw new AstroError(
AstroErrorData.SvgoOptimizationFailed,
{ cause }
)
}Let me know if you need any clarification
|
Thanks for another iteration of the review. What has been done:
|
| /** | ||
| * @docs | ||
| */ | ||
| export const CannotOptimizeSvg = { |
There was a problem hiding this comment.
@sarah11918 there's now an error that would need your review
There was a problem hiding this comment.
Some notes here:
- This error message should probably tell people what is going to happen and/or what they should do. I believe from the feature it will just fall back to an unoptimized SVG and there's nothing they can actually do about it? So, probably makes sense to additionally say that the original SVG will be used/building with the original SVG instead... something like that.
- I know sometimes we can say which file led to the error. If so, it's a nicety here to be able to say which SVG could not be optimized. This lets people try to do something with that file in particular, if they want. And, in that case, you could suggest optimizing it manually through something like SVGOMG, if desired, in the error message.
- These errors are all indexed on a single docs page in the order that they're listed here, and they are in categories. So, if there's a better category/placement, then we add new errors in that location in the file. We can't just stick new ones at the end or they end up in the last category. Right now the existing image errors are in the catch-all "Astro Errors" category, so it probably makes sense to put it in that one. (We could and maybe should have a separate images error section, but that's not the responsibility of this PR!)
| * When enabled, all imported SVG files will be optimized for smaller file sizes | ||
| * and better performance while maintaining visual quality. | ||
| */ | ||
| optimize?: boolean; |
There was a problem hiding this comment.
I think you brought this up sarah already, about directly having the config object as a root property instead of having optimize: boolean. What was the conclusion of that?
Because I think it could be nice to have config.experimental.svgo: boolean | SvgoConfig or something like that
There was a problem hiding this comment.
Done. ✅️️️️
Also made the corresponding changes to the pull request with documentation:
withastro/docs@3005899
9 participants
Changes
svg.optimizeandsvg.svgoConfigconfiguration optionsTesting
Added unit tests.
Benefits
Astro planned to add SVGO earlier:
#12067
Docs
I'll create another PR to docs.