Skip to content
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

Merged
merged 17 commits into from Mar 7, 2022

Conversation

sinoon
Copy link
Contributor

@sinoon sinoon commented Mar 2, 2022

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
/nested/nested.less
/nested/bad.css

top.less

import './nested/nested.less';

/nested/nested.less

@import './bad.css'

/nested/bad.css

div {
  color: blue;
}

and if we don't set any vite config.

then vite it and we will see something like this:
image

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 become top.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:

  1. use resolvers.less to resolve file
  2. if resolved, call a functions named rebaseUrls to overwrite file for url()

how rebaseUrls function works? find all url() 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 use url() with bad.css.

Often, people will go for the Less option: relativeUrls, it will cover this edge 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, and rebaseUrls 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 support url(xx), support the similar behavior of ./xxx is reasonable.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link
Member

@sodatea sodatea left a 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 rebaseUrlsfunction.
To avoid accidentally breaking sass functionality, could you add a test case for .scss files too?

@sodatea
Copy link
Member

sodatea commented Mar 4, 2022

And could you test whether .scss files worked as expected before this PR?

Because I'm thinking that, if .scss already worked well, maybe we don't need this extra replacement.
We can just remove the rebaseUrls() from the less plugin (to avoid duplication) and use the rewriteUrls: 'local' option instead.

Of course, if .scss didn't work and is fixed by this PR, I think we can go ahead with this PR.

@sinoon
Copy link
Contributor Author

sinoon commented Mar 4, 2022

Ok, I will add test case as you expect.

If we remove rebaseUrls in less file, alias feature for less file will lose.Because rebaseUrls will impact @import './xxx.css' and all of url(xxx)case.You can find it in functionsrewriteCssUrls`

async function rebaseUrls(
file: string,
rootFile: string,
alias: Alias[]
): Promise<Sass.ImporterReturnType> {
file = path.resolve(file) // ensure os-specific flashes
// in the same dir, no need to rebase
const fileDir = path.dirname(file)
const rootDir = path.dirname(rootFile)
if (fileDir === rootDir) {
return { file }
}
// no url()
const content = fs.readFileSync(file, 'utf-8')
if (!cssUrlRE.test(content)) {
return { file }
}
const rebased = await rewriteCssUrls(content, (url) => {
if (url.startsWith('/')) return url
// match alias, no need to rewrite
for (const { find } of alias) {
const matches =
typeof find === 'string' ? url.startsWith(find) : find.test(url)
if (matches) {
return url
}
}
const absolute = path.resolve(fileDir, url)
const relative = path.relative(rootDir, absolute)
return normalizePath(relative)
})
return {
file,
contents: rebased
}
}

@sodatea
Copy link
Member

sodatea commented Mar 4, 2022

Yeah but the postcss-import package can deal with CSS import aliases. url() aliases for asset files are also processed in other plugins.
Aliases in .less paths are done in the loadFile hook later in this file.

So I guess it won't break.

@sinoon sinoon changed the title fix: import css in less (fix 3293) fix: import css in less/scss (fix 3293) Mar 4, 2022
@sinoon
Copy link
Contributor Author

sinoon commented Mar 4, 2022

It also fails to import CSS in SCSS file with a relative path. reproduction repo: https://github.com/sinoon/reproduction-import-scss-in-scss

image

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 relativeUrls, so we have to handle the relative path.

In order to be compatible with both LESS and SCSS for proper handling in this case, my current implementation is satisfactory.

@sinoon sinoon requested a review from sodatea March 5, 2022 04:13
@sinoon
Copy link
Contributor Author

sinoon commented Mar 5, 2022

And could you test whether .scss files worked as expected before this PR?

Because I'm thinking that, if .scss already worked well, maybe we don't need this extra replacement. We can just remove the rebaseUrls() from the less plugin (to avoid duplication) and use the rewriteUrls: 'local' option instead.

Of course, if .scss didn't work and is fixed by this PR, I think we can go ahead with this PR.

I tested this way and the test case for alias would fail.

Action: https://github.com/sinoon/vite/actions/runs/1937341205
diff: sinoon@daf6874

Copy link
Member

@sodatea sodatea left a 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.

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
Co-authored-by: Haoqun Jiang <haoqunjiang@gmail.com>
@sodatea sodatea added the p3-minor-bug An edge case that only affects very specific usage (priority) label Mar 7, 2022
@patak-dev patak-dev merged commit 9b51a3a into vitejs:main Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
5 participants