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.

Sign up for GitHub

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

Jump to bottom

feat(build)!: inline SVGs #14643

Merged
merged 2 commits into from
Oct 19, 2023
Merged

feat(build)!: inline SVGs #14643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
415ee55
feat(build)!: inline SVGs
ArnaudBarre Oct 16, 2023
2eee0ec
CR
ArnaudBarre Oct 18, 2023
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
38 changes: 34 additions & 4 deletions packages/vite/src/node/plugins/asset.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,8 @@ async function fileToBuiltUrl(
let url: string
if (
config.build.lib ||
(!file.endsWith('.svg') &&
// Don't inline SVG with fragments, as they are meant to be reused
(!(file.endsWith('.svg') && id.includes('#')) &&
!file.endsWith('.html') &&
content.length < Number(config.build.assetsInlineLimit) &&
!isGitLfsPlaceholder(content))
Expand All @@ -382,9 +383,13 @@ async function fileToBuiltUrl(
)
}

const mimeType = mrmime.lookup(file) ?? 'application/octet-stream'
// base64 inlined as a string
url = `data:${mimeType};base64,${content.toString('base64')}`
if (file.endsWith('.svg')) {
url = svgToDataURL(content)
} else {
const mimeType = mrmime.lookup(file) ?? 'application/octet-stream'
// base64 inlined as a string
url = `data:${mimeType};base64,${content.toString('base64')}`
}
} else {
// emit as asset
const { search, hash } = parseUrl(id)
Expand Down Expand Up @@ -428,3 +433,28 @@ export async function urlToBuiltUrl(
true,
)
}

// Inspired by https://github.com/iconify/iconify/blob/main/packages/utils/src/svg/url.ts
function svgToDataURL(content: Buffer): string {
const stringContent = content.toString()
// If the SVG contains some text, any transformation is unsafe, and given that double quotes would then
// need to be escaped, the gain to use a data URI would be ridiculous if not negative
if (stringContent.includes('<text')) {
return `data:image/svg+xml;base64,${content.toString('base64')}`
} else {
return (
'data:image/svg+xml,' +
stringContent
.trim()
.replaceAll('"', "'")
.replaceAll('%', '%25')
.replaceAll('#', '%23')
.replaceAll('<', '%3c')
.replaceAll('>', '%3e')
Comment on lines +452 to +453
Copy link

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:

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

Lines 1522 to 1536 in 7881457

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.

Copy link

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'
        };
    }
}

// Spaces are not valid in srcset it has some use cases
// it can make the uncompressed URI slightly higher than base64, but will compress way better
// https://github.com/vitejs/vite/pull/14643#issuecomment-1766288673
.replaceAll(/\s+/g, '%20')
)
}
}
7 changes: 6 additions & 1 deletion playground/assets/__tests__/assets.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,12 @@ 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
? // Assert trimmed (data URI starts with < and ends with >)
/^data:image\/svg\+xml,%3c.*%3e#icon-heart-view$/
: /svg#icon-heart-view$/,
)
})
})

Expand Down
1 change: 1 addition & 0 deletions playground/legacy/vite.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export default defineConfig({
cssCodeSplit: false,
manifest: true,
sourcemap: true,
assetsInlineLimit: 100, // keep SVG as assets URL
rollupOptions: {
input: {
index: path.resolve(__dirname, 'index.html'),
Expand Down
1 change: 1 addition & 0 deletions playground/worker/vite.config-es.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export default defineConfig({
},
build: {
outDir: 'dist/es',
assetsInlineLimit: 100, // keep SVG as assets URL
rollupOptions: {
output: {
assetFileNames: 'assets/[name].[ext]',
Expand Down
1 change: 1 addition & 0 deletions playground/worker/vite.config-iife.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export default defineConfig({
},
build: {
outDir: 'dist/iife',
assetsInlineLimit: 100, // keep SVG as assets URL
manifest: true,
rollupOptions: {
output: {
Expand Down
1 change: 1 addition & 0 deletions playground/worker/vite.config-relative-base-iife.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export default defineConfig({
},
build: {
outDir: 'dist/relative-base-iife',
assetsInlineLimit: 100, // keep SVG as assets URL
rollupOptions: {
output: {
assetFileNames: 'other-assets/[name]-[hash].[ext]',
Expand Down
1 change: 1 addition & 0 deletions playground/worker/vite.config-relative-base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export default defineConfig({
},
build: {
outDir: 'dist/relative-base',
assetsInlineLimit: 100, // keep SVG as assets URL
rollupOptions: {
output: {
assetFileNames: 'other-assets/[name]-[hash].[ext]',
Expand Down
1 change: 1 addition & 0 deletions playground/worker/worker-sourcemap-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export default (sourcemap) => {
},
build: {
outDir: `dist/iife-${typeName}/`,
assetsInlineLimit: 100, // keep SVG as assets URL
sourcemap: sourcemap,
rollupOptions: {
output: {
Expand Down