fix(less): always use root file as base when resolving id

[smeng9]

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.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[smeng9]

Still needs a test case

[bluwy]

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.

[bluwy]

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.

[smeng9]

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

[bluwy]

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.

[smeng9]

I have tested the different fix you suggested but it seems breaks the original test case from #7147

[smeng9]

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: ***@***.***>