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

feat(build)!: inline SVGs #14643

Merged
merged 2 commits into from
Oct 19, 2023
Merged

feat(build)!: inline SVGs #14643

merged 2 commits into from
Oct 19, 2023

Conversation

ArnaudBarre
Copy link
Member

@ArnaudBarre ArnaudBarre commented Oct 16, 2023

Closes #2909

I think we should ship this as part of Vite 5, this SVG exception is really bad when having a lot of small SVG icons used as images.

Not using https://github.com/tigt/mini-svg-data-uri for multiple reason

  • colors minification should be done ahead of time or with a plugin with other optimizations (using svgo for example)
  • using multiple replaceAll (node 15) which is faster than encodeURI or regexes or one regex with match function

@ArnaudBarre ArnaudBarre self-assigned this Oct 16, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for handling this. I also looked through the history recently and stumped on what to do:

  1. If we can't replace away %20, there's a good chance base64 might be better?
  2. Should we swap between encoded-svg or base64 if one is shorter?
  3. What happens if the final encoded string is longer than original?

I don't quite have an answer to those, but if it means reducing the extra network request I guess we can stick with one strategy and accept the cost. Another idea is to support .svg?src (keeps instead of %20), but feels too complicated at that point.

packages/vite/src/node/plugins/asset.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/asset.ts Outdated Show resolved Hide resolved
@ArnaudBarre
Copy link
Member Author

ArnaudBarre commented Oct 16, 2023

Yeah you're write I will do a check to be use the base64 if shorter so we don't end up with something more than 33% of "inlineLimit" (which is what already happen for images).

I think this ok to provide a safe and simple inlining by default, and if people want something more aggressive because they know they don't use srcset they can provide their own plugin?

@bluwy
Copy link
Member

bluwy commented Oct 16, 2023

Thanks, yeah if one of the compression technique tend to be smaller, maybe we can choose that and stick to it. If we were doing both in runtime, it could be a little slower. 🤔

if people want something more aggressive because they know they don't use srcset they can provide their own plugin?

Yeah I think that's doable too. We can later check if this is something people use often and revisit again.

@ArnaudBarre
Copy link
Member Author

Ok so actually I was mistaken by the initial use of srcset. This HTML property is used when using multiple entries for responsive image, which would be stupid to use with SVG 😅

So I think this is ok that people using it in srcset get a broking source. In that case, any meaningful non text SVG will always be shorter with an URI, so no need for a check.

Require #14657 to fix tests

@sapphi-red sapphi-red linked an issue Oct 17, 2023 that may be closed by this pull request
@bluwy
Copy link
Member

bluwy commented Oct 17, 2023

Ok so actually I was mistaken by the initial use of srcset. This HTML property is used when using multiple entries for responsive image, which would be stupid to use with SVG 😅

I guess that's somewhat true. The original request was from #2909 (review) which also mention for image-set(). If this kind of usage is rare, maybe it's fine to not do it by default and support some query to force base64 in the future?

If we're really nitpicking it, the replacement of " to ' would also fail if the attribute is quoted with single quotes for some reason too. So there's really no easy middleground here.

cc @polarathene maybe you can share how SVGs would be frequently used in srcsets?

@sapphi-red
Copy link
Member

We can replace spaces with %20 for srcset here if we need to.

urlToBuiltUrl(url, id, config, this),

It seems spaces in image-set is already handled here.
if (wrap === '' && newUrl !== encodeURI(newUrl)) {
// The new url might need wrapping even if the original did not have it, e.g. if a space was added during replacement
wrap = "'"
}

@polarathene
Copy link

  1. If we can't replace away %20, there's a good chance base64 might be better?

Compression is generally a given when size matters. The original PR has this comment of mine:

