-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: ensure CSS URLs are correct when inlining client stylesheets #15153
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: ensure CSS URLs are correct when inlining client stylesheets #15153
Conversation
🦋 Changeset detectedLatest commit: 3b89c6b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
| "test:dev": "DEV=true playwright test && DEV=true PATHS_ASSETS=https://cdn.example.com/stuff playwright test", | ||
| "test:build": "playwright test && PATHS_ASSETS=https://cdn.example.com/stuff playwright test", |
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 seemed like the easiest way to test both CSS inlining with and without paths.assets without introducing another test matrix.
paths.assets replaces the CSS asset urls at build time but paths.relative does it at runtime
packages/kit/src/utils/css.js
Outdated
| * @returns {string} | ||
| */ | ||
| export function replace_css_relative_url(contents, base) { | ||
| return contents.replaceAll(/url\((['"]?)\.\//ig, `url($1${base}/`); |
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.
Fortunately, we don't have to worry about the @import 'app.css' case that omits the url(...) syntax because Vite inlines the referenced stylesheet even when referenced by more than one other stylesheet https://vite.dev/guide/features#import-inlining-and-rebasing
elliott-with-the-longest-name-on-github
left a comment
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.
I pushed some whitespace-handling changes just to guard against edge cases (I don't think we actually have to worry about them because I don't think Vite will output CSS like that... but better to be safe).
The only other edge cases I can think of:
- Is it a problem that this doesn't work on relative paths without
./in front of them? i.e.url(img.png)won't get transformed. - This will replace content inside strings. So if you had the CSS
content: "Example: If you were to puturl(./rickroll.gif)here...", it would get modified, in spite of being a string. I don't see a way around this other than actually parsing the CSS contents with a CSS parser...
I think after Vite processes the CSS the urls are always prefixed with ./ but I can check how it’s handled from both the static folder or otherwise to see if they do end up this way.
Yeah, I think I should try out a CSS parser. Otherwise, maybe we’d have to do a negative lookahead. Are there any CSS parsers that you’d recommend? EDIT: I see we were previously using csstree in Svelte so we can probably try that? |
So I did some testing and here are the results:
|
format
09886d8 to
df13a14
Compare
|
|
Ok, I think it's ready. Some additional edge cases accounted for are URLs with escaped quotes, fragments, query strings, etc.. Only known assets are processed too so it doesn't just match anything with |
elliott-with-the-longest-name-on-github
left a comment
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 is looking awesome, tysm
packages/kit/src/utils/css.js
Outdated
| const url_value_match = /url\(\s*(['"]?)(.*?)\1\s*\)/i.exec(url_declaration); | ||
| if (!url_value_match) continue; | ||
|
|
||
| const [, , url] = url_value_match; |
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.
I think you could use non-capturing groups ((?:)) for the stuff you don't actually want to capture to avoid these weird commas
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 quotation's using a capture group so that it matches against the same type of closing quotation mark. This avoids matching a double quote in a single quoted string.
Unfortunately, this doesn't account for backslashed quotes in the same string
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.
I think I've fixed the escaped characters edge case but the CSS parser is duplicating the escaped characters in the declaration value it returns. I think we need to avoid adding the same characters twice here https://github.com/sveltejs/svelte/blob/9662010a135937b7a920eaf93933be7b8b0c9c9e/packages/svelte/src/compiler/phases/1-parse/read/style.js#L508-L525 Currently, a string such as \s becomes \\ss in the returned AST
EDIT: opened a PR for it here sveltejs/svelte#17554
EDIT2: We just need to wait for sveltejs/svelte#17555 to be merged and update the Svelte version in Kit to get the tests passing
packages/kit/src/utils/css.js
Outdated
| * @param {string} value | ||
| * @returns {string} | ||
| */ | ||
| function tippex_comments_and_strings(value) { |
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.
I did have to google what tippex meant :laugh:
elliott-with-the-longest-name-on-github
left a comment
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.
I fixed several tippex function edge cases and pushed a bunch of test cases to make sure it never regresses -- thank you so much for your work on this gnarly bug. Great job!
52965cf
into
main
fixes #13878
closes #15147
This PR replaces the janky server-client css mapping (introduced by me) with simply replacing the asset URL references with the correct path when we inline the styles. Not only does this massively simplify things, it allows us to correctly prefix the paths based on the settings from
paths.relativeorpath.assets.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Edits