feat: allow .svg files to be inlined #2909

princed · 2021-04-08T06:19:14Z

This is an updated version of #1716 by @aleclarson with the inclusion of SVG URL-escaping. Below is the original PR description:

Base64 is unnecessary for SVG files:
#1197 (comment)

Also using Buffer.byteLength instead of character length when comparing with the assetsInlineLimit option.

Closes #1204

princed · 2021-04-08T06:30:11Z

Okay, I didn't realise I needed to build before running the tests, so still has the same todo as the original PR:

avoid inlining svg fragments

polarathene

Hello, I'm providing a review as this is a feature of interest I've come across while evaluating adopting vite for projects.

I have a concern about the current implementation here breaking for srcSet (html) and image-set() (css) handling. The reference project you've based your PR on from here in the comment has an issue from me a while back where this was an issue an caused breakage.

It would be better to use the upstream package directly rather than duplicating a portion of it here imo. That project provides additional improvements along with a toSrcset() variant that will keep spaces encoded.

Either way, you need to encode the spaces for these two value types and ideally keep the optimized data url without encoded spaces everywhere else. A related vite issue that fixed processing for this type of source values is here. Hopefully that helps identify where to address the issue rather than introducing a bug.

Regarding the review comment about dropping ;utf-8 that is mentioned in a codepen blog post by the author of the same referenced package.

polarathene · 2021-04-25T10:16:34Z

packages/vite/src/node/plugins/asset.ts

-    // base64 inlined as a string
-    url = `data:${mime.getType(file)};base64,${content.toString('base64')}`
+    // svgs can be inlined without base64
+    url = file.endsWith('.svg')


An earlier issue I saw mentioned using .SVG as a workaround for the current exclusion; This PR will continue to process that asset with base64. Perhaps do a case-insensitive check here instead?

Has file been lower-cased prior to processing?

It doesn't seem so looking at cleanUrl(), when a maintainer reviews, they'll know if it should be handled here or better suited for lower-casing in cleanUrl() util for everything instead.

While /\.svg$/i.test(file) is more fool-proof, I don't think all-caps file extensions are particularly common?

I used plain endsWith merely to be consistent with the rest of the codebase. If someone ever decides to supports case-insensitive extensions it'll be an easy fix.

I don't think all-caps file extensions are particularly common?

It shouldn't be common no. Depends if you want to avoid it ever being an issue for users that do though.

Not a big deal, worse that happens is it gets encoded as base64 instead and if that bothers/confuses a user, they raise an issue and someone can point out lowercase extension is only supported or someone adds the fix by lowercasing the filename (only for checking the extension).

If someone ever decides to supports case-insensitive extensions it'll be an easy fix.

I don't know what you mean here? You just use toLowerCase() on the string.

url = file.toLowerCase().endsWith('.svg')

packages/vite/src/node/plugins/asset.ts

polarathene · 2021-04-25T10:40:50Z

packages/vite/src/node/plugins/asset.ts

+function specialHexEncode(match: string) {
+  // Browsers tolerate these characters, and they're frequent
+  switch (match) {
+    case '%20':
+      return ' '
+    case '%3D':
+      return '='
+    case '%3A':
+      return ':'
+    case '%2F':
+      return '/'
+    default:
+      return match.toLowerCase() // compresses better
+  }
+}
+
+const doubleQuoteRE = /"/g
+const whitespaceRE = /\s+/g
+const urlHexPairsRE = /%[\dA-F]{2}/g
+
+// Adopted from https://github.com/tigt/mini-svg-data-uri
+function uriEncodeSvg(content: string) {
+  const normalizedContent = content
+    .trim()
+    .replace(whitespaceRE, ' ')
+    .replace(doubleQuoteRE, "'")
+
+  return encodeURIComponent(normalizedContent).replace(
+    urlHexPairsRE,
+    specialHexEncode
+  )
+}


This is for the most part the referenced package logic, well a portion of it.

Any reason not to leverage the upstream package directly? It has a few extra improvements you're opting out of here, as well as the support for not breaking srcSet usage which the implementation you've provided here will fail to support.

