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: allow .svg files to be inlined #2909

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/playground/assets/__tests__/assets.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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$/
Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

)
})
})

Expand All @@ -189,7 +191,7 @@ if (isBuild) {
for (const file of listAssets('foo')) {
if (file.endsWith('.css')) {
expect(entry.css).toContain(`assets/${file}`)
} else if (!file.endsWith('.js')) {
} else if (!file.endsWith('.js') && !file.endsWith('.svg')) {
expect(entry.assets).toContain(`assets/${file}`)
}
}
Expand Down
4 changes: 3 additions & 1 deletion packages/playground/vue/__tests__/vue.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,9 @@ describe('asset reference', () => {

test('svg fragment', async () => {
const img = await page.$('.svg-frag')
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$/
)
})

test('relative url from <style>', async () => {
Expand Down
29 changes: 29 additions & 0 deletions packages/vite/LICENSE.md
Original file line number Diff line number Diff line change
Expand Up @@ -3076,6 +3076,35 @@ Repository: sindresorhus/mimic-fn

---------------------------------------

## mini-svg-data-uri
License: MIT
By: Taylor “Tigt” Hunt
Repository: git+https://github.com/tigt/mini-svg-data-uri.git

> MIT License
>
> Copyright (c) 2018 Taylor Hunt
>
> Permission is hereby granted, free of charge, to any person obtaining a copy
> of this software and associated documentation files (the "Software"), to deal
> in the Software without restriction, including without limitation the rights
> to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> copies of the Software, and to permit persons to whom the Software is
> furnished to do so, subject to the following conditions:
>
> The above copyright notice and this permission notice shall be included in all
> copies or substantial portions of the Software.
>
> THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> SOFTWARE.

---------------------------------------

## minimatch
License: ISC
By: Isaac Z. Schlueter
Expand Down
1 change: 1 addition & 0 deletions packages/vite/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@
"launch-editor-middleware": "^2.2.1",
"magic-string": "^0.25.7",
"mime": "^2.4.7",
"mini-svg-data-uri": "^1.3.3",
"minimatch": "^3.0.4",
"okie": "^1.0.1",
"open": "^7.4.2",
Expand Down
17 changes: 12 additions & 5 deletions packages/vite/src/node/plugins/asset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { cleanUrl } from '../utils'
import { FS_PREFIX } from '../constants'
import { PluginContext, RenderedChunk } from 'rollup'
import MagicString from 'magic-string'
import svgToTinyDataUri from 'mini-svg-data-uri'
import { createHash } from 'crypto'

export const assetUrlRE = /__VITE_ASSET__([a-z\d]{8})__(?:\$_(.*?)__)?/g
Expand Down Expand Up @@ -207,12 +208,18 @@ async function fileToBuiltUrl(

let url
if (
config.build.lib ||
(!file.endsWith('.svg') &&
content.length < Number(config.build.assetsInlineLimit))
(config.build.lib ||
Buffer.byteLength(content) < config.build.assetsInlineLimit) &&
hash == null
) {
// base64 inlined as a string
url = `data:${mime.getType(file)};base64,${content.toString('base64')}`
// svgs can be inlined without base64
url = file.endsWith('.svg')

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

? // 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')}`
} else {
// emit as asset
// rollup supports `import.meta.ROLLUP_FILE_URL_*`, but it generates code
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5447,6 +5447,11 @@ mini-create-react-context@^0.4.0:
"@babel/runtime" "^7.12.1"
tiny-warning "^1.0.3"

mini-svg-data-uri@^1.3.3:
version "1.3.3"
resolved "https://registry.yarnpkg.com/mini-svg-data-uri/-/mini-svg-data-uri-1.3.3.tgz#91d2c09f45e056e5e1043340b8b37ba7b50f4fac"
integrity sha512-+fA2oRcR1dJI/7ITmeQJDrYWks0wodlOz0pAEhKYJ2IVc1z0AnwJUsKY2fzFmPAM3Jo9J0rBx8JAA9QQSJ5PuA==

minimatch@^3.0.4:
version "3.0.4"
resolved "https://registry.yarnpkg.com/minimatch/-/minimatch-3.0.4.tgz#5166e286457f03306064be5497e8dbb0c3d32083"
Expand Down