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

feat: support callback for build.assetsInlineLimit #8717

Closed
wants to merge 2 commits into from
Closed

feat: support callback for build.assetsInlineLimit #8717

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
17ebf9a
feat: Adding callback to `assetsInlineLimit`. Passes the `filePath: s…
seivan Jun 22, 2022
2eef2a8
Merge branch 'main' into feature/shouldInlineAssets
seivan Jul 4, 2022
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 docs/config/build-options.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,12 @@ Specify the directory to nest generated assets under (relative to `build.outDir`

## build.assetsInlineLimit

- **Type:** `number`
- **Default:** `4096` (4kb)
- **Type:** `number` | `((filePath: string, size: number, totalBundledSize: number) => boolean)`
- **Default:** `6144` (6kb)

Imported or referenced assets that are smaller than this threshold will be inlined as base64 URLs to avoid extra http requests. Set to `0` to disable inlining altogether.
Can also implement a function that return boolean if it should bundled.
Passes the `filePath: string`, `contentSize: number` and currently accrued bundled size `totalBundledSize: number`

::: tip Note
If you specify `build.lib`, `build.assetsInlineLimit` will be ignored and assets will always be inlined, regardless of file size.
Expand Down
10 changes: 7 additions & 3 deletions packages/vite/src/node/build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,14 @@ export interface BuildOptions {
assetsDir?: string
/**
* Static asset files smaller than this number (in bytes) will be inlined as
* base64 strings. Default limit is `4096` (4kb). Set to `0` to disable.
* base64 strings. Default limit is `6144` (6kb). Set to `0` to disable.
* Can also implement a function that return boolean if it should bundled.
* Passes the `filePath`, `contentSize` and currently accrued bundled size `totalBundledSize`
* @default 4096
*/
assetsInlineLimit?: number
assetsInlineLimit?:
| number
| ((filePath: string, size: number, totalBundledSize: number) => boolean)
/**
* Whether to code-split CSS. When enabled, CSS in async chunks will be
* inlined as strings in the chunk and inserted via dynamically created
Expand Down Expand Up @@ -239,7 +243,7 @@ export function resolveBuildOptions(
polyfillModulePreload: true,
outDir: 'dist',
assetsDir: 'assets',
assetsInlineLimit: 4096,
assetsInlineLimit: 6144,
cssCodeSplit: !raw?.lib,
cssTarget: false,
sourcemap: false,
Expand Down
2 changes: 1 addition & 1 deletion packages/vite/src/node/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ cli
)
.option(
'--assetsInlineLimit <number>',
`[number] static asset base64 inline threshold in bytes (default: 4096)`
`[number] static asset base64 inline threshold in bytes (default: 6144)`
)
.option(
'--ssr [entry]',
Expand Down
136 changes: 89 additions & 47 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 @@ -15,7 +15,10 @@ export const assetUrlRE = /__VITE_ASSET__([a-z\d]{8})__(?:\$_(.*?)__)?/g
const rawRE = /(\?|&)raw(?:&|$)/
const urlRE = /(\?|&)url(?:&|$)/

const assetCache = new WeakMap<ResolvedConfig, Map<string, string>>()
const assetCache = new WeakMap<
ResolvedConfig,
Map<string, { url: string; size: number }>
>()

const assetHashToFilenameMap = new WeakMap<
ResolvedConfig,
Expand Down Expand Up @@ -364,6 +367,11 @@ export function publicFileToBuiltUrl(
return `__VITE_PUBLIC_ASSET__${hash}__`
}

const byteSizeOf = (function () {
const encoder = new TextEncoder()
const encode = encoder.encode.bind(encoder)
return (input: string) => encode(input).length
})()
/**
* Register an asset to be emitted as part of the bundle (if necessary)
* and returns the resolved public URL
Expand All @@ -381,63 +389,97 @@ async function fileToBuiltUrl(
const cache = assetCache.get(config)!
const cached = cache.get(id)
if (cached) {
return cached
return cached.url
}

const file = cleanUrl(id)
const content = await fsp.readFile(file)

let url: string
if (
config.build.lib ||
(!file.endsWith('.svg') &&
!file.endsWith('.html') &&
content.length < Number(config.build.assetsInlineLimit))
) {
const mimeType = mrmime.lookup(file) ?? 'application/octet-stream'
// base64 inlined as a string
url = `data:${mimeType};base64,${content.toString('base64')}`
} else {
// emit as asset
// rollup supports `import.meta.ROLLUP_FILE_URL_*`, but it generates code
// that uses runtime url sniffing and it can be verbose when targeting
// non-module format. It also fails to cascade the asset content change
// into the chunk's hash, so we have to do our own content hashing here.
// https://bundlers.tooling.report/hashing/asset-cascade/
// https://github.com/rollup/rollup/issues/3415
const map = assetHashToFilenameMap.get(config)!
const contentHash = getHash(content)
const { search, hash } = parseUrl(id)
const postfix = (search || '') + (hash || '')

const fileName = assetFileNamesToFileName(
resolveAssetFileNames(config),
file,
contentHash,
content
)
if (!map.has(contentHash)) {
map.set(contentHash, fileName)
}
const emittedSet = emittedHashMap.get(config)!
if (!emittedSet.has(contentHash)) {
const name = normalizePath(path.relative(config.root, file))
pluginContext.emitFile({
name,
fileName,
type: 'asset',
source: content
})
emittedSet.add(contentHash)
let size: number

/*
lib should always inlined
svg should never be inlined (unless lib)
*/
if (config.build.lib) {
url = fileToInlinedAsset(file, content)
size = 0
} else if (file.endsWith('.svg') === false) {
const inlinedURL = fileToInlinedAsset(file, content)
Copy link
Member

Choose a reason for hiding this comment

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

This is running filetoInlinedAsset on every asset file even if it doesn't end up get inlined, seems wasteful especially when there are many larger files that don't need to be inlined.

I think we should just use the raw buffer size like before.

Copy link
Author

@seivan seivan Sep 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@yyx990803 Fair enough, but being accurate with size is inherently important as many small add up.
However I got the same feedback internally and the current design is to just pass an object with various methods

So instead of passing three arguments, it's one argument with "heavy" stuff as computed on demand;

{
  name: string
  bufferSize: number
  size(): number
  totalSize(): number
}

And it isn't additional code/complexity either, which is nice.
Though I would want filePath in addition to name. Sometimes you're just looking for that one file, and other times it's all the files in a path.

Let the user decide if it's wasteful or not, at least Vite won't be doing it now unless opt-in, and even then you can choose to do it for e.g just assets/*

Will that work for you?

Copy link
Member

Choose a reason for hiding this comment

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

I think the object is a good idea, but I don't think we should expose size() here. It is a clever trick but we will be allowing users to easily reach for inefficient code. For most use cases, it is ok to use the file size and we should promote that.
I think we should have the object but with a plain size prop and not totalSize.

Copy link
Author

@seivan seivan Sep 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@patak-dev The problem is bufferSize doesn't match the REAL size and that is the concern this PR is trying to solve.

We have a bunch of assets, some should be inlined and some should not.
File name matters (explicit inlining)
Paths matter (glob inlining)
Size matters (It's too big, opt out for this one)
TotalSize (We're reaching our limit, stop inlining)

Now, all of these could inherently be done by the user in the callback, but it would cumbersome.

for instance, totalSize is very easy to do outside, just totalSize += realSize outside the callback.
But expecting the user to know and do

  1. That the current size given to you is a lie
  2. Getting the real size from said number

That being said, I've already written this for myself, so it's not a big deal.

Yeah I agree having an object there makes more sense for any further information needed to be passed down.
At the very least, using a fixed static size as a boolean to decide is broken so that would be fixed regardless of what data is sent.

Your call
👍🏼

That being said, I need help with a few things:

What's the consensus on #2909 - should SVG be under the inline assets umbrella without base64?

I might need some help resolving the conflict.

Copy link
Member

Choose a reason for hiding this comment

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

As you say, if someone needs the totalSize, it can be computed externally. I don't think it is common enough to justify having it in core.

And the same goes for the real size, we can be more clear in the docs and add a rationale about why the API is using the file size.

About #2909, I think it should but that PR should be merged first, no? We can already move with this PR before that IIUC.

Let me know if you try to resolve the conflict and you get blocked and I'll check it out 👍🏼

const inlinedSize: number = byteSizeOf(inlinedURL)

const assetInlineLimit = config.build.assetsInlineLimit ?? 0

const shouldInline =
typeof assetInlineLimit === 'number'
? inlinedSize < Number(assetInlineLimit)
: assetInlineLimit(
file,
inlinedSize,
[...cache.values()].reduce((memo, { size }) => memo + size, 0)
)

if (shouldInline) {
size = inlinedSize
url = inlinedURL
}

url = `__VITE_ASSET__${contentHash}__${postfix ? `$_${postfix}__` : ``}` // TODO_BASE
}

cache.set(id, url)
url ??= fileToLinkedAsset(id, config, pluginContext, file, content)
size ||= 0

cache.set(id, { url, size })
return url
}

function fileToInlinedAsset(file: string, content: Buffer): string {
const mimeType = mrmime.lookup(file) ?? 'application/octet-stream'
return `data:${mimeType};base64,${content.toString('base64')}`
}

function fileToLinkedAsset(
id: string,
config: ResolvedConfig,
pluginContext: PluginContext,
file: string,
content: Buffer
): string {
// emit as asset
// rollup supports `import.meta.ROLLUP_FILE_URL_*`, but it generates code
// that uses runtime url sniffing and it can be verbose when targeting
// non-module format. It also fails to cascade the asset content change
// into the chunk's hash, so we have to do our own content hashing here.
// https://bundlers.tooling.report/hashing/asset-cascade/
// https://github.com/rollup/rollup/issues/3415
const map = assetHashToFilenameMap.get(config)!
const contentHash = getHash(content)
const { search, hash } = parseUrl(id)
const postfix = (search || '') + (hash || '')

const fileName = assetFileNamesToFileName(
resolveAssetFileNames(config),
file,
contentHash,
content
)
if (!map.has(contentHash)) {
map.set(contentHash, fileName)
}
const emittedSet = emittedHashMap.get(config)!
if (!emittedSet.has(contentHash)) {
const name = normalizePath(path.relative(config.root, file))
pluginContext.emitFile({
name,
fileName,
type: 'asset',
source: content
})
emittedSet.add(contentHash)
}

return `__VITE_ASSET__${contentHash}__${postfix ? `$_${postfix}__` : ``}` // TODO_BASE
}

export async function urlToBuiltUrl(
url: string,
importer: string,
Expand Down
6 changes: 5 additions & 1 deletion playground/css/vite.config-relative-base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* @type {import('vite').UserConfig}
*/

let totalSize = 0
const baseConfig = require('./vite.config.js')
module.exports = {
...baseConfig,
Expand All @@ -11,7 +12,10 @@ module.exports = {
outDir: 'dist/relative-base',
watch: false,
minify: false,
assetsInlineLimit: 0,
assetsInlineLimit: (_file, fileSize, combinedSize) => {
totalSize += fileSize
return true && totalSize === combinedSize + fileSize
},
rollupOptions: {
output: {
entryFileNames: 'entries/[name].js',
Expand Down
4 changes: 3 additions & 1 deletion playground/wasm/vite.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ export default defineConfig({
build: {
// make can no emit light.wasm
// and emit add.wasm
assetsInlineLimit: 80
assetsInlineLimit: (_file: string, _size: number, _totalSize: number) => {
return true
}
}
})