Remove non-stable SVG features #13488

stramel · 2025-03-22T04:57:17Z

Changes

After discussing with @natemoo-re,

Sprite Mode: We decided it was better to push this off until later. Inlining SVGs alone is a big win for Astro and there are some hurdles to creating a good DX without footguns in adding sprites.

Size Prop: We decided to remove this all together as it is pretty opinionated.

Title Prop: We're good with holding off on this until later as well considering there is a good discussion about how this should be implemented by default. Removing this allows users to control the a11y while we work to provide a better DX.

Default Role Attribute: Similar to above, it isn't obvious what we should apply by default. So we'll remove this in favor of a better solution down the road.

Memory Issue: I highly suspect this was tied to Sprite mode but I can look into it more if it still persists.

Additionally, I added a SVGComponent type that is equivalent to AstroComponentFactory to provide a better DX for passing SVGs down to other Astro components.

References withastro/roadmap#1035

Testing

Ran tests

Docs

Will need to update the docs but will need some help

/cc @withastro/maintainers-docs for feedback!

changeset-bot · 2025-03-22T04:57:22Z

🦋 Changeset detected

Latest commit: 41f1e0b

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

codspeed-hq · 2025-03-22T05:09:56Z

CodSpeed Performance Report

Merging #13488 will not alter performance

_{Comparing stramel:svg-cleanup (41f1e0b) with main (c43bf8c)}

Summary

✅ 6 untouched benchmarks

natemoo-re

LGTM! Thanks for grabbing time to talk through this and quick follow-up! 🎉

natemoo-re · 2025-03-23T14:34:59Z

packages/astro/src/assets/runtime.ts

 */
 const ATTRS_TO_DROP = ['xmlns', 'xmlns:xlink', 'version'];
-const DEFAULT_ATTRS: SvgAttributes = { role: 'img' };
+const DEFAULT_ATTRS: SvgAttributes = {};


Just calling out this change! Probably the right move given the other changes, but I'd still like to provide good accessibility defaults where we can.

For sure, we definitely need to follow up on this. Just removing for the initial release as there isn't consensus on what the best approach is yet

ematipico

Thank you! Let's update the changeset to follow the contribution guide.

Also, regarding the memory leak, the malicious code is here:

https://github.com/withastro/astro/blob/main/packages/astro/src/assets/utils/node/emitAsset.ts#L56-L60

I'm not sure if it's directly related to the sprite mode. What we have seen is that a Starlight website that uses a lot of SVG was growing in memory, without control. The experimental SVG feature wasn't enabled.

.changeset/little-steaks-rule.md

sarah11918

Just making some suggestions with what the changeset could look like here! As Ema mentioned, we do have a standard that we follow to let people know that this contains breaking changes if (and only if) you are using the experimental feature. And since that's the case, we really need to guide people through updating to compatible code (suggestions included! make them accurate!)

Commenting for the Docs changes needed in a PR, if I'm reading this right, it looks like it will be sufficient to:

