fix(css): css file emit synchronously

[sun0day]

Description

This pr contains 2 parts:

1. rollup3.20.0 broke the rule first assets determines the name of getFileName, and 3.20.1 fix that, see Only set asset names when finalizing rollup/rollup#4919, this pr will upgrade rollup to 3.20.1

2. rollup3.20.1 will lead Builds are non-deterministic #11911 (which was fixed in chore: upgrade rollup 3.20.0 #12497) to happen again, vite has to handle CSS file(with the same content) emission order by itself. This pr will force CSS files to emit synchronously and make the final asset name deterministic.

Additional context

Maybe lead to some perf lose after this pr.

rollup 3.20.0 -> 3.20.1 changelog

---

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 PR Title 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.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[sun0day]

The code will be more neat if we make finalizeCss synchronous

[sun0day]

@sapphi-red cc

[sapphi-red]

I found meta.chunks existing in renderChunk hook.
I wonder if we can emit all CSS files in a single renderChunk call and ignore the calls later on.

const called = false
const plugin = {
  renderChunk(code, chunk, opts, meta) {
    if (!called) {
      called = true
      for (const chunkName in meta.chunks) {
        // emit css file
      }
    }
  },
  buildStart() {
    called = false
  }
}

[sun0day]

I found meta.chunks existing in renderChunk hook. I wonder if we can emit all CSS files in a single renderChunk call and ignore the calls later on.

const called = false
const plugin = {
  renderChunk(code, chunk, opts, meta) {
    if (!called) {
      called = true
      for (const chunkName in meta.chunks) {
        // emit css file
      }
    }
  },
  buildStart() {
    called = false
  }
}

Good catch. But I think if we handle all the chunks via meta.chunks in one renderChunk, I am afraid the CSS plugin are losing the ability to reuse some common logic, it breaks the renderChunk common flow. What do you think @bluwy

[bluwy]

Since finalizeCss is pure, we should be able to track the first emitted CSS file and use that for subsequent same-content CSS files? Something like this:

chunkCSS = resolveAssetUrlsInCss(chunkCSS, cssAssetName)

let emitName = cssToNameMap.get(chunkCSS)
if (!emitName) {
  emitName = path.basename(cssFileName)
  cssToNameMap.set(chunkCSS, emitName)
}

chunkCSS = await finalizeCss(chunkCSS, true, config)

const referenceId = this.emitFile({
  name: emitName,
// ...

I feel like this would be simpler and has lesser toll on keeping tasks in memory. We could also hash the chunkCSS to shorten the string so that it occupies lesser memory. (though maybe good to measure first)

[sun0day]

Since finalizeCss is pure, we should be able to track the first emitted CSS file and use that for subsequent same-content CSS files? Something like this:

Yeah, finalizeCss is pure, but the input chunkCSS maybe different even though their finalizeCss outputs are the same. This means that the cssToNameMap cache may be invalid

[bluwy]

I'd probably accept that caveat for now since it's rare, but the longer I stare at this, maybe this PR can work too. I have some ideas to simplify this and might push directly here if you're fine with it.

[sun0day]

I'd probably accept that caveat for now since it's rare, but the longer I stare at this, maybe this PR can work too. I have some ideas to simplify this and might push directly here if you're fine with it.

I'm fine. Go ahead.

[bluwy]

I've refactored it as a promise-based queue so that each finalizeCss can be called in parallel, but the emitFile will be called sequentially based on the order of renderChunk calls instead of the file name (renderChunk order should be deterministic?). I can't reproduce the non-deterministic behaviour in the issue repro so but I've ran this and it seems to work.

[sun0day]

There is already a test case for this issue.

I thought about using a promise queue before. But I am not sure whether rollup will guarantee the async renderChunk order is deterministic or not . Besides, would it be better that keep the emit file name determination the same with rollup's rule ?

[bluwy]

There is already a test case for this issue.

Ah nice!

But I am not sure whether rollup will guarantee the async renderChunk order is deterministic or not

Judging from this rollup code I hope it does, but if not, I think it should be fixed in Rollup instead.

Besides, would it be better that keep the emit file name determination the same with rollup's rule ?

IIUC the rule applies pre-renderStart, so for post-renderStart we'd let Rollup handle it the way it wants so in the future if it changes again, our CSS emit logic should follow too.

[sun0day]

Ok. Make sense to me. 👍

[bluwy]

Probably need another review before we merge this.

[patak-dev]

Maybe there is a way to avoid the promise chain here, by using a single semaphore promise and a counter. We could review the aproach if we see a perf issue