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

Skip asset inlining for Git LFS placeholders #9714

Closed
4 tasks done
MajorBreakfast opened this issue Aug 17, 2022 · 3 comments · Fixed by #9795
Closed
4 tasks done

Skip asset inlining for Git LFS placeholders #9714

MajorBreakfast opened this issue Aug 17, 2022 · 3 comments · Fixed by #9795
Labels

Comments

@MajorBreakfast
Copy link
Contributor

Description

Vite by default automatically inlines assets that are below a specified size limit (build.assetsInlineLimit, docs).

Unfortunately, if the build system does not download the LFS files for increased build performance (e.g. Netlify), this means that Git LFS placeholder files, which are small, also get inlined even though they do not contain the actual file content.

Example Git LFS placeholder file:

version https://git-lfs.github.com/spec/v1
oid sha256:5c908e755763256d23fdf72665459361bfa6e9e0de8fe9de1c122f3db6708317
size 176336

Suggested solution

Detect whether a file is an LFS placeholder (e.g. by comparing its first line) and skip inlining for detected LFS placeholders.

Alternative

No response

Additional context

No response

Validations

@bluwy
Copy link
Member

bluwy commented Aug 21, 2022

This make sense to me. The change needed would be at

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 {

I think as a start, we could simply check if it starts with version https://git-lfs.github.com/spec/v1 and bail. Tests for this could be tricky too so it should be fine without a test for now.

Also to confirm, if we do skip inlining it, would it properly served by default?

@github-actions
Copy link

Hello @MajorBreakfast. We like your proposal/feedback and would appreciate a contribution via a Pull Request by you or another community member. We thank you in advance for your contribution and are looking forward to reviewing it!

@MajorBreakfast
Copy link
Contributor Author

@bluwy @patak-dev @sapphi-red Thanks for all the support in getting this shipped! :shipit:

@github-actions github-actions bot locked and limited conversation to collaborators Sep 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants