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

This will definitely need documentation added to it. Will need some assistance in writing it.

/cc @withastro/maintainers-docs for feedback!

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

🦋 Changeset detected

Latest commit: 5f8997c

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.

natemoo-re · 2024-09-30T12:03:41Z

packages/astro/src/assets/runtime.ts

+		Object.defineProperty(Component, Symbol.for('nodejs.util.inspect.custom'), {
+			value: (_: any, opts: any, inspect: any) => inspect(meta, opts),
+		});


Not sure how necessary this is, but maintaining the current console.log behavior seems like a good idea.

A comment here explaining why we're doing this would be helpful 👍

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

natemoo-re · 2024-09-30T12:07:15Z

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

+	if (size) {
+		props.height = size;
+		props.width = size;
+	}


Looks like size will override width and height attributes if present. I think size should only apply when they are undefined.

natemoo-re · 2024-09-30T12:09:49Z

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

+				it('has height and width overridden - size set', () => {
+					let $svg = $('#override-attrs svg');
+					assert.equal($svg.length, 1);
+					assert.equal($svg.attr('height'), '16');
+					assert.equal($svg.attr('width'), '16');
+					assert.equal(!!$svg.attr('size'), false);
+				});


This test will need updating if the size behavior switches

natemoo-re · 2024-09-30T12:12:26Z

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

+					assert.equal(!!$svg.attr('version'), false);
+				});
+			});
+			describe('additional props', () => {


Would be nice to assert that the default role="img" is overwritable. Now that I think of it, we should also double check that role="img" is the best default since I know aria-hidden and role="presentation" are also used sometimes.

natemoo-re · 2024-09-30T12:12:57Z

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

😩 this is the old logo

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

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

@@ -0,0 +1,54 @@
+import { parse, renderSync } from 'ultrahtml';


Just a callout that I maintain ultrahtml—this could easily be swapped for another XML parser if preferred, but keep in mind that it will be bundled into the server runtime so we should try to keep a light footprint.

Princesseuh · 2024-09-25T07:27:59Z

packages/astro/client.d.ts

@@ -1,6 +1,7 @@
 /// <reference types="vite/types/import-meta.d.ts" />
 /// <reference path="./types/content.d.ts" />
 /// <reference path="./types/actions.d.ts" />
+/// <reference path="./jsx-runtime.d.ts" />


This shouldn't be needed, was there something you were encountering?

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

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

Open

stramel force-pushed the add-svg-support branch 4 times, most recently from 934d200 to 5a7b888 Compare September 25, 2024 15:14

stramel added 5 commits September 25, 2024 15:41

feat(astro): add SVG Component support

8a8f52a

Utilize SVG Components in blog example

279eaf5

Allow inline icons

a6ecdfa

add changeset

5290098

fix: enable backwards-compat of svg imports

20db55b

stramel force-pushed the add-svg-support branch from 12cd643 to 2ff0715 Compare September 25, 2024 20:42

test: add supporting tests

5f8997c

stramel force-pushed the add-svg-support branch 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

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

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

natemoo-re Sep 30, 2024

natemoo-re Sep 30, 2024

Princesseuh Sep 30, 2024

natemoo-re Sep 30, 2024

natemoo-re Sep 30, 2024

natemoo-re Sep 30, 2024

natemoo-re Sep 30, 2024

natemoo-re Sep 30, 2024

Princesseuh Sep 25, 2024

Princesseuh Sep 30, 2024

		@@ -0,0 +1,54 @@
		import { parse, renderSync } from 'ultrahtml';

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 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

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

Loading

changeset-bot bot commented Sep 25, 2024 •

edited

Loading