Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SVG components #2006

Open
mfulton26 opened this issue Nov 5, 2023 · 11 comments
Open

SVG components #2006

mfulton26 opened this issue Nov 5, 2023 · 11 comments

Comments

@mfulton26
Copy link

SVGs can be rendered various ways. Each have tradeoffs:

  1. Using <img>. Images load after the HTML which sometimes causing shifting, etc. which isn't desirable.
  2. Using JSX. This requires transforming the raw SVG XML to JSX to use camel casing for attributes and then the image is no longer self contained in its own file; this means the image is no longer rendered as an image in pull requests, file viewers, etc.

Idea: provide a way to store SVGs as plain .svg files and then easily inline them in components

e g. This could be done by supporting importing them as React components like CRA does: https://create-react-app.dev/docs/adding-images-fonts-and-files/#adding-svgs

Alternatively this could be done through some sort of loader which could be provided by Fresh to fetch and inline the SVG inside of an async component function.

@marvinhagemeister
Copy link
Collaborator

Thanks for opening a feature request. It's very unlike that we'd ever support importing .svg files as Preact components, because it's a common performance issue in many applications. See https://twitter.com/_developit/status/1382838799420514317 . The more optimal way is to use an SVG sprite with symbols and reference those with <use>.

@mfulton26
Copy link
Author

Thank you for the insights there. Perhaps then a section in the docs on best practices with examples would be a good addition to Fresh's docs. Folks coming from other frameworks sometimes don't know that all along there's been a better way. Thank you.

@nnmrts
Copy link

nnmrts commented Jan 3, 2024

Another argument for inline SVGs vs img is that when using img tags, SVGs won't load web fonts (or anything remote). If included inline in HTML, they will. But anyway, for cases where you have control over the SVG code, the simple workaround of course, like @mfulton26 mentioned, is to just make a .jsx file with basically the same contents as in the SVG. Still unnecessary code duplication but fixes most problems 99% of the time.

@marvinhagemeister I really don't want to start a big argument over this but™ the tweet you shared and the answers below it gloss over the fact that there are indeed many differences between inline, img, object, background-image and use/symbol SVGs. I tried to find a good, definitive resource for this but couldn't; from experience I know there are a lot of small, not unfixable but annoyingly unexpected issues and differences with stuff like viewBox, the width and height attributes on the svg tag, image, foreignObject, currentColor, (sub-)pixel rendering, web fonts, CSS, etc. There's a lot of gotchas with either way of using SVGs, but (again speaking from experience) inline SVGs has the fewest of them. Here are just some links related to these in case anyone's interested:

Regarding performance, here is a great article/study written by Tyler Sticka, comparing various different techniques (released half a year after the tweet): https://cloudfour.com/thinks/svg-icon-stress-test/

As you can see, the article comes to a different conclusion than the tweet, showing that using use/symbol can even be slower than inline SVGs when the SVGs are optimized. Also, the article and the tweet are mainly concerned with cases where you have dozens or hundreds of icons. A lot of people just want to include two or three SVGs of their website logo and that's it - there, performance really doesn't matter.

All in all, to "use an SVG sprite with symbols and reference those with ." is not definitively the "optimal way", not even definitively in the use-case of embedding a lot of icons.

But I'm not even arguing for inline SVGs being the best thing ever; my main point is: fresh's support of features shouldn't be based on a tweet "just" because the author of the tweet created preact. There are use-cases/reasons for all ways of embedding SVGs (except for iframe maybe :P) and ideally devs should be able to decide for themselves which one they use.

As for my personal suggestion on how to support SVG components:

  • Either go for the way create-react-app is doing it like @mfulton26 mentioned:
    • import { ReactComponent as Logo } from './logo.svg';
    • probably faster and easier to implement
  • Or investigate doing it with import attributes, which seems like a good fit if I understand the proposal correctly
    • import logo from "./logo.svg" with { type: "svg" };
    • probably way harder to implement but is way closer to the ES standard and in a few years might actually be part of it

Until then, I'll be using JSX for my own SVGs and img tags for remote/package SVGs. fresh is flexible and comfortable enough in a lot of other areas anyway so this isn't a huge pain point. But still, I think it would be wise to consider supporting some form of SVG components in the future. They are very common in node.js/React land and people migrating to deno/fresh would appreciate it.

@marvinhagemeister
Copy link
Collaborator

marvinhagemeister commented Jan 3, 2024

@nnmrts Don't get me wrong, I definitely agree that longterm Fresh should support importing SVG files either directly or via an import attribute. Heck, if it were solely to me I would have already added general support for loaders to Fresh and by extension Deno a long time ago. I'm of the opinion that it is wrong for a framework to only allow one way to do things, given that there are often different tradeoffs to certain choices as the article you linked to outlined.

For context: Jason is a good buddy of mine since I also work on Preact and before I joined Deno we both worked together on various performance topics. Performance issues related to SVG icons always popped up in metrics we took and even if the tweet is older, we've seen the SVG symbol approach outperform other solutions again and again. That said they certainly come with a small set of limitations compared to inline SVGs. It's not "just" a tweet and the advice is rooted in real world findings.

But again the framework should enable all kinds of use cases, not just only one opinionated way of doing things because different use cases have different requirements and inherently come with different tradeoffs. I'd love for Fresh to move in a direction where all those scenarios are possible.

The crux of the problem is the same as in #1622 and #2181 in that Fresh doesn't transpile the server code, only the island code that is shipped to the browser. This is something that for sure needs to change to make Fresh a viable option for more users. Once the server code is transpiled, adding support for loading SVG files, CSS modules and all that sort of thing becomes trivial. If it were solely up to me I would have already changed Fresh to support that long ago.

@nnmrts
Copy link

nnmrts commented Jan 3, 2024

@marvinhagemeister Thanks for the quick answer and the insight. I'm sure if right now we would communicate in german I could articulate myself a bit more nuanced, sorry if some parts came across as too harsh...

Also thanks for linking those two issues, I'll definitely watch them in the future. I personally wouldn't like server code being transpiled, but rather the problem being solved on Deno's side or maybe an option to do "server islands", but that's just me.

Below is the comment I wanted to write before you answered, just an interesting example where neither use nor img fixed my problem and I'm still looking for a better solution than just using JSX. It's not fresh specific but I hope you're okay with me sharing it here.


A real world example, because I just wanted to try out using use on the project I'm currently working on and already came across a problem:

This is my svg: https://github.com/denoland/fresh/assets/20396367/dfbf7ccf-81f2-48ac-8f6e-d4d29ea50ffb

It uses gradients and AFAIK the only two ways to actually apply a gradient to an element in SVG is by doing it either in a fill attribute or in a CSS fill property, so:

either

<rect fill="url(#gradient)" />

or

<style>
.rect { fill: url(#gradient); }
</style>
<rect class="rect" />

If you then use such an SVG like so (doesn't matter if this is in HTML, JSX or another SVG):

<use href="logo.svg#main" />

It doesn't render. More specifically, everything with a fill="url(#id)" renders as transparent. There's probably a similar issue with patterns, which are also used in fill attributes.

Read more about this epic chromium bug, which will be 12 years old in 2 days, here: https://bugs.chromium.org/p/chromium/issues/detail?id=109212

A funny quote from there:

How is this still not fixed 10 years later? This bug is now just as old as Voyager 1 leaving the solar system or Lana Del Rey debut album. We went through an entire Spider Man reboot cycle and 3 James Bond movies. A whole generation of consoles came and passed by with PlayStation 4 being both released and superseded by PlayStation 5 while this bug was open. This bug is older than Grand Theft Auto 5. We live in a whole different world now, especially considering the pace web is developing and we STILL cannot store an icon with a gradient in an external SVG sheet.

@marvinhagemeister
Copy link
Collaborator

@nnmrts That's true. The symbol sprite approach assumes that the icons are static and don't need to be changed. It's best used for static icons. More complex use cases are better served with inline SVGs like you outlined.

Also thanks for linking those two issues, I'll definitely watch them in the future. I personally wouldn't like server code being transpiled, but rather the problem being solved on Deno's side (...)

Agree, I've been pushing for support for loaders in Deno for a couple of months now. It looks like we might at least get a small taste in the form of an url, text and byte loader in the near future.

import imageUrl from "./foo/my-image.jpg" with { type: "url" };
import text from "./my-text.txt" with { type: "text" };
import imageData from "./image.jpg" with { type: "bytes" };

This won't help with neither CSS Modules or importing SVG, but it's a step in the right direction. It lays the foundation for different module types internally in Deno. With that in place it's much easier to argue to add more types or open up an API for Deno users to add their own custom ones. There are some details to figure out regarding how code with custom loaders is distributed and security concerns in relation to what happens when a project expects a loader to be present but it isn't. Right now the fallback is to always try to load it as JavaScript which could get hairy.

(...) or maybe an option to do "server islands", but that's just me.

I'm not sure what a server island would look like and what it would do. Can you elaborate on this?

@nnmrts
Copy link

nnmrts commented Jan 3, 2024

@marvinhagemeister

I'm not sure what a server island would look like and what it would do. Can you elaborate on this?

"Server islands" is my idea of a solution that could be implemented in just fresh, without waiting for Deno. Some way of marking a server-rendered file as a file that should be transpiled/transformed/prepared through some pipeline before being served, the pipeline defined in the fresh config maybe. This way, not all files have to be transpiled. Idk, maybe something like:

./routes/dashboard/icons.jsx

const Icons = () => {
	return (
		<div>
			<svg className="h-full" viewBox="0 0 100 100">
				<use href="./logo.svg" />
			</svg>
		</div>
	);
};

./fresh.config.js

import transformUseToInlineSvg from "transform-use-to-inline-svg";
import { defineConfig } from "$fresh/server.ts";

const {
	env
} = Deno;

export default defineConfig({
	port: Number(env.get("PORT")) || 8000,
	build: {
		target: "es2022"
	},
	ssrTransformers: [
		{
			files: ["./routes/dashboard/icons.jsx"],
			transformer: transformUseToInlineSvg
		}
	]
});

./routes/dashboard/icons.jsx is then transformed with transformUseToInlineSvg whenever changed or built, and only after that it is consumed by fresh's usual processes, but still as a SSR component.

But writing this out, I realized this is maybe too niche, too overkill, or something a plugin or a _middleware.js should do, I guess.

@CAYdenberg
Copy link
Contributor

Being able to lint improper camel casing in Preact-SVG components would be a big help IMO. That's bitten me so many times.

@nnmrts
Copy link

nnmrts commented Jan 7, 2024

I've written a fresh plugin to inject SVGs referenced in img tags:

https://github.com/pumpncode/fresh-plugin-svg-inject
https://deno.land/x/fresh_plugin_svg_inject

Maybe someone finds this useful.

@rschristian
Copy link
Contributor

As you can see, the article comes to a different conclusion than the tweet, showing that using use/symbol can even be slower than inline SVGs when the SVGs are optimized.

For what it's worth, I'm not too sure that article is representative of the situation. There's a pretty big difference between injecting inlined SVGs using el.innerHTML as that article/test does and rendering SVGs inlined into JSX. This is primarily where Jason's tweet came from.

Injecting HTML content into a page is indeed fast; rendering SVGs as JSX elements is not going to be nearly as fast as letting the browser use the SVG as an img source or reuse the existing structure via <use>.

Read more about this epic chromium bug, which will be 12 years old in 2 days, here: https://bugs.chromium.org/p/chromium/issues/detail?id=109212

I've been celebrating its birthday for the last 4 years :p. One day we'll have nice things!

@nnmrts
Copy link

nnmrts commented Feb 2, 2024

This issue should probably be moved to a discussion, sorry if I'm using it as that.

Injecting HTML content into a page is indeed fast; rendering SVGs as JSX elements is not going to be nearly as fast as letting the browser use the SVG as an img source or reuse the existing structure via .

You are right that I forgot to consider that difference at all and for client-rendered JSX/React elements it's definitely slower than probably anything else, but for SSR I'm not so sure. I don't know how it exactly works but I'd like to imagine a cached server component with some SVG in it, is basically the same to a browser performance-wise as some HTML with some SVG in it or an img tag. But browsers are weird and I guess especially img sources can get treated and/or cached very differently and more efficiently than one might expect.

The point I was trying to make is that it's a bit more complex, that there are many use-cases and a framework shouldn't "disable" all of them except for one because of this tweet that, yes, as I now understand, applies to a lot of cases and seen as an advice for devs it makes sense, shouldn't be the reason to "turn down" this feature request. That's how it appeared to me in the beginning, but now, a couple of comments later, I understand that there are other, in my opinion bigger reasons, SVG components aren't quite there yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants