-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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(less): always resolve to absolute path on fs when rebasing urls #17857
base: main
Are you sure you want to change the base?
Conversation
Run & review this pull request in StackBlitz Codeflow. |
Still needs a test case |
const resolved = await resolvers.less(filename, path.join(dir, '*')) | ||
const resolved = await resolvers.less(filename, rootFile) |
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 don't think this is correct? The filename
should always be resolved from the file that imports it, not the rootFile. The problem I'm seeing is that Less is treating @import url()
weirdly.
With @import url('../../less/ommer.less');
in the test, the filename
becomes less/ommer.less
. But with @import '../../less/ommer.less';
, filename
becomes ../../less/ommer.less
.
I think we need to figure out why this happens first.
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.
Actually it doesn't look like the test fails in main. When Vite fails to resolve it, it'll fallback to Less to load it itself (super.loadFile
), and it'll work again.
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 difference between @import url('../../less/ommer.less');
and @import '../../less/ommer.less';
is the first one gets everything in url() rebased, the second one does not get rebased.
Also what do you mean
it doesn't look like the test fails in main.
Do you want to make vite resolve it instead of falling back to less?
The test case without the fix indeed fails in main when called with pnpm run test-build
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 test case without the fix indeed fails in main when called with
pnpm run test-build
My bad looks like it does. I was running the playground manually and seeing that the text was darkorange even without this PR's changes.
Do you want to make vite resolve it instead of falling back to less?
Ideally Vite should always be able to resolve it yes.
However after the import in line 2535 we rebased the url to the rootfile. Do you mean we should rebase the file to the current file instead of rootfile? Then next time the file name will be relative to the file that imports it instead of relative to the root file.
I think I see the issue now. I didn't expect rebaseUrls
to be the culprit, but that function should really not rebase any urls related to @import "";
and @import url("")
(seems to start from #7147). The goal of rebaseUrls
is to make sure the url()
or data-uri()
would still reference the correct file path as that reference will be left as is after compiling to a single file.
@import
is different because they don't get left in that single file, their contents are bundled instead, unless it's importing an external URL, data URL, or something, but in that case you wouldn't need to rebase it either.
Maybe I'm missing something, but if what I'm getting is correct. The fix is to remove this:
vite/packages/vite/src/node/plugins/css.ts
Lines 2501 to 2504 in 6700594
// fix css imports in less such as `@import "foo.css"` | |
if (hasImportCss) { | |
rebased = await rewriteImportCss(content, rebaseFn) | |
} |
and update this to not capture @import url(
:
vite/packages/vite/src/node/plugins/css.ts
Lines 1632 to 1633 in 6700594
export const cssUrlRE = | |
/(?<=^|[^\w\-\u0080-\uffff])url\((\s*('[^']+'|"[^"]+")\s*|[^'")]+)\)/ |
I'm not sure if this would break other tests, feel free to let me know though if the issue/fix is getting overwhelming and I can help make a fix too.
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 have tested the different fix you suggested but it seems breaks the original test case from #7147
However after the import in line 2535 we rebased the url to the rootfile. Do you mean we should rebase the file to the current file instead of rootfile? Then next time the file name will be relative to the file that imports it instead of relative to the root file.
Get Outlook for iOS<https://aka.ms/o0ukef>
________________________________
From: Bjorn Lu ***@***.***>
Sent: Wednesday, August 14, 2024 2:03:01 AM
To: vitejs/vite ***@***.***>
Cc: Meng, Shaoyu ***@***.***>; Author ***@***.***>
Subject: Re: [vitejs/vite] fix(less): always use root file as base when resolving id (PR #17857)
@bluwy commented on this pull request.
________________________________
In packages/vite/src/node/plugins/css.ts<https://urldefense.com/v3/__https://github.com/vitejs/vite/pull/17857*discussion_r1716565269__;Iw!!DZ3fjg!9EoFQrEdBL2JKSVtsCDryoIMd7koBPIHEaMpf1Vxm_arg5NTwH7xq3nmpqne5xPcpl3orVCRtmzZr4uiZAz5KfwW6kI$>:
- const resolved = await resolvers.less(filename, path.join(dir, '*'))
+ const resolved = await resolvers.less(filename, rootFile)
I don't think this is correct? The filename should always be resolved from the file that imports it, not the rootFile. The problem I'm seeing is that Less is treating @import url() weirdly.
With @import url('../../less/ommer.less'); in the test, the filename becomes less/ommer.less. But with @import '../../less/ommer.less';, filename becomes ../../less/ommer.less.
I think we need to figure out why this happens first.
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/vitejs/vite/pull/17857*pullrequestreview-2237574274__;Iw!!DZ3fjg!9EoFQrEdBL2JKSVtsCDryoIMd7koBPIHEaMpf1Vxm_arg5NTwH7xq3nmpqne5xPcpl3orVCRtmzZr4uiZAz5rgd0t40$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AJHAEC26KZCM7YNDRD3IPRLZRMMMLAVCNFSM6AAAAABMJQP3GOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMZXGU3TIMRXGQ__;!!DZ3fjg!9EoFQrEdBL2JKSVtsCDryoIMd7koBPIHEaMpf1Vxm_arg5NTwH7xq3nmpqne5xPcpl3orVCRtmzZr4uiZAz5vuOFoeU$>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Rebased to fix conflicts with v6 environment api. the @import is also used in scss, removing that line will cause scss to fail.
|
Hi @bluwy I have used a different method from the one you suggested to solve the problem. The relative path is not very robust, we can just choose to use the absolute path as the id for a file. If we use absolute path, less is able resolve the file instead of falling back to vite to resolve the file |
Description
Closes #13143
Since we called the rebaseUrls functions to rebase everything to the root file,Next time when we use resolvers.less we must also pass the root file instead of the current dir.
Used a different method to fix the issue. See below.