For anyone that outputs static HTML/CSS, they can probably post-process if they want to convert %20 to (space) when valid.
It can make for decent savings on larger SVG uncompressed (it's often larger than base64 with encoded spaces), but most of the time compression will be in use anyway which closes the gap notably.

For example, an SVG of 1,164 bytes url-encoded (but spaces retained) is 590 bytes gzip level 6, and the space encoded version is 1,494 bytes but only 623 bytes via gzip 6. Base64 equivalent 1,474 bytes uncompressed and 863 bytes gzip 6.
Original SVGO optimized SVG is 1,084 bytes as a file (561 bytes gzip 6).

Worth keeping that in mind, rather than only considering the uncompressed size.


reducing the extra network request

Depending on context, if an SVG is re-used it can be more efficient to have that network request (IIRC browsers coalesce the same asset into one request + can leverage cache), or inline CSS style that defines the graphic once and reuses a class to apply it multiple times (there's also SVG fragments as an alternative).

A similar approach is used with CSS for bundling many small images into a larger sprite sheet.


@polarathene maybe you can share how SVGs would be frequently used in srcsets?

I don't write much frontend web these days, and I'm having trouble recalling specific projects that I used srcset with SVG.

  • I was previously a frequent contributor to Gatsby image package before it got rewritten, that did focus quite a bit on responsive image support and SVG was part of that I think, along with base64 placeholder graphic while the actual image request loads (demo of mine without SVG involved).
  • gatsby-image being a React component used during a build, it didn't have the context concern brought up here. Perhaps you could support a way to hint which encoding type to use?

One of the use-cases there was art direction. You don't always want the same SVG graphic depending on context.

  • Viewport orientation / dimensions being landscape vs portrait (eg: mobile vs desktop), I've seen marketing pages adjust the content to accommodate.
  • Likewise while SVG generally scales well, the detail may not suit smaller sizes (like icons) or larger sizes look better with some adjustments (scaling of size of curved corners vs keeping them consistent for example).
  • Mozilla art direction summary section under "Resolution Switching" also seems to suggest SVG with srcset + sizes as useful.

Perhaps it's not that frequent to encounter, but it has its uses 🤷‍♂️


If it's helpful, I did find this example I put together a few years ago (not using srcset). It inlines a blank SVG into the HTML, and also for a CSS property; both without encoding spaces.

At the bottom of this 404 page is an img tag with a blank SVG data URI encoded.

  • It prevents rendering the broken image graphic for an invalid src, while defining the width + height. The viewBox dimensions served a purpose too, I just don't recall the specifics 😅
  • CSS is used with mask-image and background-color: currentColor to enable mouse hover of the link to adjust the SVG colour.
  • The webkit prefixed compatibility fallback generated at build was a non-issue as compression minimized that quite well IIRC.
Relevant snippets from that webpage for reference
<a class="netlify-link" href="https://www.netlify.com/" target="_blank" rel="noopener noreferrer">
  <span>Powered by</span> 
  <img alt="Netlify" class="logo-netlify" src="data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 147 40' /%3e" width="147" height="40">
</a>
.logo-netlify {
    background-color: currentColor;

    -webkit-mask-image: url(data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 147 40'%3e%3cg fill='white' fill-rule='evenodd'%3e%3cpath d='m53.37 12.98.12 2.2a6.82 6.82 0 0 1 5.53-2.55c3.95 0 5.96 2.26 6.03 6.8V32h-4.26V19.68c0-1.21-.26-2.1-.78-2.69-.52-.58-1.37-.87-2.55-.87-1.72 0-3 .78-3.84 2.34V32h-4.26V12.98h4.01zm24.38 19.37a8.8 8.8 0 0 1-6.57-2.56c-1.68-1.7-2.52-3.97-2.52-6.8v-.54c0-1.9.37-3.6 1.1-5.08a8.13 8.13 0 0 1 7.5-4.74c2.58 0 4.58.82 5.99 2.48 1.4 1.65 2.11 3.99 2.11 7.01v1.72h-12.4c.13 1.57.65 2.81 1.57 3.73s2.07 1.37 3.46 1.37c1.96 0 3.55-.79 4.77-2.37l2.3 2.2a7.68 7.68 0 0 1-3.04 2.64 9.5 9.5 0 0 1-4.27.94zm-.51-16.3c-1.17 0-2.11.42-2.83 1.24a6.38 6.38 0 0 0-1.38 3.42h8.12v-.31a5.2 5.2 0 0 0-1.14-3.25 3.56 3.56 0 0 0-2.77-1.1zm16.77-7.7v4.63h3.34v3.16H94v10.62c0 .73.15 1.25.43 1.57.3.33.8.49 1.54.49a6.55 6.55 0 0 0 1.49-.18v3.3c-.97.28-1.9.4-2.8.4-3.28 0-4.92-1.8-4.92-5.42V16.14h-3.12v-3.16h3.12V8.36H94zM105.13 32h-4.26V5h4.26zm9.17 0h-4.26V12.98h4.26zM109.8 8.04c0-.66.2-1.2.62-1.63.42-.44 1.01-.65 1.78-.65s1.37.21 1.8.65c.41.43.62.97.62 1.63a2.2 2.2 0 0 1-.63 1.6 2.4 2.4 0 0 1-1.79.65c-.77 0-1.36-.21-1.78-.64a2.22 2.22 0 0 1-.62-1.61zM120.45 32V16.14h-2.9v-3.16h2.9v-1.74c0-2.11.59-3.74 1.75-4.89 1.17-1.15 2.81-1.72 4.91-1.72.75 0 1.55.1 2.4.32l-.11 3.34a8.38 8.38 0 0 0-1.64-.14c-2.03 0-3.05 1.04-3.05 3.14v1.69h3.86v3.16h-3.86V32h-4.26zm17.87-6.12 3.86-12.9h4.54l-7.54 21.9c-1.16 3.2-3.12 4.8-5.9 4.8-.61 0-1.3-.1-2.04-.32v-3.3l.8.05c1.08 0 1.89-.2 2.43-.59a3.9 3.9 0 0 0 1.3-1.97l.6-1.64-6.66-18.93h4.6z'/%3e%3cpath fill-rule='nonzero' d='M27.89 14.13h-.02l-.02-.01a.11.11 0 0 1-.03-.1l.78-4.72 3.62 3.62-3.77 1.6a.08.08 0 0 1-.03.01h-.02l-.02-.01a1.72 1.72 0 0 0-.5-.38zm5.25-.28 3.88 3.87c.8.8 1.2 1.21 1.36 1.68l.05.2-9.26-3.92a.73.73 0 0 0-.02 0c-.03-.02-.08-.04-.08-.07s.05-.06.08-.07h.02zm5.13 7c-.2.38-.59.77-1.25 1.43l-4.37 4.37L27 25.47h-.03c-.05-.01-.1-.02-.1-.07a1.7 1.7 0 0 0-.66-1.2c-.02-.02-.01-.05 0-.08v-.02l1.06-6.52v-.03c0-.05.02-.1.06-.1a1.73 1.73 0 0 0 1.16-.67l.03-.03c.03-.01.07 0 .1.02l9.65 4.08zm-6.62 6.8-7.19 7.19 1.23-7.56v-.04c.02-.03.05-.04.07-.05h.01a1.85 1.85 0 0 0 .7-.52c.02-.03.05-.05.09-.06a.09.09 0 0 1 .03 0l5.06 1.04zm-8.71 8.7-.81.82-8.95-12.94a.42.42 0 0 0-.01-.02l-.03-.06.02-.04.01-.01.08-.13.02-.03c.01-.03.03-.05.05-.06.02-.01.05-.01.07 0l9.92 2.04a.16.16 0 0 1 .08.03l.02.05a1.76 1.76 0 0 0 1.03 1.17c.03.02.01.05 0 .08a.24.24 0 0 0-.01.05l-1.49 9.06zm-1.7 1.7c-.59.59-.94.9-1.34 1.03a2 2 0 0 1-1.2 0c-.47-.15-.87-.55-1.68-1.36l-9-8.99 2.36-3.64a.15.15 0 0 1 .04-.05c.02-.02.06-.01.09 0a2.43 2.43 0 0 0 1.64-.08c.02-.01.05-.02.07 0a.19.19 0 0 1 .03.03zM7.17 27.86 5.1 25.8l4.07-1.74a.08.08 0 0 1 .04 0c.03 0 .05.03.07.06a2.91 2.91 0 0 0 .13.18l.01.02v.05l-2.26 3.5zM4.18 24.9l-2.6-2.61c-.45-.45-.77-.77-1-1.05l7.94 1.65a.84.84 0 0 0 .03 0c.05.01.1.02.1.07s-.06.07-.1.09l-.03.01zm-4.05-5a2 2 0 0 1 .09-.5c.15-.46.55-.86 1.36-1.67l3.34-3.34a2175.53 2175.53 0 0 0 4.62 6.69c.03.04.06.08.03.1-.15.17-.3.34-.4.53a.16.16 0 0 1-.05.07h-.04L.13 19.89zm5.68-6.4 4.49-4.5c.42.2 1.96.84 3.33 1.42l2.29.97c.03.01.06.03.07.06v.06a2 2 0 0 0 .52 1.82c.03.03 0 .08-.02.11l-.02.03-4.56 7.06a.14.14 0 0 1-.04.05h-.09a2.27 2.27 0 0 0-.54-.07 3 3 0 0 0-.52.06h-.06a.21.21 0 0 1-.04-.06L5.8 13.5zm5.4-5.4 5.81-5.81c.8-.8 1.21-1.21 1.68-1.36a2 2 0 0 1 1.2 0c.47.15.87.55 1.68 1.36l1.26 1.26-4.14 6.4a.15.15 0 0 1-.04.05c-.03.02-.06 0-.1 0a2.1 2.1 0 0 0-1.91.37c-.03.03-.07.01-.1 0L11.2 8.09zm12.5-3.67 3.82 3.81-.92 5.7v.02a.14.14 0 0 1 0 .03l-.06.03a1.83 1.83 0 0 0-.54.28.15.15 0 0 0-.02.01l-.04.03a.11.11 0 0 1-.05 0l-5.81-2.48h-.02c-.03-.02-.08-.04-.08-.08a2.2 2.2 0 0 0-.3-.91c-.03-.05-.07-.1-.04-.14zm-3.93 8.6 5.45 2.31c.03.01.07.03.08.06a.1.1 0 0 1 0 .06c-.02.08-.03.17-.03.26v.15c0 .04-.04.06-.07.07h-.02l-12.14 5.18c-.01 0-.04 0-.05-.02-.03-.03 0-.07.02-.11a.76.76 0 0 0 .02-.02l4.48-6.94v-.01c.03-.04.06-.1.11-.1l.05.02.28.02c.68 0 1.3-.33 1.69-.9a.16.16 0 0 1 .03-.03c.03-.02.07-.01.1 0zm-6.25 9.19 12.28-5.24.04.02.18.15.03.02c.02.01.05.03.05.05v.03l-1.06 6.46v.03c0 .05-.01.1-.06.1a1.73 1.73 0 0 0-1.37.85l-.06.07h-.07l-9.8-2.02-.15-.52z'/%3e%3c/g%3e%3c/svg%3e);

    mask-image: url(data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 147 40'%3e%3cg fill='white' fill-rule='evenodd'%3e%3cpath d='m53.37 12.98.12 2.2a6.82 6.82 0 0 1 5.53-2.55c3.95 0 5.96 2.26 6.03 6.8V32h-4.26V19.68c0-1.21-.26-2.1-.78-2.69-.52-.58-1.37-.87-2.55-.87-1.72 0-3 .78-3.84 2.34V32h-4.26V12.98h4.01zm24.38 19.37a8.8 8.8 0 0 1-6.57-2.56c-1.68-1.7-2.52-3.97-2.52-6.8v-.54c0-1.9.37-3.6 1.1-5.08a8.13 8.13 0 0 1 7.5-4.74c2.58 0 4.58.82 5.99 2.48 1.4 1.65 2.11 3.99 2.11 7.01v1.72h-12.4c.13 1.57.65 2.81 1.57 3.73s2.07 1.37 3.46 1.37c1.96 0 3.55-.79 4.77-2.37l2.3 2.2a7.68 7.68 0 0 1-3.04 2.64 9.5 9.5 0 0 1-4.27.94zm-.51-16.3c-1.17 0-2.11.42-2.83 1.24a6.38 6.38 0 0 0-1.38 3.42h8.12v-.31a5.2 5.2 0 0 0-1.14-3.25 3.56 3.56 0 0 0-2.77-1.1zm16.77-7.7v4.63h3.34v3.16H94v10.62c0 .73.15 1.25.43 1.57.3.33.8.49 1.54.49a6.55 6.55 0 0 0 1.49-.18v3.3c-.97.28-1.9.4-2.8.4-3.28 0-4.92-1.8-4.92-5.42V16.14h-3.12v-3.16h3.12V8.36H94zM105.13 32h-4.26V5h4.26zm9.17 0h-4.26V12.98h4.26zM109.8 8.04c0-.66.2-1.2.62-1.63.42-.44 1.01-.65 1.78-.65s1.37.21 1.8.65c.41.43.62.97.62 1.63a2.2 2.2 0 0 1-.63 1.6 2.4 2.4 0 0 1-1.79.65c-.77 0-1.36-.21-1.78-.64a2.22 2.22 0 0 1-.62-1.61zM120.45 32V16.14h-2.9v-3.16h2.9v-1.74c0-2.11.59-3.74 1.75-4.89 1.17-1.15 2.81-1.72 4.91-1.72.75 0 1.55.1 2.4.32l-.11 3.34a8.38 8.38 0 0 0-1.64-.14c-2.03 0-3.05 1.04-3.05 3.14v1.69h3.86v3.16h-3.86V32h-4.26zm17.87-6.12 3.86-12.9h4.54l-7.54 21.9c-1.16 3.2-3.12 4.8-5.9 4.8-.61 0-1.3-.1-2.04-.32v-3.3l.8.05c1.08 0 1.89-.2 2.43-.59a3.9 3.9 0 0 0 1.3-1.97l.6-1.64-6.66-18.93h4.6z'/%3e%3cpath fill-rule='nonzero' d='M27.89 14.13h-.02l-.02-.01a.11.11 0 0 1-.03-.1l.78-4.72 3.62 3.62-3.77 1.6a.08.08 0 0 1-.03.01h-.02l-.02-.01a1.72 1.72 0 0 0-.5-.38zm5.25-.28 3.88 3.87c.8.8 1.2 1.21 1.36 1.68l.05.2-9.26-3.92a.73.73 0 0 0-.02 0c-.03-.02-.08-.04-.08-.07s.05-.06.08-.07h.02zm5.13 7c-.2.38-.59.77-1.25 1.43l-4.37 4.37L27 25.47h-.03c-.05-.01-.1-.02-.1-.07a1.7 1.7 0 0 0-.66-1.2c-.02-.02-.01-.05 0-.08v-.02l1.06-6.52v-.03c0-.05.02-.1.06-.1a1.73 1.73 0 0 0 1.16-.67l.03-.03c.03-.01.07 0 .1.02l9.65 4.08zm-6.62 6.8-7.19 7.19 1.23-7.56v-.04c.02-.03.05-.04.07-.05h.01a1.85 1.85 0 0 0 .7-.52c.02-.03.05-.05.09-.06a.09.09 0 0 1 .03 0l5.06 1.04zm-8.71 8.7-.81.82-8.95-12.94a.42.42 0 0 0-.01-.02l-.03-.06.02-.04.01-.01.08-.13.02-.03c.01-.03.03-.05.05-.06.02-.01.05-.01.07 0l9.92 2.04a.16.16 0 0 1 .08.03l.02.05a1.76 1.76 0 0 0 1.03 1.17c.03.02.01.05 0 .08a.24.24 0 0 0-.01.05l-1.49 9.06zm-1.7 1.7c-.59.59-.94.9-1.34 1.03a2 2 0 0 1-1.2 0c-.47-.15-.87-.55-1.68-1.36l-9-8.99 2.36-3.64a.15.15 0 0 1 .04-.05c.02-.02.06-.01.09 0a2.43 2.43 0 0 0 1.64-.08c.02-.01.05-.02.07 0a.19.19 0 0 1 .03.03zM7.17 27.86 5.1 25.8l4.07-1.74a.08.08 0 0 1 .04 0c.03 0 .05.03.07.06a2.91 2.91 0 0 0 .13.18l.01.02v.05l-2.26 3.5zM4.18 24.9l-2.6-2.61c-.45-.45-.77-.77-1-1.05l7.94 1.65a.84.84 0 0 0 .03 0c.05.01.1.02.1.07s-.06.07-.1.09l-.03.01zm-4.05-5a2 2 0 0 1 .09-.5c.15-.46.55-.86 1.36-1.67l3.34-3.34a2175.53 2175.53 0 0 0 4.62 6.69c.03.04.06.08.03.1-.15.17-.3.34-.4.53a.16.16 0 0 1-.05.07h-.04L.13 19.89zm5.68-6.4 4.49-4.5c.42.2 1.96.84 3.33 1.42l2.29.97c.03.01.06.03.07.06v.06a2 2 0 0 0 .52 1.82c.03.03 0 .08-.02.11l-.02.03-4.56 7.06a.14.14 0 0 1-.04.05h-.09a2.27 2.27 0 0 0-.54-.07 3 3 0 0 0-.52.06h-.06a.21.21 0 0 1-.04-.06L5.8 13.5zm5.4-5.4 5.81-5.81c.8-.8 1.21-1.21 1.68-1.36a2 2 0 0 1 1.2 0c.47.15.87.55 1.68 1.36l1.26 1.26-4.14 6.4a.15.15 0 0 1-.04.05c-.03.02-.06 0-.1 0a2.1 2.1 0 0 0-1.91.37c-.03.03-.07.01-.1 0L11.2 8.09zm12.5-3.67 3.82 3.81-.92 5.7v.02a.14.14 0 0 1 0 .03l-.06.03a1.83 1.83 0 0 0-.54.28.15.15 0 0 0-.02.01l-.04.03a.11.11 0 0 1-.05 0l-5.81-2.48h-.02c-.03-.02-.08-.04-.08-.08a2.2 2.2 0 0 0-.3-.91c-.03-.05-.07-.1-.04-.14zm-3.93 8.6 5.45 2.31c.03.01.07.03.08.06a.1.1 0 0 1 0 .06c-.02.08-.03.17-.03.26v.15c0 .04-.04.06-.07.07h-.02l-12.14 5.18c-.01 0-.04 0-.05-.02-.03-.03 0-.07.02-.11a.76.76 0 0 0 .02-.02l4.48-6.94v-.01c.03-.04.06-.1.11-.1l.05.02.28.02c.68 0 1.3-.33 1.69-.9a.16.16 0 0 1 .03-.03c.03-.02.07-.01.1 0zm-6.25 9.19 12.28-5.24.04.02.18.15.03.02c.02.01.05.03.05.05v.03l-1.06 6.46v.03c0 .05-.01.1-.06.1a1.73 1.73 0 0 0-1.37.85l-.06.07h-.07l-9.8-2.02-.15-.52z'/%3e%3c/g%3e%3c/svg%3e);

    width: auto;
    height: 1.5rem;
    margin-left: .5rem;
}

@ArnaudBarre
Copy link
Member Author

That's a lot of good arguments. And yeah a lot of %20 will compress well so maybe that's simpler to always enable it see and later add a way to get a more compact version via argument (both options are correct I think given the rare usage of srcset for SVG anyways)
Yeah there is cases where an extra network request is better, but that's true for any assets, and the the goal of the builb.inlineAssetSize to get have good heuristic. But currently having the bar to 0 for SVG is quite bad for me

@bluwy
Copy link
Member

bluwy commented Oct 17, 2023

Thanks for the explanation too. Yeah in that case %20 also seems reasonable. So between keeping or %20, I think it wouldn't hurt much to use %20 for now.

If anyone feels strongly to use instead, I'm also fine with it as later if we found that %20 is more reliable, that would be a non-breaking change.

We can replace spaces with %20 for srcset here if we need to.

I think it would be safer to cover usages in frameworks too as those aren't covered by Vite by default.

@patak-dev
Copy link
Member

I also vote to use %20 and play safe first here.

@polarathene
Copy link

I haven't been following Vite for a while (due to working in non-frontend projects), but if this is strictly for building static HTML output, you could always have a post-process step afterwards that can take the %20 and replace it with space char as the appropriate context would be available by this point (the HTML)?

I don't know if that's something Vite would handle, or some community plugin, or third-party processing outside of Vite 🤷‍♂️

I believe I did that with the 404 page example (I recall using NextJS to output a static HTML + CSS build without JS, then putting it through some optimizers, one of those was for HTML (possibly was htmlnano), which all the CSS was also embedded into).

@bluwy bluwy added the p3-significant High priority enhancement (priority) label Oct 19, 2023
@bluwy bluwy added this to the 5.0 milestone Oct 19, 2023
@bluwy bluwy merged commit 5acda5e into main Oct 19, 2023
12 checks passed
@bluwy bluwy deleted the arnaud/inline-svg branch October 19, 2023 07:36
@bluwy
Copy link
Member

bluwy commented Oct 19, 2023

Ah dang it, I forgot to set the co-authors to credit the previous PRs. Sorry, appreciate the former work done here @princed and @aleclarson 🙏

@princed
Copy link

princed commented Oct 20, 2023

Thanks for making this happen @ArnaudBarre @bluwy! ❤️

Comment on lines +452 to +453
.replaceAll('<', '%3c')
.replaceAll('>', '%3e')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this feature, for the sake of which I'm switching to v5-beta without waiting for a release!

But these two characters don't need to be encoded. Here is an example where both characters work in all three major browser engines (and it seems like a long time ago).

I'm not sure if a pull request is needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! Effectively the wikipedia page says that this is not reserved: https://en.wikipedia.org/wiki/Percent-encoding
A bit more precision on wether it was at one point not supported and if is it like ie11 ages or three years ago will help decide on this change.
Anyway a PR for discussing this change is welcome, you can tag me in

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's required for the cases like url(data:image/svg+xml,<svg viewBox='0 0 32 16' xmlns='http://www.w3.org/2000/svg'><path d='M0 7v2h25v4l7-5-7-5v4z'/></svg>) (note that the value is not quoted).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should already be auto-quoting SVGs here, so I think it should be fine in most cases:

let newUrl = await replacer(rawUrl)
// The new url might need wrapping even if the original did not have it, e.g. if a space was added during replacement
if (wrap === '' && newUrl !== encodeURI(newUrl)) {
wrap = '"'
}
// If wrapping in single quotes and newUrl also contains single quotes, switch to double quotes.
// Give preference to double quotes since SVG inlining converts double quotes to single quotes.
if (wrap === "'" && newUrl.includes("'")) {
wrap = '"'
}
// Escape double quotes if they exist (they also tend to be rarer than single quotes)
if (wrap === '"' && newUrl.includes('"')) {
newUrl = newUrl.replace(nonEscapedDoubleQuoteRe, '\\"')
}
return `${funcName}(${wrap}${newUrl}${wrap})`

If the user passes the SVG string to a manually constructed url() through some JS framework, they would also need to double quote it to safely pass it today 🤔 I guess it's a tradeoff.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway a PR for discussing this change is welcome, you can tag me in

@ArnaudBarre, done: #14815 🙂

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Vue2. If it's written like something similar to this, it's going to be a problem.

export default class Overview extends Vue {
      @Prop({ type: Object, default: () => ({}) }) readonly config!: CardConfig;

      get iconStyle() {
        return {
            width: this.iconSize,
            height: this.defaultHeight + 'px',
            background: 'url(' + this.config.image + ')',
            backgroundSize: 'contain',
            backgroundRepeat: 'no-repeat',
            backgroundPositionY: 'center'
        };
    }
}

@taylorkline
Copy link

I was able to cherry pick this onto my local Vite 4.3.9 with patch-package. Working perfectly. Thank you!

@IanVS
Copy link
Contributor

IanVS commented Dec 7, 2023

Just a note in case anyone else hits it, but this change broke a few background images in my app due to my CSP not allowing the inlined SVG.

To address, I updated my CSP to include default-src 'self' data:;.

However, I'm unclear on whether that is safe or not. (not, according to this overflow answer: https://security.stackexchange.com/a/95011). Did the Vite team do any research into whether or not this can introduce security risks when y'all made this change? (Just so I can piggyback off your learnings.)

I'm now looking for a way to disable this behavior, but I don't see this change mentioned in the migration doc and I don't see any discussion about an option in this PR, so I'm not hopeful that I'll find anything.

@taylorkline
Copy link

Just a note in case anyone else hits it, but this change broke a few background images in my app due to my CSP not allowing the inlined SVG.

To address, I updated my CSP to include default-src 'self' data:;.

However, I'm unclear on whether that is safe or not. (not, according to this overflow answer: https://security.stackexchange.com/a/95011). Did the Vite team do any research into whether or not this can introduce security risks when y'all made this change? (Just so I can piggyback off your learnings.)

This seems overly permissive; I found img-src 'self' data:; to be sufficient. And, on the same link, this answer seems to argue that data: within img-src should be OK.

@firefoxic
Copy link

firefoxic commented Dec 7, 2023

@IanVS You can disable inlining SVG by setting the assetsInlineLimit option to 0. The default is 4096 (4KiB).
https://vitejs.dev/config/build-options.html#build-assetsinlinelimit

@IanVS
Copy link
Contributor

IanVS commented Dec 7, 2023

Thanks for the tips! I think using img-src is a good option. default-src also might work so long as script-src is also limited, but it seems unnecessary. And thanks for the info on how to disable it, @firefoxic!

@ArnaudBarre
Copy link
Member Author

Nope I didn't know/think that using inline assets would not have the same security content policy than remote assets and the data-type being ignored.

Inlining SVG is a build time feature, so you will need to use at build time untrusted user provided assets already on your disk which seems odds without proper validation so I'm unsure of this being a viable source of attack, but I understand that this can be problematic to relax this CSP for all assets displayed on a web page.

@diego-hourly
Copy link

This should be documented somewhere on the web. Spend quite some time figuring out why my svgs weren't working inlining in the styles. Now I realize that wrapping then in double quotes fixes the issue. 😆

@piotr-cz
Copy link

Tip: To disable inlining SVGs globally, use following config:

export default defineConfig({
  build: {
    assetsInlineLimit: filePath => filePath.endsWith('.svg') ? false : undefined,
  },
})

@juliankrieger
Copy link

@piotr-cz This does not seem to work. SVGs are still being inlined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support inlining SVG assets