fix(css): ensure order of extracted CSS

[susnux]

Description

Without this the order of the extracted CSS rules is defined by the order renderChunk of the css plugin is called. So with this the order of CSS rules is always in alphabetical order of the output chunks.

Previously on my project every time I ran the build the output style.css was different by rule order, which caused unnecessary conflicts.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[bluwy]

Seems like the best fix for now. If the bundle order is non-deterministic, it would be a bug in Rollup, otherwise it should work consistently now.

For other reviewers, from what I understand chunkCSSMap (and the renderChunk hook that sets it) is non-deterministic because Rollup calls renderChunk for all chunks in parallel. If some hook before the CSS plugin does async work with various timing, the calls would be affected as well.

[patak-dev]

Isn't this change unrelated to ordering? Wouldn't this mean that we'll be adding CSS of lazy loaded chunks into the parent chunks? 🤔

[susnux]

No this is only about cssCodeSplit: false. For splitted css the CSS assets are emitted in the renderChunk hook.
So this needs to include all styles.

[bluwy]

Hmm even for cssCodeSplit: false, I think it may make sense to remove this too. When iterating the bundle chunks, we don't want to eagerly crawl the dynamic imports and concatenate its CSS. Dynamic chunks styles should be ordered last so it has a higher priority.

Removing this should also be safe because we iterate chunks with isDynamicEntry: true, which will cover all these dynamic imported chunks.

[susnux]

Tested it right now, removing makes it non deterministic again, because the order of dynamic entries / dynamic imports in the bundle is not always the same.

[susnux]

Dynamic chunks styles should be ordered last so it has a higher priority.

So a working (tests) alternative proposal:
Do not include .isDynamicEntry in the main loop, but move the chunk.dynamicImports.forEach to the end.

Because the import order is deterministic, but the output order of dynamic entry chunks is not deterministic in the bundle.

Meaning it will look like this:

        function collect(chunk: OutputChunk | OutputAsset) {
          if (!chunk || chunk.type !== 'chunk' || collected.has(chunk)) return
          collected.add(chunk)

          // First collect all styles from the synchronous imports (lowest priority)
          chunk.imports.forEach((importName) => collect(bundle[importName]))
          // Then collect the styles of the current chunk (might overwrite some styles from previous imports)
          css += chunkCSSMap.get(chunk.preliminaryFileName) ?? ''
          // Finally collect the styles of dynamic imports (they have the highest priority and might overwrite other styles)
          chunk.dynamicImports.forEach((importName) =>
            collect(bundle[importName]),
          )
        }

        // The bundle is guaranteed to be deterministic, if not then we have a bug in rollup.
        // So we use it to ensure a deterministic order of styles
        for (const chunk of Object.values(bundle)) {
          if (chunk.type === 'chunk' && chunk.isEntry) {
            collect(chunk)
          }
        }

I tested this and the styles are always deterministic and the dynamically imported chunks will overwrite previous styles.

[bluwy]

I'm not sure if that's enough. If the bundle has multiple direct entries, wouldn't that mean on the iteration of the first direct entry, we would crawl its dynamicImports before iterating the second direct entry?

Maybe we should do two loops instead:

1. Loop through direct entries and collect the CSS, and record dynamic entries to a Set.
2. Loop through the Set (of dynamic entries), and collect the CSS.

That should still keep the dynamic entries deterministic?

[susnux]

@bluwy you are right, tested this and it works (see commit I pushed).

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI on 9f584f1: Open

suite	result	latest scheduled
astro	undefined skipped	✅ success
sveltekit	❌ failure	❌ failure
vike	❌ failure	❌ failure
vitest	❌ failure	❌ failure

✅ analogjs, histoire, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, remix, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress

[susnux]

Rebased to resolve the conflict.

[bluwy]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI on f34bc55: Open

suite	result	latest scheduled
nuxt	❌ failure	❌ failure
vite-plugin-react-pages	❌ failure	❌ failure
vitest	❌ failure	❌ failure

✅ analogjs, astro, histoire, ladle, laravel, marko, previewjs, quasar, qwik, rakkas, remix, sveltekit, unocss, vike, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress