-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
fix(build): decode urls in CSS files (fix #15109) #15246
Conversation
Run & review this pull request in StackBlitz Codeflow. |
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.
Thank you!
Would you add a test in playground/assets
? Around
vite/playground/assets/css/css-url.css
Lines 8 to 11 in 0f9e1bf
.css-url-relative { | |
background: url(../nested/asset.png); | |
background-size: 10px; | |
} |
vite/playground/assets/__tests__/assets.spec.ts
Lines 152 to 154 in 0f9e1bf
test('relative', async () => { | |
expect(await getBg('.css-url-relative')).toMatch(assetMatch) | |
}) |
Absolutely! Could you help me out with the const encodedAssetMatch = isBuild
? /\/foo\/bar\/assets\/asset\[copy\]-[-\w]{8}\.png/
: '/foo/bar/nested/asset[copy].png' |
Of course! I guess |
Struggling to get that failing test to pass (in build mode) - for some reason, even though I've set the background URL to the image with special characters in its name, it still seems to be getting the normal test('encoded', async () => {
const bg = await getBg('.css-url-encoded');
console.log(bg);
expect(await getBg('.css-url-encoded')).toMatch(encodedAssetMatch)
}) .css-url-encoded {
background: url(/nested/asset%5Bcopy%5D.png);
background-size: 10px;
} <div class="css-url-encoded">
<span style="background: #fff">CSS background (encoded)</span>
</div> The output of the Gotta log off for the day, but I'll keep going tomorrow morning and hopefully figure it out |
Ah, I guess that's because Vite dedupes assets with same content. Changing the image slightly will fix it. |
Damn, I never knew that! Just made the file smaller (600px --> 500px) and it looks like it's passing locally now. Changes pushed 👍 |
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.
Thanks for the PR @ndv99!
@sapphi-red I'm good merging this one now, but I think it opens the door for a regression. If the url is name%2525
, when we decode it, it becomes name%25
(imagine this is a real file). If we use this url as the replacement, the browser will send it back and we will decode it again, becoming name%
and then the file will not be resolved.
I think it is ok because we have this issue in general IIUC, we should be encoding all the URLs before writing them to JS, HTML and CSS files and we aren't doing that right now.
Yeah, I think it's ok about that. But maybe it's safer to return vite/packages/vite/src/node/plugins/html.ts Lines 491 to 497 in 8ad81b4
|
Just tested this locally, and the build still works in my test project, resolving the encoded URL for the font file. Edit: Might also be a good idea to use |
Not really sure what's causing the failure on the Windows pipeline, might work itself out if a re-run is triggered. |
Description
decodeURI
inurlReplacer
in the CSS plugin to decode CSS urlsAdditional context
Previously,
url()
calls in CSS were not decoded at build time. Web browsers do decode this call, so anyurl()
call with an encoded string as a parameter would not resolve at build time, resulting in the following error:`\n${url} referenced in ${id} didn't resolve at build time, it will remain unchanged to be resolved at runtime`
This is important for some font files, which contain special characters in their names for variable weight/width. This PR fixes this by calling
decodeURI
at the top ofurlReplacer
, converting characters like%5B
and%5D
into their actual characters[
and]
respectively.More information on this bug can be found here: #15109
QA steps
packages/vite
, and runpnpm run dev
CONTRIBUTING.MD
to link this branch to the new Vite projectfonts
and place a font file inside, ensuring the name contains[
and]
index.css
, define the font face, but replace[
and]
with%5B
and%5D
respectively in thesrc
property. Example:pnpm run build
, and ensure the font file is resolved.Note: The font file will resolve with underscores replacing special characters in the build output directory - this behaviour can be mitigated by setting
build.rollupOptions.output.sanitizeName
tofalse
.What is the purpose of this pull request?
Fixes
Fixes #15109
Before submitting the PR, please make sure you do the following
fixes #123
).Update the corresponding documentation if needed.