delete the section on size (it simply won't exist at this point)
delete the section on the mode component attribute (if mode="sprite" can't be set, then there's no other value it could take? I'm assuming everything will be mode="inline" not just default, but as the only option? In that case, we wouldn't show mode at all)
delete the entry for experimental.svg.mode - similarly, it can't be set, so no need to show it there.
maybe document the SVGComponent type? I'll ask @ArmandPhilippot whether this feels necessary here!

Thanks for your help getting this right, and helping experimental users update from what they might already have! 🙌

.changeset/little-steaks-rule.md

ArmandPhilippot · 2025-03-24T18:23:02Z

@sarah11918 We don't document AstroComponentFactory and SVGComponent is only an alias, so I'm not sure. Last time I used AstroComponentFactory was to make Typescript happy (with an unusual setup), not because this adds something like autocomplete.

I'm not familiar with this experimental feature so I'm a little surprised by the example shown in the link. My assumption was that the imported SVG is a regular Astro component so it should be used with a slot and not as prop. If so, I'm not sure when this type might be useful. If it's only for uncommon setups, maybe we can wait until someone asks to document it? 😅

stramel · 2025-03-31T22:07:52Z

Just making some suggestions with what the changeset could look like here! As Ema mentioned, we do have a standard that we follow to let people know that this contains breaking changes if (and only if) you are using the experimental feature. And since that's the case, we really need to guide people through updating to compatible code (suggestions included! make them accurate!)

Commenting for the Docs changes needed in a PR, if I'm reading this right, it looks like it will be sufficient to:

delete the section on size (it simply won't exist at this point)

delete the section on the mode component attribute (if mode="sprite" can't be set, then there's no other value it could take? I'm assuming everything will be mode="inline" not just default, but as the only option? In that case, we wouldn't show mode at all)

delete the entry for experimental.svg.mode - similarly, it can't be set, so no need to show it there.

maybe document the SVGComponent type? I'll ask @ArmandPhilippot whether this feels necessary here!

Thanks for your help getting this right, and helping experimental users update from what they might already have! 🙌

@sarah11918 @ematipico Thanks for the guidance on the changeset! I have updated based on the feedback.

WRT the Docs, you're correct on all points. I have removed the SVGComponent type as @ArmandPhilippot pointed out it is unnecessary.

ematipico

Code wise, it looks good, but the changeset is incorrect. The feature is still experimental, we just removed some stuff.

ematipico · 2025-04-01T12:09:46Z

.changeset/little-steaks-rule.md

+'astro': patch
+---
+
+The SVG import feature introduced behind a flag in [v5.0.0](https://github.com/withastro/astro/blob/main/packages/astro/CHANGELOG.md#500) is no longer experimental and is available for general use.


The feature is still experimental, judging by the code. I think, maybe, there's been a misunderstanding on how to reword the changeset. Here's the relevant paragraph: https://contribute.docs.astro.build/docs-for-code-changes/experimental-feature-docs/#breaking-changes-while-still-experimental

We just need to add at the top of the changeset the following:

**BREAKING CHANGE to the experimental SVG configuration only**

Oh my gosh, how embarrassing 😂 Sorry for that! I'll get that updated today

Just checking @ematipico -- taking this out of experimental might be a plan for Launch week? Which now would be our next minor release. So, not sure whether this PR is intended as a patch first, or whether this will merge with the next minor at launch.

But yes, either the code needs to catch up to that, or the changeset needs to only discuss the remove the features. I'll wait to polish up the changeset until we have decided what the goal of this PR is!

I went ahead and pushed the breaking changes changeset instead. If we need to ready for stable, we can revert that commit.

@sarah11918 that's the plan, but that's not for this PR. We still have a memory leak that we need to fix, and if we can't fix it, we can't make the feature stable.

Do you mind tagging that issue for me here? I'm having trouble finding where you pointed it out.

@stramel There isn't an issue for that, but there's a Discord thread: https://discord.com/channels/830184174198718474/1349441804193239150

This was caught by a contributor of ours who works at Cloudflare. They tried to upgrade their Astro and Starlight dependencies. They didn't use the SVG experimental feature, however we idenfied that the issue was coming from code that was changed for the SVG support.

I believe Cloudflare project uses many SVG files, and I believe Astro was parsing/saving them exponentially. This is just a gut feeling.

ematipico

Looks good code wise. @sarah11918 are you happy with the changeset?

sarah11918

Looks great, glad we got that sorted! Just added some extra "padding" to the text in a few places to give explicit instruction as to code changes needed as well as some gentle editing.

Also noting: I don't see a PR to docs to update the reference material yet. Is this something on your radar, @stramel ?

.changeset/little-steaks-rule.md

Co-authored-by: Sarah Rainsberger <[email protected]>

ADTC · 2025-04-07T07:23:31Z

Wait, what?

I added a SVGComponent type that is equivalent to AstroComponentFactory to provide a better DX for passing SVGs down to other Astro components.

and then:

I have removed the SVGComponent type as @ArmandPhilippot pointed out it is unnecessary.

~~So is it there or it's not there? If it's there, how do I use it? 🫤~~ Seems like it was removed, which is sad. @stramel

ADTC · 2025-04-07T07:40:19Z

Re: SVGComponent type:

@sarah11918 it's not an internal implementation. It's meant to improve the developer experience. I specifically requested for it, so that SVGs can be passed as props and be type-checked correctly in actual projects.

@ArmandPhilippot we treat SVG files as vector images, so since there's an ImageMetadata type for raster images (JPG, PNG, etc.) and we can pass them as props to components, I think it makes much sense to be able to pass SVG images as props as well. This is the reason for requesting the SVGComponent type.

Currently there's no easy way to type a prop that accepts SVG other than this: ((_props: any) => any) & ImageMetadata. The fact that it is indeed an image with some additional (_props: any) speaks to the fact that many developers treat SVG as vector images that can be passed around, similar to raster images. It's rather unusual to consider SVGs as Astro components that should be inserted in slots. We have never once done so in our projects.

It's also helpful to type-check dynamically set SVG vector images in data structures. All these I have explained in more detail in my recent comments in Roadmap #1035.

FWIW, currently I'm using this to define it manually in our type definitions file. Perhaps this is better than aliasing AstroComponentFactory type:

type SVGComponent = typeof import('astro/client/*.svg').default

ArmandPhilippot · 2025-04-07T12:34:37Z

@ADTC While the following works:

<SVGComponentAsProp icon={SVGIcon} />
<ComponentAsProp child={RegularComponent} />

This doesn't:

<ComponentAsProp child={<RegularComponent prop="foo" />} />

So, for me, this is not the recommended way to use components. A slot (either default or a named one) should be used instead.
With this experimental feature, the imported SVG is a component. IMO, the same rule should be applied. So I don't see why this would be "unusual"... Passing an object (ImageMetadata as in your example for jpg files) and a component is not the same thing.

So what I was saying is: I don't think if an SVGComponent type is defined, we should necessarily document it because that's not the usage we recommend and I don't see a recommended way to use it.
Since then, the type has been removed but you are free to define your own type alias if you need it!

ADTC · 2025-04-07T14:52:45Z

@stramel so can we re-add the removed type? @ArmandPhilippot only asked to remove the documentation, not the type itself. The type itself is useful, and since it's only a type it doesn't affect anything semantically in the build output.

* Remove non-stable features * docs: add changeset * align changeset with unflagging experimental features docs * Drop SVGComponent type * Add examples for removals to changeset * Report changeset as breaking change rather than unflagged * Apply suggestions from code review Co-authored-by: Sarah Rainsberger <[email protected]> * Apply suggestions from code review Co-authored-by: Sarah Rainsberger <[email protected]> --------- Co-authored-by: Emanuele Stoppa <[email protected]> Co-authored-by: Sarah Rainsberger <[email protected]>

Remove non-stable features

830c515

github-actions bot added pkg: astro Related to the core `astro` package (scope) docs pr labels Mar 22, 2025

docs: add changeset

bdc1dfd

natemoo-re approved these changes Mar 23, 2025

View reviewed changes

ematipico reviewed Mar 24, 2025

View reviewed changes

.changeset/little-steaks-rule.md Show resolved Hide resolved

matthewp approved these changes Mar 24, 2025

View reviewed changes

sarah11918 reviewed Mar 24, 2025

View reviewed changes

.changeset/little-steaks-rule.md Outdated Show resolved Hide resolved

.changeset/little-steaks-rule.md Outdated Show resolved Hide resolved

ematipico mentioned this pull request Mar 25, 2025

Experimental SVG imports can generate invalid SVG code on site build #13496

Closed

1 task

stramel added 3 commits March 31, 2025 16:56

align changeset with unflagging experimental features docs

2d1d26f

Drop SVGComponent type

a513941

Add examples for removals to changeset

32b7582

stramel requested review from ematipico and sarah11918 March 31, 2025 22:07

ematipico requested changes Apr 1, 2025

View reviewed changes

Report changeset as breaking change rather than unflagged

92ef1e6

stramel requested a review from ematipico April 2, 2025 03:00

ematipico approved these changes Apr 2, 2025

View reviewed changes

ematipico added this to the v5.6.0 milestone Apr 2, 2025

Merge remote-tracking branch 'origin/main' into svg-cleanup

000bfbb

sarah11918 reviewed Apr 2, 2025

View reviewed changes

ematipico and others added 2 commits April 2, 2025 12:00

Apply suggestions from code review

9a3be1b

Co-authored-by: Sarah Rainsberger <[email protected]>

Apply suggestions from code review

41f1e0b

Co-authored-by: Sarah Rainsberger <[email protected]>

ematipico merged commit d777420 into withastro:main Apr 2, 2025
6 checks passed

astrobot-houston mentioned this pull request Apr 2, 2025

[ci] release #13540

Merged

sarah11918 mentioned this pull request Apr 2, 2025

Update svg reference for removed features withastro/docs#11348

Merged

stramel deleted the svg-cleanup branch April 2, 2025 20:47

Uh oh!

Remove non-stable SVG features #13488

Remove non-stable SVG features #13488

Uh oh!

Conversation

stramel commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Testing

Docs

Uh oh!

changeset-bot bot commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

codspeed-hq bot commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #13488 will not alter performance

Summary

Uh oh!

natemoo-re left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ematipico left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sarah11918 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ArmandPhilippot commented Mar 24, 2025

Uh oh!

stramel commented Mar 31, 2025

Uh oh!

ematipico left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ematipico Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ematipico left a comment

Choose a reason for hiding this comment

Uh oh!

sarah11918 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ADTC commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ADTC commented Apr 7, 2025

Uh oh!

ArmandPhilippot commented Apr 7, 2025

stramel commented Mar 22, 2025 •

edited

Loading

changeset-bot bot commented Mar 22, 2025 •

edited

Loading

codspeed-hq bot commented Mar 22, 2025 •

edited

Loading

ematipico Apr 2, 2025 •

edited

Loading

sarah11918 left a comment •

edited

Loading

ADTC commented Apr 7, 2025 •

edited

Loading