-
Notifications
You must be signed in to change notification settings - Fork 648
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
Comments
Thanks for opening a feature request. It's very unlike that we'd ever support importing |
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. |
Another argument for inline SVGs vs @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,
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 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 As for my personal suggestion on how to support SVG components:
Until then, I'll be using JSX for my own SVGs and |
@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. |
@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 A real world example, because I just wanted to try out using 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 either <rect fill="url(#gradient)" /> or <style>
.rect { fill: url(#gradient); }
</style>
<rect class="rect" /> If you then <use href="logo.svg#main" /> It doesn't render. More specifically, everything with a 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:
|
@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.
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 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.
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:
const Icons = () => {
return (
<div>
<svg className="h-full" viewBox="0 0 100 100">
<use href="./logo.svg" />
</svg>
</div>
);
};
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
}
]
});
But writing this out, I realized this is maybe too niche, too overkill, or something a plugin or a |
Being able to lint improper camel casing in Preact-SVG components would be a big help IMO. That's bitten me so many times. |
I've written a fresh plugin to inject SVGs referenced in https://github.com/pumpncode/fresh-plugin-svg-inject Maybe someone finds this useful. |
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 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
I've been celebrating its birthday for the last 4 years :p. One day we'll have nice things! |
This issue should probably be moved to a discussion, sorry if I'm using it as that.
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 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. |
SVGs can be rendered various ways. Each have tradeoffs:
<img>
. Images load after the HTML which sometimes causing shifting, etc. which isn't desirable.Idea: provide a way to store SVGs as plain
.svg
files and then easily inline them in componentse 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.
The text was updated successfully, but these errors were encountered: