fix(css): should not wrap with double quote when the url rebase feature bailed out #20068
Conversation
sapphi-red
added
feat: css
p3-minor-bug
| if (rawUrl === newUrl) { | ||
| return matched | ||
| } |
There was a problem hiding this comment.
Maybe we could add a comment here that says that the replacer will bail out when checking the first variable in the case of a concat and this is why it works? I think it would be better to check this in skipUrlReplacer but I understand it is a big refactoring to pass the variablePrefix to it. So good to go for me if we add the comment to avoid confusion later on.
There was a problem hiding this comment.
That would be better. Added a comment for now 👍
There was a problem hiding this comment.
Now that I see the comment, what happens when rawUrl has a space and rawUrl === newUrl? Don't we still need to re-wrap it in that case?
There was a problem hiding this comment.
If the rawUrl contains a space and rawUrl is not quoted, the input is not valid. The value in url() should be quoted to contain a space. So this code will keep the invalid code as-is when rawUrl === newUrl.
There was a problem hiding this comment.
rawUrl can originally have a space and be quoted, and we are removing the quotes here in that case https://github.com/vitejs/vite/pull/20068/files#diff-2cfbd4f4d8c32727cd8e1a561cffbde0b384a3ce0789340440e144f9d64c10f6R2053
Or am I reading this wrong? I think it is probably good to have a local unquotedUrl instead of overriding rawUrl, as this is indeed confusing.
There was a problem hiding this comment.
Ah, I overlooked that 🤦
There was a problem hiding this comment.
Let me come up with a more proper way to fix this.
There was a problem hiding this comment.
I've changed the impl to allow returning false from CssUrlReplacer. The previous impl didn't handle interpolation properly and that is not possible to have it inside skipUrlReplacer because the behavior is different for each preprocessors.
There was a problem hiding this comment.
I see now that the comment confused me as it was
// If the replacer output is same with the input, we don't need to wrap it with quotes.
but the code was returning matched, so it was working before but the new code is way better and you fixed other issues so this was good in the end :)
| if (skipUrlReplacer(unquotedUrl)) { | ||
| return matched | ||
| } | ||
| if (isExternalUrl(rawUrl) || isDataUrl(rawUrl) || rawUrl[0] === '#') { |
There was a problem hiding this comment.
This now checks functionCallRE.test(unquotedUrl) additionally.
The following code works and the URL replacement should be skipped.
@function foo() {
@return 'foo.css';
}
@import url(foo());| const skipRebaseUrls = (unquotedUrl: string, rawUrl: string) => { | ||
| const isQuoted = rawUrl[0] === '"' || rawUrl[0] === "'" | ||
| // matches `url($foo)` | ||
| if (!isQuoted && unquotedUrl[0] === '$') { | ||
| return true | ||
| } | ||
| // matches `url(#{foo})` and `url('#{foo}')` | ||
| return unquotedUrl.startsWith('#{') | ||
| } |
There was a problem hiding this comment.
Description
rebaseUrlwas replacingurl($var-c1 + $var-c2)withurl("$var-c1 + $var-c2").fixes #19196