-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
feat: support images outside the project #7139
Conversation
🦋 Changeset detectedLatest commit: 994507a The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The /@fs
handling looks good to me 👍
meta.src = rootRelativePath(settings.config, url); | ||
meta.src = `/@fs` + prependForwardSlash(fileURLToNormalizedPath(url)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A note on this: Vite alternates between a root relative path and /@fs
depending on whether the path is inside the root or not. Here it's always using /@fs
which is and should fine, but there are small bugs now and then because Vite's rule is not applied by frameworks. Something to keep an eye on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm willing to take the risk. I think the consistency of it is great, even though I don't necessarily want users to rely on it. Will definitely reconsider if it becomes an issue!
const filePath = url.searchParams.get('href')?.slice('/@fs'.length); | ||
const filePathURL = new URL('.' + filePath, 'file:'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually kinda neat and it seems to handle Windows too. Vite handles it in a slightly complex way.
Changes
Changes the returned
src
for imported images to be absolute path with a Vite/@fs/
thing so we can support imports from outside the project. This also allows us to add an error message when someone make the common mistake of passing an imported image'ssrc
toImage
andgetImage
instead of passing the full importThis PR also includes some minor fixes to error messages that I noticed while adding a new error.
Fix #6635
Testing
Added tests, modified tests that tested the URL precisely, and tests should still pass!
Docs
All the usages of
src
that are documented should still work, so no change necessary.