feat(assets): improve inlining svg

[firefoxic]

Description

All major browser engines do not need to encode > and < characters.

It's also easy to remove all whitespaces between tags before encoding whitespaces.

Both changes allow you to slightly reduce the amount of data in the output.

Additional context

Based on discussion

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the PR Title Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[ArnaudBarre]

@bluwy @sapphi-red What's your opinion on this?
Has pointed out by @sapphi-red maybe this is not safe in CSS?
Given the SVG syntax and that we skip the <text> case, I think the space optimisation between tags is good

[bluwy]

I think it's still not clear to me the cases we need to support in order to decide. For example, among these usages:

1. CSS: background: url(./foo.svg)
2. CSS: background: url('./foo.svg')
3. CSS: background: url("./foo.svg")
4. JS: background: url(${svgImport})
5. JS: background: url('${svgImport}')
6. JS: background: url("${svgImport}")

In practice I think we can only support 1,2,3,6? And if we acknowledge that, which means SVG data URIs will always be double quoted, then I guess we can get away with not encoding things like in this PR.

---

Also, another thing we need to encode specifically is </style>. (Replacing the < only to %3e) Otherwise CSS inlined to HTML files via <style> tags will get cut off.

[sapphi-red]

If the size improvement is big, I think we can say we don't support 4, 5 and skip encoding </>.

---

Also I think that we should skip .replaceAll(/>\s+</g, '><') for SVG's that contains <foreignObject>, because triminmg spaces between tags is not safe in HTML. We probably shouldn't do .replaceAll('"', "'") as well in this case, because something like <p>foo"bar</p> would be replaced.

[ArnaudBarre]

Oh I didn't know about foreignObject. I think it should go in the same case than text and just go with base64 so that we can continue to make optimization for 99% of the cases (i.e. icons).

@firefoxic can you include the foreignObject case in your PR?

[firefoxic]

@firefoxic can you include the foreignObject case in your PR?

I will try to add foreignObject 🙂

But to be honest…

It's not very clear to me why we would need to automatically inline SVG in markup or scripts. The inlining itself is needed to get rid of tens or hundreds of requests to small icon files. Icons are additional decoration to interface text, and it's logical to see them in styles.

But in markup, it would be good to leave SVG only in the form of logos, charts, and other vector illustrations, as these are usually quite large files and there are not many of them.

In scripts, too, it would be better to leave paths to files without encoding them. And if you need to work dynamically from scripts with icons in styles, it would be better to work with custom properties where icons are automatically encoded in styles.

Based on this, I personally would like to be able to disable all SVG inlining except for icons in styles. I used to use postcss-url, and with this built-in feature I wanted to finally switch to LightningCSS.

Maybe I'm old-fashioned in this approach. But if inlining was only in CSS, we wouldn't have to encode whitespace. And I've never encountered foreignObject in icons.

But yeah, this is all just my personal opinion.

[ArnaudBarre]

Being used in CSS doesn't mean it doesn't contain some specific tag. This is not because most people will use it only for icons that everyone will and we need to handle this.
I don't see what is bad to pass an SVG to the src of an image. It serves exactly the same purpose as jpgs, and being aligned with all other assets is more logical.

[firefoxic]

I apologize for the delay.

I see that from the discussion above, ignoring foreignObject is already implemented and removing spaces between > and < is also implemented.

All that remains is to not encode the > and < characters, but only encode < before style (I did that).

What about <![CDATA[...]]>? Wouldn't there be a problem with that?

[ojj1123]

Doesn't have any update?
The inlining svg feature is good one for improving the performance but someone is confused.
#14643 (comment)

In my case, I also have spent a bit time for debugging due to just omitting double quote. below:

// First my try: wrong
<div style={{ mask: `url(${dataUrlForSvg})` }} />

// By chance, I write double quote: correct
<div style={{ mask: `url("${dataUrlForSvg}")` }} />

I didn't even know i should write double quote so I set build.assetInlineLimit: 0 thinking of vite's bug. (actually not bug)
-> https://github.com/softeerbootcamp-3rd/Team1-driving-today/pull/245

---

I think it's still not clear to me the cases we need to support in order to decide. For example, among these usages:

1. CSS: background: url(./foo.svg)
2. CSS: background: url('./foo.svg')
3. CSS: background: url("./foo.svg")
4. JS: background: url(${svgImport})
5. JS: background: url('${svgImport}')
6. JS: background: url("${svgImport}")

In practice I think we can only support 1,2,3,6? And if we acknowledge that, which means SVG data URIs will always be double quoted, then I guess we can get away with not encoding things like in this PR.

I think Vite need to support 1~6. Otherwise, It's a bit confusing in the real use case.