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

fix(build): decode urls in CSS files (fix #15109) #15246

Merged
merged 5 commits into from Dec 12, 2023

Conversation

ndv99
Copy link
Contributor

@ndv99 ndv99 commented Dec 5, 2023

Description

  • Call decodeURI in urlReplacer in the CSS plugin to decode CSS urls

Additional context

Previously, url() calls in CSS were not decoded at build time. Web browsers do decode this call, so any url() 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 of urlReplacer, converting characters like %5B and %5D into their actual characters [ and ] respectively.

More information on this bug can be found here: #15109

QA steps

  1. Clone this branch and go to packages/vite, and run pnpm run dev
  2. Create a new Vite project in a different directory (I tested with React + TypeScript)
  3. Follow the instructions in CONTRIBUTING.MD to link this branch to the new Vite project
  4. Create a subfolder fonts and place a font file inside, ensuring the name contains [ and ]
  5. In index.css, define the font face, but replace [ and ] with %5B and %5D respectively in the src property. Example:
@font-face {
  font-family: 'Ubuntu variable';
  font-stretch: 100%; /*  min and max value for the width axis, expressed as percentage */
  font-style: normal;
  font-weight: 100 800; /*  min and max value for the weight axis */
  src: url('/fonts/f1ea362b-Ubuntu%5Bwdth,wght%5D-latin-v0.896a.woff2')
    format('woff2-variations');
}
  1. Run 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 to false.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Fixes

Fixes #15109

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Dec 5, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@ndv99 ndv99 changed the title fix(build): decode urls in CSS files (fix #15109[,#15109]) fix(build): decode urls in CSS files (fix #15109) Dec 5, 2023
@sapphi-red sapphi-red added feat: css p3-minor-bug An edge case that only affects very specific usage (priority) labels Dec 5, 2023
Copy link
Member

@sapphi-red sapphi-red left a 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

.css-url-relative {
background: url(../nested/asset.png);
background-size: 10px;
}

test('relative', async () => {
expect(await getBg('.css-url-relative')).toMatch(assetMatch)
})

@ndv99
Copy link
Contributor Author

ndv99 commented Dec 5, 2023

Thank you!

Would you add a test in playground/assets?

Absolutely! Could you help me out with the assetMatch regex? I've created a file asset[copy].png to test this with (which is just a copy of asset.png), I'm guessing it should look something like this:

const encodedAssetMatch = isBuild
  ? /\/foo\/bar\/assets\/asset\[copy\]-[-\w]{8}\.png/
  : '/foo/bar/nested/asset[copy].png'

@sapphi-red
Copy link
Member

sapphi-red commented Dec 5, 2023

Of course! I guess /\/foo\/bar\/assets\/asset\[copy\]-[-\w]{8}\.png/ needs to be /\/foo\/bar\/assets\/asset_copy_-[-\w]{8}\.png/ because we are not setting build.rollupOptions.output.sanitizeName.

@ndv99
Copy link
Contributor Author

ndv99 commented Dec 5, 2023

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 asset/png.

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 console.log is url("http://localhost:4173/foo/bar/assets/asset-tYbLOGqm.png"), when I'd expect it to be url("http://localhost:4173/foo/bar/assets/asset_copy_-tYbLOGqm.png")

Gotta log off for the day, but I'll keep going tomorrow morning and hopefully figure it out

@sapphi-red
Copy link
Member

Ah, I guess that's because Vite dedupes assets with same content. Changing the image slightly will fix it.

@ndv99
Copy link
Contributor Author

ndv99 commented Dec 6, 2023

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 👍

@ndv99 ndv99 requested a review from sapphi-red December 6, 2023 09:59
sapphi-red
sapphi-red previously approved these changes Dec 6, 2023
patak-dev
patak-dev previously approved these changes Dec 6, 2023
Copy link
Member

@patak-dev patak-dev left a 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.

@sapphi-red
Copy link
Member

Yeah, I think it's ok about that.

But maybe it's safer to return url instead of decodedUrl here?
https://github.com/vitejs/vite/pull/15246/files#diff-2cfbd4f4d8c32727cd8e1a561cffbde0b384a3ce0789340440e144f9d64c10f6L295
We're doing like that in another place.

async ({ url }) => {
const decodedUrl = decodeURI(url)
if (!isExcludedUrl(decodedUrl)) {
const result = await processAssetUrl(url)
return result !== decodedUrl ? result : url
}
return url

@ndv99
Copy link
Contributor Author

ndv99 commented Dec 6, 2023

Yeah, I think it's ok about that.

But maybe it's safer to return url instead of decodedUrl here?

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 url instead of decodedUrl here for debug logging

@ndv99
Copy link
Contributor Author

ndv99 commented Dec 11, 2023

Not really sure what's causing the failure on the Windows pipeline, might work itself out if a re-run is triggered.

@patak-dev patak-dev merged commit ea6a7a6 into vitejs:main Dec 12, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Variable-width fonts not resolving during Vite build
3 participants