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

fix: Skip inlining Git LFS placeholders (fix #9714) #9795

Merged
merged 2 commits into from Aug 24, 2022
Merged

fix: Skip inlining Git LFS placeholders (fix #9714) #9795

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
7f880dd
fix: Skip inlining Git LFS placeholders
MajorBreakfast Aug 23, 2022
bd14f70
fix: Emit warning when inlining Git LFS placeholder during lib build
MajorBreakfast Aug 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion docs/config/build-options.md
View file
Open in desktop
Expand Up @@ -53,8 +53,10 @@ Specify the directory to nest generated assets under (relative to `build.outDir`

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.

Git LFS placeholders are automatically excluded from inlining because they do not contain the content of the file they represent.

::: tip Note
If you specify `build.lib`, `build.assetsInlineLimit` will be ignored and assets will always be inlined, regardless of file size.
If you specify `build.lib`, `build.assetsInlineLimit` will be ignored and assets will always be inlined, regardless of file size or being a Git LFS placeholder.
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but we should find a way to avoid this in lib mode too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, then there's a strong desire to inline everything in lib mode. Hence there are essentially three ways this can behave:

  • Inline the LFS placeholder. Con: The resulting bundle is bricked. The LFS placeholder ins't the actual file content. It's how it behaves today.
  • Skip inlining for the LFS placeholder. Con: Lib mode should bundle everything. So that would be an exception to that rule
  • Display an error to the user. Con: It's an error

In regards to dealing with the situation in this PR there's these options:

  • Keep current behavior and document it
  • Keep current behavior but remove the highlighted sentence from the docs. Then the exact behavior for this edge case is in no longer documented and can be considered undefined behavior. Undefined behavior can be adjusted later without the need of a breaking change
  • Pick another option from three possible behaviors above and document it

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could keep the behaviour like in this PR, but if we encounter git lfs in lib build, we can warn about it to download the assets, as it's unlikely someone wants to build for a library that contains unreferenced assets (e.g. git lfs wouldn't work after npm publish)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. I'll see if I can make it produce a warning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed the code that emits a warning when encountering this edge case. I doubt that anyone will ever see this, though, because it seems rather unusual/unlikely that someone would use Git LFS in a library project :D

:::

## build.cssCodeSplit
Expand Down
2 changes: 2 additions & 0 deletions docs/guide/assets.md
View file
Open in desktop
Expand Up @@ -26,6 +26,8 @@ The behavior is similar to webpack's `file-loader`. The difference is that the i

- Assets smaller in bytes than the [`assetsInlineLimit` option](/config/build-options.md#build-assetsinlinelimit) will be inlined as base64 data URLs.

- Git LFS placeholders are automatically excluded from inlining because they do not contain the content of the file they represent. To get inlining, make sure to download the file contents via Git LFS before building.

### Explicit URL Imports

Assets that are not included in the internal list or in `assetsInclude`, can be explicitly imported as a URL using the `?url` suffix. This is useful, for example, to import [Houdini Paint Worklets](https://houdini.how/usage).
Expand Down
18 changes: 17 additions & 1 deletion packages/vite/src/node/plugins/asset.ts
View file
Open in desktop
@@ -1,6 +1,7 @@
import path from 'node:path'
import { parse as parseUrl } from 'node:url'
import fs, { promises as fsp } from 'node:fs'
import { Buffer } from 'node:buffer'
import * as mrmime from 'mrmime'
import type {
NormalizedOutputOptions,
Expand All @@ -10,6 +11,7 @@ import type {
RenderedChunk
} from 'rollup'
import MagicString from 'magic-string'
import colors from 'picocolors'
import { toOutputFilePathInString } from '../build'
import type { Plugin } from '../plugin'
import type { ResolvedConfig } from '../config'
Expand Down Expand Up @@ -398,6 +400,13 @@ export function publicFileToBuiltUrl(
return `__VITE_PUBLIC_ASSET__${hash}__`
}

const GIT_LFS_PREFIX = Buffer.from('version https://git-lfs.github.com')
function isGitLfsPlaceholder(content: Buffer): boolean {
if (content.length < GIT_LFS_PREFIX.length) return false
// Check whether the content begins with the characteristic string of Git LFS placeholders
return GIT_LFS_PREFIX.compare(content, 0, GIT_LFS_PREFIX.length) === 0
}

/**
* Register an asset to be emitted as part of the bundle (if necessary)
* and returns the resolved public URL
Expand Down Expand Up @@ -426,8 +435,15 @@ async function fileToBuiltUrl(
config.build.lib ||
(!file.endsWith('.svg') &&
!file.endsWith('.html') &&
content.length < Number(config.build.assetsInlineLimit))
content.length < Number(config.build.assetsInlineLimit) &&
!isGitLfsPlaceholder(content))
) {
if (config.build.lib && isGitLfsPlaceholder(content)) {
config.logger.warn(
colors.yellow(`Inlined file ${id} was not downloaded via Git LFS`)
)
}

const mimeType = mrmime.lookup(file) ?? 'application/octet-stream'
// base64 inlined as a string
url = `data:${mimeType};base64,${content.toString('base64')}`
Expand Down