fix: Skip inlining Git LFS placeholders (fix #9714) #9795
Conversation
|
@bluwy Please have a look at whether this implementation looks like what you had in mind. I'm happy to make adjustments. I have manually tested the change locally. |
MajorBreakfast
force-pushed
the
skip-inlining-lfs-placeholders
branch
from
f77d9f7
to
4ad397d
Compare
|
At first glance it looks to me as if the CI is flaky. Can somebody with more insight into this say whether that's indeed the case or whether I need to adapt the code? |
|
It's failing three times in a row. So I think it's not a flaky fail. |
|
@sapphi-red excellent. I'll try that and report back |
MajorBreakfast
force-pushed
the
skip-inlining-lfs-placeholders
branch
from
4ad397d
to
92b1068
Compare
|
The advice with running the assets playground locally helped. 👍 I found the bug |
MajorBreakfast
force-pushed
the
skip-inlining-lfs-placeholders
branch
from
92b1068
to
7f880dd
Compare
|
Just pushed another minor tweak to the docs: "To get inlining, make sure to download the file contents via Git LFS before building." - Just to make clearer what's happening |
| ::: 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. |
There was a problem hiding this comment.
Not for this PR, but we should find a way to avoid this in lib mode too.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Alright. I'll see if I can make it produce a warning
There was a problem hiding this comment.
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
Description
Git LFS placeholder files do not contain the actual file content and can therefore not be meaningfully inlined. This PR makes it so that they are automatically excluded from inlining. (Fixes #9714)
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).