feat(astro): add Built-in SVG component support #12067

stramel · 2024-09-25T04:40:21Z

Changes

This PR implements Built-in SVG Components following up on the discussions on withastro/roadmap#699

By default, SVG files will be components and can be imported by referencing their file path and then treating them like a component. Take this for example:

previously:

---
import Image from 'astro:assets'
import logo from './path/to/svg/file.svg'
---

<div set:html={logo}  style={{height: '24px', width: '24px' }} />
<!-- or -->
<Image src={logo} width={24} height={24} />

now:

---
import Logo from './path/to/svg/file.svg';
---

<Logo size={24} />

GOALS:

Allow users to import and render local .svg files as if they were an Astro component
Allow passing props onto the root element while overriding the existing props
Automate best practices for to minimize performance footguns
- Including many inlined on a page can hurt browser parsing performance. We can automatically reduce the number of nodes by using and .
- Inline often have xmlns and xmlns:xlink and version attributes. These are not needed in HTML5 and we can automatically drop them to save a few bytes.
Support this in a backwards-compatible way, meaning the current public interface of an .svg import remains unchanged
Stretch: support rendering .svg components in frameworks without inlining them into the client side JavaScript
- Creating framework components for .svg icons is an anti-pattern. It’s a terrible idea that bloats your JavaScript and creates all sorts of headaches for HMR and clientside performance in production.
- If we provide a great component for each supported framework, we’ll make it easy for users to avoid this anti-pattern. It’s so common now because there aren’t many other good solutions.

This builds a foundation for us which can be extended in the future within the core astro and even by third-parties. For instance using astro-icon, which has been the critical plugin when it comes to icons, as a playground, I was able to extend the foundation built here to add dynamic icon import for Iconify.

FUTURE:

Optimization: Allow hooking in SVGO or other SVG Optimizers

/cc @natemoo-re

Testing

I have ensured that existing tests are functioning properly and added additional suite for SVG Components.

Manually, I have updated the blog example to include using the SVG components. I have also tested locally against astro-icon and its demo application. Will work on some testing for this change.

Docs

Docs added in withastro/docs#9911

changeset-bot · 2024-09-25T04:40:27Z

🦋 Changeset detected

Latest commit: 9344a25

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

github-actions

This PR is blocked because it contains a major changeset. A reviewer will merge this at the next release if approved.

natemoo-re

Left a few suggestions, but this looks really good!

natemoo-re · 2024-09-30T12:01:53Z

packages/astro/client.d.ts

+		/**
+		 * Bypasses automatic sprite optimization by directly inlinining the SVG
+		 */
+		inline?: boolean


After sitting with this for a while, inline behavior should be the default. There are a few edge-cases that have come up with dynamic client-side behavior in astro-icon, so while I still think the optimization is very useful, we should prioritize flexibility by keeping the sprite behavior opt-in.

You're right. I think especially with the SVG imports that developers wouldn't necessarily assume the optimization by default.

My first thought is to offer a "mode" that allows for "sprite" or "inline" ("inline" being the default) that is configurable in the Astro Config. I assume we'll later have some other properties under and "svg" scope such as an "image service" (like svgo).

I have also seen an interesting use-case (natemoo-re/astro-icon#238) that we might be able to support later for exporting symbols instead. This would be fairly trivial with a "mode" and might enable some better DX around framework usages.

I would like to discuss and perhaps iterate a bit on the way we allow developers to opt-in/out of inline/sprite usages. Will bring this up in the Stage 3 RFC.

packages/astro/src/assets/runtime.ts

natemoo-re · 2024-09-30T12:05:31Z

packages/astro/src/assets/utils/node/emitAsset.ts

+	// Attach file data for SVGs
+	if (fileMetadata.format === 'svg') {
+		emittedImage.contents = fileData;
+	}


Would love @Princesseuh's take on this. Any expected footguns with including the file content for SVGs in the image metadata?

If you have a lot of SVGs, this can be problematic for SSR since it'll get in your bundle, that's my only concerns

Right. Unfortunately that will be unavoidable for this feature.

I think the DX improvement is worth the tradeoff, though!

This is definitely outside my realm of expertise. I'll defer to whatever the consensus is here :)

Something we could do one day (definitely outside of this PR), though it'd make SVG icons somewhat slower in SSR is add a lazy data property or something, that's essentially async () => await fetch(this.src) in SSR and something else in SSG so that you can load the data without it being added to the bundle.

packages/astro/src/assets/utils/svg.ts

packages/astro/test/core-image-svg.test.js

packages/astro/test/fixtures/core-image-svg/src/assets/astro.svg

packages/astro/src/assets/utils/svg.ts

packages/astro/client.d.ts

Princesseuh · 2024-09-30T13:33:55Z

packages/astro/src/assets/utils/node/emitAsset.ts

+	// Attach file data for SVGs
+	if (fileMetadata.format === 'svg') {
+		emittedImage.contents = fileData;
+	}


If you have a lot of SVGs, this can be problematic for SSR since it'll get in your bundle, that's my only concerns

examples/blog/src/components/Footer.astro

packages/astro/src/assets/utils/svg.ts

packages/astro/src/assets/vite-plugin-assets.ts

stramel · 2024-10-05T04:31:54Z

I have added it behind an experiment flag called svg. With this change, I swapped the default implementation to be a more straightforward approach of inlining the SVG. In order to control this a bit better, I have a mode setting in the svg config to apply a default, which is defaulted to inline. I also changed the inline prop to a mode prop that will allow configuring between inline and sprite on specific SVGs.

@natemoo-re would love your opinion on this approach for configuring the sprite vs inline method

stramel · 2024-10-22T06:15:07Z

Going to double-check everything tomorrow! Hopefully we're good to merge after that

stramel · 2024-10-22T23:02:47Z

Should be good to go!

sarah11918 · 2024-10-28T15:50:45Z

Just pointing out I haven't seen any docs for this yet, so please let me know if those are coming or if help is needed!

stramel · 2024-10-28T18:46:59Z

Just pointing out I haven't seen any docs for this yet, so please let me know if those are coming or if help is needed!

I'm not familiar with the docs process. I would be happy to write some if you could point me to a previous PR or something as a guide. I also would welcome some help as writing isn't my forte 🙂

ematipico · 2024-10-30T11:51:11Z

@stramel

Since we are about to send this PR to next, you'll have to send the docs PR to their respective branch, which is 5.0.0-beta.

As for where, I would start with the Images guide: https://docs.astro.build/en/guides/images/

https://github.com/withastro/docs/blob/5.0.0-beta/src/content/docs/en/guides/images.mdx

As for where in the page, I don't exactly know, but I would start with a new paragraph (maybe an h2 heading) and send the PR, then Sarah and team will take it from there, and will help you to craft the content.

packages/astro/src/assets/runtime.ts

bluwy · 2024-10-31T15:24:51Z

packages/astro/src/assets/runtime.ts

+/**
+ * Make sure these IDs are kept on the module-level so they're incremented on a per-page basis
+ */
+let ids = 0;


IIUC, if this is on the module-level (and also globally as a singleton), wouldn't this value not be incremented on a per-page basis? If per page, I think it should be bound to the result from createComponent as a single result is created when rendering per page. So maybe we can use a WeakMap here to track the ids per result, which we can also use it to track the first render instead of using WeakSet<Response>?

I agree that WeakMap seems like a good idea here!

I'm not quite following how to use the WeakMap here.

The suggestion here is to use const ids = new WeakMap<SSRResult, number> instead to track the id per result (from createComponent)

I'm having trouble getting this to work. I'll give it a shot again tomorrow. Maybe I'm just tired 😅

I'll help you out :)

Here 6ab0f1a (#12067)

Thanks! I missed that we would still need to keep the counter/ids. I'm curious what the WeakMap adds in this case since we're still generating the ids by the counter.

packages/astro/src/assets/vite-plugin-assets.ts

natemoo-re

Looks really good!

packages/astro/src/assets/runtime.ts

natemoo-re · 2024-11-05T03:45:51Z

packages/astro/src/assets/runtime.ts

+/**
+ * Make sure these IDs are kept on the module-level so they're incremented on a per-page basis
+ */
+let ids = 0;


I agree that WeakMap seems like a good idea here!

.changeset/blue-socks-doubt.md

ematipico · 2024-11-06T10:41:33Z

@stramel While I was working on the PR, I noticed that you added a new configuration in Astro, which is fine, however the RFC doesn't mention anything about it. We should update the RFC and explain the options.

Also, here's some feedback regarding experimental options. There aren't written rules, but in the past I was in your same situation and I went for this route:

export default defineConfig({
	svg: {
		mode: "inline"
	},
	experimental: {
		svg: true
	}
})

We still need to have the experimental flag, which stays a boolean every time. Then, you create a new section in the Astro configuration. You're still free to change it as you see fit, because it's part of the experimental feature, however I found that the maintenance of these two separate sections is easier for us maintainers.

sarah11918

Thank you so much for this amazing feature @stramel ! I think people are going to really enjoy this! 🙌

I've done a quick pass on the documentation (changeset and flag docs) and you'll find some suggestions below for your consideration!

I know Nate has a docs PR going, and it seems like the mode property is newer than that, so we'll just make sure that we've got everything we need in the docs PR, too, before docs totally signs off on this! 🚀

.changeset/blue-socks-doubt.md

packages/astro/src/types/public/config.ts

sarah11918 · 2024-11-06T16:36:36Z

packages/astro/src/types/public/config.ts

+			 * - `inline`: Astro will inline the SVG content into your HTML output.
+			 * - `sprite`: Astro will generate a sprite sheet with all imported SVG files.
+			 * 
+			 * When using 'sprite' mode, it is important to note that the symbol definition will be included with the first instance of the SVG that is rendered. This means that if the first instance is hidden or removed from the DOM, all other references will break.


Suggested change

* When using 'sprite' mode, it is important to note that the symbol definition will be included with the first instance of the SVG that is rendered. This means that if the first instance is hidden or removed from the DOM, all other references will break.

* When using 'sprite' mode, it is important to note that the symbol definition will be included with the first instance of the SVG that is rendered. This means that if the first instance is hidden or removed from the DOM, all other references will break.

Question: is this unique to Astro's implementation, or would this be something common to using a sprite sheet in general? I get that it's a caveat, but if it's not an Astro-specific caveat then it's a little bit more for the reader to have to process before they even see how to use this feature.

I might suggest we either remove this paragraph entirely (we do say full details in the RFC!), or place it after the code example if this is a super key thing and people will have a lot of trouble using the feature if they don't know this immediately up front.

This is unique to the implementation that we chose to go with. Though it would probably apply to the spritesheet in general but is significantly less common if you just have a single spritesheet.

This is something I was debating about adding as an enhancement later or opt-in. Would need some help from the core team on it though.

I'll leave it up to you if we keep this caveat or remove it.

packages/astro/src/types/public/config.ts

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

github-actions bot added pkg: example Related to an example package (scope) pkg: astro Related to the core `astro` package (scope) semver: major Change triggers a `major` release labels Sep 25, 2024

github-actions bot requested changes Sep 25, 2024

View reviewed changes

This was referenced Sep 25, 2024

feat(core): rewrite to use built-in svg components natemoo-re/astro-icon#239

Open

Built-in SVG components withastro/roadmap#699

Closed

stramel force-pushed the add-svg-support branch 6 times, most recently from 2ff0715 to 5f8997c Compare September 25, 2024 20:43

stramel marked this pull request as ready for review September 25, 2024 21:15

natemoo-re reviewed Sep 30, 2024

View reviewed changes

Princesseuh reviewed Sep 30, 2024

View reviewed changes

Princesseuh reviewed Oct 1, 2024

View reviewed changes

examples/blog/src/components/Footer.astro Outdated Show resolved Hide resolved

stramel force-pushed the add-svg-support branch from 5f8997c to 313caaa Compare October 2, 2024 04:20

github-actions bot removed the pkg: example Related to an example package (scope) label Oct 2, 2024

stramel requested review from Princesseuh and natemoo-re October 2, 2024 04:45

Princesseuh added semver: minor Change triggers a `minor` release and removed semver: major Change triggers a `major` release labels Oct 4, 2024

Princesseuh reviewed Oct 4, 2024

View reviewed changes

packages/astro/src/assets/utils/svg.ts Outdated Show resolved Hide resolved

Princesseuh reviewed Oct 4, 2024

View reviewed changes

packages/astro/src/assets/vite-plugin-assets.ts Outdated Show resolved Hide resolved

github-actions bot added the docs pr label Oct 5, 2024

stramel requested a review from Princesseuh October 5, 2024 04:52

stramel force-pushed the add-svg-support branch 2 times, most recently from ad55d4e to bf5e98b Compare October 5, 2024 05:00

stramel added 6 commits October 22, 2024 00:23

do not override width and height with size

2b6744b

add test to assert role is overridable

a751408

update to correct astro logomark

11e2eba

add comment explaining the code for backwards compat

3c576fd

test markdown works as well

3f274c4

add behind an experiment flag

a92cb57

stramel force-pushed the add-svg-support branch from bf5e98b to a92cb57 Compare October 22, 2024 05:31

stramel changed the base branch from main to next October 22, 2024 05:31

github-actions bot removed the docs pr label Oct 22, 2024

Merge branch 'next' into add-svg-support

66eec7a

github-actions bot added the docs pr label Oct 22, 2024

update tests

fcc6eca

bluwy reviewed Oct 31, 2024

View reviewed changes

natemoo-re reviewed Nov 5, 2024

View reviewed changes

natemoo-re mentioned this pull request Nov 5, 2024

Add svg component reference withastro/docs#9911

Open

stramel and others added 6 commits November 6, 2024 01:05

docs: update changeset

78490fb

refactor: inline functions

ddf5604

fix: make non enumerable dev only

767559c

refactor: allow boolean flag

31b08e7

docs: update documentation around experimental.svg flag

c03a467

track IDs using a WeakMap, and fix configuration types

6ab0f1a

sarah11918 reviewed Nov 6, 2024

View reviewed changes

stramel and others added 2 commits November 6, 2024 22:24

Apply suggestions from code review

d40d45d

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

refactor: swap experimental.svg schema implementation

9344a25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(astro): add Built-in SVG component support #12067

feat(astro): add Built-in SVG component support #12067

stramel commented Sep 25, 2024 •

edited by natemoo-re

Loading

changeset-bot bot commented Sep 25, 2024 •

edited

Loading

github-actions bot left a comment

natemoo-re left a comment

natemoo-re Sep 30, 2024

stramel Oct 2, 2024

natemoo-re Sep 30, 2024

Princesseuh Sep 30, 2024

natemoo-re Oct 1, 2024

stramel Oct 2, 2024

Princesseuh Oct 4, 2024

Princesseuh Sep 30, 2024

stramel commented Oct 5, 2024 •

edited

Loading

stramel commented Oct 22, 2024

stramel commented Oct 22, 2024

sarah11918 commented Oct 28, 2024

stramel commented Oct 28, 2024

ematipico commented Oct 30, 2024 •

edited

Loading

bluwy Oct 31, 2024

natemoo-re Nov 5, 2024

stramel Nov 6, 2024

bluwy Nov 6, 2024

stramel Nov 6, 2024

ematipico Nov 6, 2024

ematipico Nov 6, 2024

stramel Nov 7, 2024

natemoo-re left a comment

natemoo-re Nov 5, 2024

ematipico commented Nov 6, 2024 •

edited

Loading

sarah11918 left a comment

sarah11918 Nov 6, 2024

stramel Nov 7, 2024

	* When using 'sprite' mode, it is important to note that the symbol definition will be included with the first instance of the SVG that is rendered. This means that if the first instance is hidden or removed from the DOM, all other references will break.
	* When using 'sprite' mode, it is important to note that the symbol definition will be included with the first instance of the SVG that is rendered. This means that if the first instance is hidden or removed from the DOM, all other references will break.

feat(astro): add Built-in SVG component support #12067

Are you sure you want to change the base?

feat(astro): add Built-in SVG component support #12067

Conversation

stramel commented Sep 25, 2024 • edited by natemoo-re Loading

Changes

Testing

Docs

changeset-bot bot commented Sep 25, 2024 • edited Loading

🦋 Changeset detected

github-actions bot left a comment

Choose a reason for hiding this comment

natemoo-re left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stramel commented Oct 5, 2024 • edited Loading

stramel commented Oct 22, 2024

stramel commented Oct 22, 2024

sarah11918 commented Oct 28, 2024

stramel commented Oct 28, 2024

ematipico commented Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemoo-re left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico commented Nov 6, 2024 • edited Loading

sarah11918 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stramel commented Sep 25, 2024 •

edited by natemoo-re

Loading

changeset-bot bot commented Sep 25, 2024 •

edited

Loading

stramel commented Oct 5, 2024 •

edited

Loading

ematipico commented Oct 30, 2024 •

edited

Loading

ematipico commented Nov 6, 2024 •

edited

Loading