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: import css in less/scss (fix 3293) #7147
Conversation
303afc7
to
f45b01a
Compare
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 find that sass also uses the rebaseUrls
function.
To avoid accidentally breaking sass functionality, could you add a test case for .scss
files too?
And could you test whether Because I'm thinking that, if Of course, if |
Ok, I will add test case as you expect. If we remove vite/packages/vite/src/node/plugins/css.ts Lines 1110 to 1145 in a4927c5
|
Yeah but the So I guess it won't break. |
590d915
to
86cda8b
Compare
It also fails to import CSS in SCSS file with a relative path. reproduction repo: https://github.com/sinoon/reproduction-import-scss-in-scss And I add a test case for SCSS, the test is passed. Because they are the same problem, fixed LESS solutions also work for SCSS files. It seems SCSS is not provider option like In order to be compatible with both LESS and SCSS for proper handling in this case, my current implementation is satisfactory. |
I tested this way and the test case for alias would fail. Action: https://github.com/sinoon/vite/actions/runs/1937341205 |
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 code looks good to me. Just a small typo.
Co-authored-by: Haoqun Jiang <haoqunjiang@gmail.com>
Description
fix #3293
For some situations, we will import CSS file in less/scss files.This does not happen in all cases.
(tips: scss file has the same situation, so I only show less example in after)
Let's say we have three files.
top.less
import './nested/nested.less';
/nested/nested.less
/nested/bad.css
and if we don't set any vite config.
then
vite
it and we will see something like this:reproduction repo:
react: https://github.com/sinoon/reproduction-for-vite-3293-react
vue: https://github.com/sinoon/reproduction-for-vite-3293-vue
vue with scss: https://github.com/sinoon/reproduction-import-scss-in-scss
(create by pnpm create vite)
How it happen?
less transform less file start from
top.less
, and after all done, it will becometop.css
in one file.so,@import './bad.css'
should convert to@import './nested/bad.css'
.Vite is design support
url()
in less(also in sass) for relative path and alias. For this purpose, Vite use less.js with self created plugin to complete this task.In short this plugin does the following things:
rebaseUrls
to overwrite file forurl()
how
rebaseUrls
function works? find allurl()
case and rewrite it to the right path.It has worked well up to now.
BUT in case
import css in less
it not in the scope of duties. Because not useurl()
withbad.css
.Often, people will go for the Less option:
relativeUrls
, it will cover thisedge case
(not actually), and it will treating the situation correctly in@import './bad.css'
to@import './nested/bad.css
.And then unfortunately, it also cover
url()
case, andrebaseUrls
functions do the same job.@import url('./bad.css')
will be@import url('./nested/nested/bad.css')
not@import url('./nested/bad.css')
.So, we need the same support for css in the regular case, not only
@import url(xx)
but also@import 'xx.css
.Because of we already supporturl(xx)
, support the similar behavior of./xxx
is reasonable.Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).