feat(css): provide filename for css-minify error

[chaejunlee]

Description

• fixes: https://github.com/vitejs/vite/issues/15915

Previously, css code were concatenated into one string and then passed to finalizeCss() which made it hard to track where the error came from. So, I made an intermediary map to store the filename (id) with the code and passed that filename to finalizeCss() to give the user context when it errors.

Additional context

Instead of passing one giant css string to finalizeCss(), I tried to break it down and pass each filename to make the error more elegant. It is passing the test but because I am finalizing the css code by file and then concatenating, the result of hoisting might not be as same as before.

I don't think it will be a big problem, but I'll try to add a e2e test to check if this change will break anything.

---

What is the purpose of this pull request?

• [ ] Bug fix
• [x] New Feature
• [ ] Documentation update
• [ ] Other

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

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

[stackblitz[bot]]

Run & review this pull request in StackBlitz Codeflow.

[sapphi-red]

By concatenating CSS files before minifying, esbuild can apply cross-file optimizations. For example, duplicates can be removed (esbuild try). Since we are simply concatenating the files, I guess we can map the position back by keeping the number of lines of each files.

[chaejunlee]

I changed logic back to the concatenating raw css and kept track of filename with ending line number. I was able to create the correct warning message.

However, when I get the raw css code from cssPostPlugin().styles, the css file is already transformed, such as @imports ... are already inlined, so that the line number that warning logs is a little far from the original file. I don't think it is a critical flaw of this feature, but it would be great to defer the transform until the renderChunk() is called.

For that case, A) Do you think it is necessary to defer the transform of the raw css code for better warning? B) If it is, can you help a little bit with the general design? I am kind of lost with all the other logics that css handles and I don't want to just change the flow and cause more error. On top of that, I think it would be better to handle that in other PR.

A picture to better explain the case.

[sapphi-red]

We can map the position back to the original position by using source maps. But I think it's fine to leave it for now since we don't support CSS source maps in build.

[sapphi-red]

I tested by adding

{
  color: blue;
}

to playground/css/mod.module.css and running vite build. I guess the line calculation is wrong.

[chaejunlee]

@sapphi-red I think in this case, when renderChunk is being called, the code that is passed are already parsed and minimized, such as .scss, .less, .styl. In other words, code is different from the original content of the file. From this point, where the css is parsed and minimized, making the warning more accurate is beyond my ability 😥. Can you please help me with this?

We could use the already parsed and minized css to point to accurate point, but it that case, that css won't point to the correct source file.

I don't really know how to handle the css that is parsed.

[chaejunlee]

If I just print the css that is being passed to minifyCss, it looks like this.

It is totally different from the original content.

It looks like chunkCSS = resolveAssetUrlsInCss(chunkCSS, cssAssetName) is being called, and then the chunkCSS is then passed to finalizeCss. Though I am not sure if it is meaningful or not.

[sapphi-red]

From this point, where the css is parsed and minimized, making the warning more accurate is beyond my ability 😥.

That part is fine.

If I append the following content to playground/css/mod.module.css,

{
  color: blue;
}

I get Unexpected "{" [css-syntax-error] vite/playground/css/stylus.styl:29:0:. But I expect to the filename to be vite/playground/css/mod.module.css. (I understand the position cannot be mapped precisely.) The original position of the warning is 149:0. If I print concatCssEndLines, I get the following content.

[
  ...,
  {
    file: 'vite/playground/css/stylus.styl',        
    end: 151
  },
  {
    file: 'vite/playground/css/mod.module.css',     
    end: 159
  },
  ...
]

The file exists in concatCssEndLines so I guess it's possbile to map the warning to mod.module.css and the line calculation is wrong somewhere.

[chaejunlee]

Yes, you are right. I found out that I should've just went with 1-based number count and shouldn't have concat the css with \n. That was the cause of the problem. Sorry for arguing that it is impossible 😅. I corrected the collect logic as I should've done.

https://stackblitz.com/edit/node-redbgs?file=index.js

[sapphi-red]

No problem 😃

Don't we always need to append a line break at the end? Otherwise, we'll have more than one file mapped to a line and then we cannot map the line back to the original file.

For example, if the concatCssEndLines contains { file: 'foo2', end: 5 }, { file: 'foo1', end: 5 }, and we want to map line 5, we cannot know if it is foo1 or foo2.

[chaejunlee]

Yeah.. Just concating is vague to map back.. I changed all the concating logic back to concating with \n.