I did not go with mini-svg-data-uri precisely because it does some micro-optimizations that aren't strictly necessary namely conversion from %20 to , which toSrcset has to undo as its only function.

However, since the team's sentiment seems to be towards using the upstream package I switched to that. I had to always use toSrcset though, because there no way to tell whether the Data URI string is going to be used in srcSet when importing into JavaScript.

I had to always use toSrcset though, because there no way to tell whether the Data URI string is going to be used in srcSet when importing into JavaScript.

That's fine 👍

If the detection via context of usage isn't available better that it works with all cases where white-space would otherwise cause a breakage (there is more than just srcSet where SVG can be provided and this would break without encoding spaces).

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

packages/vite/src/node/plugins/asset.ts

aleclarson · 2021-05-13T20:09:10Z

@polarathene Thank you for the review. Would you like to fork this PR and carry it forward?

Note that this PR still breaks svg fragments:

vite/packages/playground/assets/index.html

Lines 83 to 100 in 30ff5a2

    
           <h2>SVG Fragments</h2> 
        
           <div> 
        
             <img 
        
               class="svg-frag-img" 
        
               src="./nested/fragment.svg#icon-clock-view" 
        
               alt="" 
        
             /> 
        
             <img 
        
               class="svg-frag-img" 
        
               src="./nested/fragment.svg#icon-heart-view" 
        
               alt="" 
        
             /> 
        
             <img 
        
               class="svg-frag-img" 
        
               src="./nested/fragment.svg#icon-arrow-right-view" 
        
               alt="" 
        
             /> 
        
           </div>

See here for the related test suite:

vite/packages/playground/assets/__tests__/assets.spec.ts

Lines 149 to 167 in 30ff5a2

    
           describe('svg fragments', () => { 
        
             // 404 is checked already, so here we just ensure the urls end with #fragment 
        
             test('img url', async () => { 
        
               const img = await page.$('.svg-frag-img') 
        
               expect(await img.getAttribute('src')).toMatch(/svg#icon-clock-view$/) 
        
             }) 
        
             test('via css url()', async () => { 
        
               const bg = await page.evaluate(() => { 
        
                 return getComputedStyle(document.querySelector('.icon')).backgroundImage 
        
               }) 
        
               expect(bg).toMatch(/svg#icon-clock-view"\)$/) 
        
             }) 
        
             test('from js import', async () => { 
        
               const img = await page.$('.svg-frag-import') 
        
               expect(await img.getAttribute('src')).toMatch(/svg#icon-heart-view$/) 
        
             }) 
        
           })

Base64 is unnecessary for SVG files: https://github.com/vitejs/vite/issues/1197\#issuecomment-738780169 Also using `Buffer.byteLength` instead of character length when comparing with the `assetsInlineLimit` option. Closes #1204

princed · 2021-05-18T05:21:50Z

Would anyone mind approving workflows in this PR? @patak-js @Shinigami92 @antfu

Shinigami92 · 2021-05-18T05:32:42Z

Would anyone mind approving workflows in this PR? @patak-js @Shinigami92 @antfu

Please keep in mind that we are humans and therefore need sleep ^^ It's 7:30 AM in the morning right now and I just start my day in around 1-2h
Just wait around 24h before pinging half the team 😅

princed · 2021-05-18T05:50:43Z

@Shinigami92 Thank you for the approval, sorry for disturbing everyone, I didn't mean to!

Shinigami92 · 2021-05-18T05:53:05Z

Did you know you can test locally with yarn test-build (& yarn test-serve)? 🙂
You need to build the project before that with just yarn build (if your branch is up to date with latest main)

princed · 2021-05-18T05:57:02Z

I did exactly that, however I didn't realize it'd run the same tests twice 🙈
Also, some tests were flaking on me locally regularly 😢

aleclarson · 2021-05-18T14:41:02Z

packages/playground/assets/__tests__/assets.spec.ts

@@ -162,7 +162,9 @@ describe('svg fragments', () => {

  test('from js import', async () => {
    const img = await page.$('.svg-frag-import')
-    expect(await img.getAttribute('src')).toMatch(/svg#icon-heart-view$/)
+    expect(await img.getAttribute('src')).toMatch(
+      isBuild ? /svg%3e#icon-heart-view$/ : /svg#icon-heart-view$/


Where does the %3e come from, and why only in build mode?

@aleclarson it's the url-encoded version of > so the string being matched is svg>#icon-heart-view, whereas the non-build mode version hasn't had the SVG inlined, thus is still referencing the SVG by filename, instead of the closing svg tag, it's the svg extension.

I'm assuming this was tested in a browser that it actually works with the fragment and not just to pass the test though. I know that # needs to be url-encoded for inlining SVG usually, but perhaps it's not meant to be when appending the fragment identifier after the inlined SVG like you would append it after a file extension? I've never used fragments, but I'm interested to know if that works as intended.

IanVS · 2021-06-21T12:37:53Z

I'd love to have this feature in my project. I see that there is an approval from about a month ago and the branch needs a rebase. Is there anything else that needs to be done before this can be merged?

ValentinKuzub · 2021-07-20T17:25:00Z

We also really need this feature. Seems only to be logical to allow to inline the SVGs among other types of images. Any updates on this?

aleclarson · 2021-07-20T18:10:48Z

For this PR to be merged, someone needs to manually test it with SVG fragments and vite build to see if SVG fragments work as intended when the .svg is inlined.

ysael · 2021-08-17T18:45:26Z

For this PR to be merged, someone needs to manually test it with SVG fragments and vite build to see if SVG fragments work as intended when the .svg is inlined.

I could do that if you need? do you have any guidelines for testing this type of enhancements? thanks

joakimriedel · 2021-09-29T11:33:34Z

Found this PR while searching for some solution to why dynamically import()-ed SVG resources were built as two assets, one .js and one .svg even though they were less than assetsInlineLimit size.

From what I can see this PR has been idle for one month now; any news on when this might be merged?

EDIT: found a workaround in this thread so no rush even though it would be nice to have it out of the box!

hornta · 2021-11-11T21:24:54Z

@princed Are you still working on this? Do you need any help from any of us? Thanks.

aleclarson · 2021-11-11T21:31:39Z

@hornta see here: #2909 (comment)

userquin · 2021-11-11T21:52:03Z

why not delegate this to existing plugins? for example, unplugin-icons can serve the svg as raw or as a component, major frameworks supported, or unocss via preset-icon (Pure CSS Icons) that will do the same here but optimized (no base64 encoding required). Both plugins support custom svg loader/resolver, also including by default @iconify (10000+ icons).

EDIT: not including 10000+ icons, will support adding 10000+ on demand, just install @iconify-json/<collection> as dev dependency

aleclarson · 2021-11-11T22:01:15Z

why not delegate this to existing plugins?

because SVGs are assets, therefore assetsInlineLimit should inline them

no base64 encoding required

This PR does not use Base64 on SVGs

userquin · 2021-11-11T22:21:47Z

I'll try to test it...

userquin · 2021-11-12T14:34:47Z

@aleclarson

I just mimic the vite:assets plugin for build process here https://stackblitz.com/edit/vitejs-vite-y9k5yr, and seems to not work when including the hash, when using this svg request import topUrlSvg from '/src/assets/fragment-ready-4.svg?url#icon-clock-view'; see first build output below.

Run npm run dev on terminal if the repro doesn't start when opening it.

With import topUrlSvg from '/src/assets/fragment-ready-4.svg?url'; works, but with this one not working: import topUrlSvg from '/src/assets/fragment-ready-4.svg?url#icon-clock-view';, on dev both works.

❯ npm run build
$ vue-tsc --noEmit && vite build
src/App.vue:4:23 - error TS2307: Cannot find module '/src/assets/fragment-ready-4.svg?url#icon-clock-view' or its corresponding type declarations.

4 import topUrlSvg from '/src/assets/fragment-ready-4.svg?url#icon-clock-view';
                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Found 1 error.


~/projects/vitejs-vite-y9k5yr
❯ npm run build
$ vue-tsc --noEmit && vite build
vite v2.6.14 building for production...
transforming (1) index.htmlMATCHED: /home/projects/vitejs-vite-y9k5yr/src/assets/fragment-ready-4.svg?url
✓ 12 modules transformed.
dist/assets/fragment-ready-4.edfb3e5c.svg   1.15 KiB
dist/index.html                             0.48 KiB
dist/assets/index.5c25b9b4.js               3.68 KiB / gzip: 1.60 KiB
dist/assets/index.f295bcc2.css              0.17 KiB / gzip: 0.13 KiB
dist/assets/vendor.2c023ab2.js              49.20 KiB / gzip: 19.81 KiB

~/projects/vitejs-vite-y9k5yr 6s
❯

userquin · 2021-11-12T14:37:48Z

I'll clone the repo for this PR and check using the modified vite.

userquin · 2021-11-12T16:12:17Z

the repro I provide is not a repro for this PR, since this PR only handles the inline svg when there is no hash, but should handle if it is a svg:

  let url: string
  if (
    file.endsWith('.svg') ||
    ((config.build.lib ||
      Buffer.byteLength(content) < config.build.assetsInlineLimit) &&
      hash == null)
  ) {
    // svgs can be inlined without base64
    url = file.endsWith('.svg')
      ? // The only difference between the default method and `toSrcset` is that
        // the latter encodes spaces as `%20`, so it's safer to always use it
        // to support `srcset` use-case even when svg is imported into JavaScript
        svgToTinyDataUri.toSrcset(content.toString())
      : // base64 inlined as a string
        `data:${mime.getType(file)};base64,${content.toString('base64')}`

In fact, on dev it is not serving the svg inlined (data-url), so we're not testing the same problem (I'll change also fileToUrl on assets plugin to do some local testing).

I change packages/playground/assets/index.html adding this entries:

  import svgFragUrl from './nested/fragment.svg?url'
  text('.svg-frag-import-js-url', svgFragUrl)
  document.querySelector('.svg-frag-import-url').style = `background: url("${svgFragUrl}") no-repeat;`

  import svgFragUrlHash from './nested/fragment.svg?url#icon-heart-view'
  text('.svg-frag-import-js-url-hash', svgFragUrlHash)
  document.querySelector('.svg-frag-import-url-hash').style = `background: url("${svgFragUrlHash}") no-repeat;`

and the html tag:

<h2>SVG Fragments via URL JS Import</h2>
<div>
  <p>Imported path: <code class="svg-frag-import-js-url"></code></p>
  <img class="svg-frag-import-url" alt="" />
</div>

<h2>SVG Fragments via URL HASH JS Import</h2>
<div>
  <p>Imported path: <code class="svg-frag-import-js-url-hash"></code></p>
  <img class="svg-frag-import-url-hash" alt="" />
</div>

Without changing fileToUrl:

Changing fileToUrl with:

results on:

userquin · 2021-11-12T16:13:55Z

I'll test build, I don't know why machine never finish the build from the assets playground, and finish the test.

Once done, I'll push to my repo the changes (without the tests) and put the link here.

userquin · 2021-11-12T16:35:31Z

Uhmm, the build script is watching, this is why the build never finish...

The result for pnpm run build, the problem is with style added for the bg image:

pnpm run build

> test-assets@0.0.0 build F:\work\projects\quini\GitHub\vite\packages\playground\assets
> vite build

vite v2.7.0-beta.4 building for production...

watching for file changes...

build started...
✓ 16 modules transformed.
[vite:css-post] Transform failed with 3 errors:
<stdin>:91:57: error: Expected ")" to end URL token
<stdin>:95:57: error: Expected ")" to end URL token
<stdin>:99:57: error: Expected ")" to end URL token

userquin · 2021-11-12T16:39:34Z

I push a draft PR to test this PR: #5658

madeleineostoja · 2022-02-10T05:11:17Z

Any update on this? Would be great to get native inline SVG handling in Vite, I'm currently doing some pretty hacky stuff to get them working in Sveltekit

wardbeyens · 2022-12-04T19:27:00Z

Any update on this? Would be great to get native inline SVG handling in Vite, I'm currently doing some pretty hacky stuff to get them working in Sveltekit

Do you mean something like this?

export const customSvgLoader = (options) => {
  return {
    name: 'vite-svg-patch-plugin',
    transform: function (code, id) {
      if (id.endsWith('.svg')) {
        const extractedSvg = fs.readFileSync(id, 'utf8');
        return `export default '${getExtractedSVG(extractedSvg, options)}'`;
      }
      return code;
    }
  };
};

nikeee · 2023-02-17T01:47:30Z

Any update on this? Inlining them would be great and the proposed hacks don't seem stable to me.

princed mentioned this pull request Apr 8, 2021

feat: allow .svg files to be inlined #1716

Closed

Shinigami92 added enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority) labels Apr 8, 2021

polarathene suggested changes Apr 25, 2021

View reviewed changes

gossi mentioned this pull request Jan 21, 2023

Svelting the UI theemo-tokens/figma-theemo#34

Closed

This comment has been minimized.

Sign in to view

aleclarson and others added 3 commits May 17, 2021 18:02

feat: allow .svg files to be inlined

685bdf7

Base64 is unnecessary for SVG files: https://github.com/vitejs/vite/issues/1197\#issuecomment-738780169 Also using `Buffer.byteLength` instead of character length when comparing with the `assetsInlineLimit` option. Closes #1204

fix: support fragments

1bb658f

fix: review feedback

0d03f5f

princed requested a review from aleclarson May 18, 2021 01:19

fix: support built tests

d62b148

Shinigami92 approved these changes May 18, 2021

View reviewed changes

Shinigami92 requested review from patak-dev, antfu and nihalgonsalves May 18, 2021 06:23

polarathene approved these changes May 18, 2021

View reviewed changes

aleclarson reviewed May 18, 2021

View reviewed changes

ysael approved these changes Aug 17, 2021

View reviewed changes

Niputi added the needs rebase label Nov 11, 2021

userquin mentioned this pull request Nov 12, 2021

test: branch for testing PR 2909 #5658

Closed

bluwy mentioned this pull request Jun 26, 2022

fix: skip inline html #8789

Merged

9 tasks

seivan mentioned this pull request Aug 2, 2022

feat: support callback for build.assetsInlineLimit #8717

Closed

13 tasks

ArnaudBarre mentioned this pull request Oct 16, 2023

feat(build)!: inline SVGs #14643

Merged

bluwy closed this in #14643 Oct 19, 2023

princed deleted the feat/inline-svg branch October 20, 2023 02:27

feat: allow .svg files to be inlined #2909

feat: allow .svg files to be inlined #2909

Conversation

princed commented Apr 8, 2021

princed commented Apr 8, 2021 • edited Loading

polarathene 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

polarathene May 18, 2021 • edited Loading

Choose a reason for hiding this comment

aleclarson commented May 13, 2021 • edited Loading

This comment has been minimized.

princed commented May 18, 2021 • edited Loading

Shinigami92 commented May 18, 2021

princed commented May 18, 2021

Shinigami92 commented May 18, 2021

princed commented May 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanVS commented Jun 21, 2021

ValentinKuzub commented Jul 20, 2021

aleclarson commented Jul 20, 2021

ysael commented Aug 17, 2021

joakimriedel commented Sep 29, 2021 • edited Loading

hornta commented Nov 11, 2021

aleclarson commented Nov 11, 2021

userquin commented Nov 11, 2021 • edited Loading

aleclarson commented Nov 11, 2021

userquin commented Nov 11, 2021

userquin commented Nov 12, 2021

userquin commented Nov 12, 2021

userquin commented Nov 12, 2021

userquin commented Nov 12, 2021

userquin commented Nov 12, 2021

userquin commented Nov 12, 2021

madeleineostoja commented Feb 10, 2022 • edited Loading

wardbeyens commented Dec 4, 2022

nikeee commented Feb 17, 2023

princed commented Apr 8, 2021 •

edited

Loading

polarathene May 18, 2021 •

edited

Loading

aleclarson commented May 13, 2021 •

edited

Loading

princed commented May 18, 2021 •

edited

Loading

joakimriedel commented Sep 29, 2021 •

edited

Loading

userquin commented Nov 11, 2021 •

edited

Loading

madeleineostoja commented Feb 10, 2022 •

edited

Loading