fix(css): compile SCSS before LightningCSS minification

[james-elicx]

Summary

• When a CSS module uses composes: foo from './other.module.scss', postcss-modules' built-in FileSystemLoader reads the raw .scss text and feeds it straight into PostCSS, bypassing Vite's Sass preprocessor.
• The unprocessed source (with $var: red; declarations) then reaches Vite 8's lightningcss minifier — the default for server environments — which rejects it with SyntaxError: [lightningcss minify] Invalid empty selector and fails the build.
• This wires up a custom css.modules.Loader that runs Vite's preprocessCSS over .scss/.sass/.less/.styl files before postcss-modules sees them. The Loader reimplements the small FileSystemLoader + Parser logic from postcss-modules (which Vite bundles internally and we cannot import directly), but interposes a preprocessor step between the file read and the PostCSS pipeline.
• Mirrors how Next.js' webpack pipeline sequences sass-loader before css-modules. See the source comments and the ported test fixtures from test/e2e/app-dir/scss/composes-external and test/e2e/app-dir/scss/nm-module-nested.

Test plan

• pnpm test tests/scss-composes.test.ts — new regression tests for both the composes-external and nm-module-nested patterns (skipped if sass not installed locally; install with pnpm add -D sass -w to exercise them).
• pnpm test tests/scss.test.ts tests/sass-options.test.ts tests/nextjs-compat/app-css.test.ts tests/ssr-css-assets.test.ts — existing CSS suite still passes.
• vp check clean on the new files.
• CI green (Check, Vitest, Playwright E2E).

Closes #1343

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@1393

commit: 6791600

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original
web	preview	production

[james-elicx]

/bigbonk review

[ask-bonk]

Review: fix(css): compile SCSS before LightningCSS minification

Good fix for a real and well-diagnosed bug. The approach of providing a custom postcss-modules Loader is the correct architectural choice given that Vite bundles postcss-modules internally and doesn't expose FileSystemLoader for extension. The documentation quality in the module header is excellent — it explains the problem, the solution, and the relationship to Next.js clearly.

A few issues to address:

---

Bug: require.resolve in ESM context

packages/vinext/src/plugins/css-modules-preprocess.ts:161

return require.resolve(unquoted);

This file is part of an ESM package ("type": "module" in package.json). Bare require is not available in ESM modules. This will throw ReferenceError: require is not defined at runtime when a CSS module uses composes: foo from 'some-package/styles.module.css' (bare specifier resolution path).

This works in the test environment because Vite's build pipeline runs modules through its transform, but in production/built form this code ships as ESM. You need to use createRequire from node:module:

import { createRequire } from "node:module";
// ...
const require = createRequire(import.meta.url);
return require.resolve(unquoted);

The rest of index.ts already uses createRequire for exactly this pattern (see lines 136, 160-161, 334).

---

Bug: String.prototype.replace only replaces the first occurrence

packages/vinext/src/plugins/css-modules-preprocess.ts:271

value = value.replace(key, translations[key]!);

String.prototype.replace with a string pattern only replaces the first occurrence. If an export value contains multiple references to the same imported symbol, only the first will be substituted. This should use replaceAll:

value = value.replaceAll(key, translations[key]!);

This is a subtle but real divergence from the built-in Parser's handleExport behavior, which uses icss-utils' replaceValueSymbols (which replaces all occurrences via regex).

Additionally, String.prototype.replace interprets special patterns like $&, $1, etc. in the replacement string. Since the replacement comes from CSS class names (which can contain $ after Sass processing), this could produce incorrect output. Using a replacement function avoids this:

value = value.replaceAll(key, () => translations[key]!);

---

Nit: Promise.all on import processing is safe but worth a comment

packages/vinext/src/plugins/css-modules-preprocess.ts:226

The Promise.all for processing imports is correct here because each import resolves independently and writes to distinct translations[decl.prop] keys. The original FileSystemLoader processes imports sequentially, but the parallel approach is fine since the depNr trace key is pre-assigned. Just noting this is an intentional divergence from the original — might be worth a one-line comment.

---

Nit: User's css.modules options may be clobbered

packages/vinext/src/index.ts:1644-1658

The config hook returns css: { modules: { Loader: ... } }. Vite's config merging does a shallow merge of css.modules, so if a user also sets css.modules.scopeBehaviour or css.modules.generateScopedName in their vite.config.ts, the merge should work correctly since both sides contribute different keys. However, if a user already sets their own css.modules.Loader, vinext will silently override it. Consider checking for a user-supplied Loader and logging a warning, or skipping the override in that case.

---

Minor: Test robustness

The tests are well-structured and cover the two main Next.js test cases (composes-external, nm-module-nested). Two small observations:

1. The findBundledCss helper uses a depth-first search that returns the first .css file found. If Vite emits multiple CSS files (e.g., in future versions with different code-splitting), this could be fragile. Consider collecting all CSS files and asserting on the concatenated content, or asserting that at least one CSS file matches.

2. Good use of describeIfSass to skip when sass is unavailable. The test comment mentions this pattern matches tests/scss.test.ts, which is good for consistency.

---

Overall

The core approach is sound and well-researched. The require.resolve ESM issue is the most important fix needed — the replace/replaceAll issue is a correctness concern that may not trigger in typical use but should be fixed for robustness. Ship it after addressing those two.

[ask-bonk]

Review posted on PR #1393. Here's a summary of the findings:

Two bugs found:

1. require.resolve in ESM context (css-modules-preprocess.ts:161) — The file uses bare require.resolve() but ships as ESM. This will throw ReferenceError at runtime when resolving bare module specifiers (e.g., composes: foo from 'some-package/styles.module.css'). Needs createRequire from node:module.

2. String.prototype.replace only replaces first occurrence (css-modules-preprocess.ts:271) — The :export value substitution loop uses .replace() which only handles the first match per key. Should be .replaceAll() with a function replacement to also avoid $&/$1 special pattern interpretation in replacement strings.

Two nits:

• The Promise.all for import processing is an intentional divergence from the sequential original — worth a comment.
• User-supplied css.modules.Loader would be silently overridden — worth checking/warning.

Overall assessment: The core approach is correct and well-documented. The fix properly addresses the root cause (postcss-modules' FileSystemLoader bypassing Vite's Sass preprocessor). The two bugs should be fixed before merge, but neither blocks the happy path tested in the PR.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Re-review: all previous feedback addressed

Verified commit 6791600 against the four items from the prior review — all fixed correctly:

1. require.resolve in ESM — Fixed. createRequire(import.meta.url) at module scope (line 78), used as nodeRequire.resolve() at line 170.
2. replace → replaceAll — Fixed. Line 297 uses replaceAll with a function replacement to avoid $&/$1 interpretation. Well-documented in the comment block (lines 284-290).
3. Promise.all divergence — Documented at lines 236-243.
4. User-supplied Loader override — Detected and warned at lines 1302-1320 in index.ts, with the override skipped.
5. Test findBundledCss fragility — Replaced with readAllBundledCss which concatenates all .css files.

---

Deep re-review of updated code

Looked closely at several areas that weren't covered in the first review:

• substitute function correctness — The tokenRe regex shares state across substitute calls from root.walk(), but this is safe: walk() is synchronous (PostCSS Container.each), and lastIndex is explicitly reset to 0 on each call. The lastIndex adjustment formula correctly positions the cursor at m.index + replacement.length regardless of whether the replacement is shorter, longer, or equal in length. This is a faithful port of icss-utils replaceValueSymbols, with a defensive improvement (explicit lastIndex reset that the original lacks).

• traceKeySorter — Character-identical to the original postcss-modules implementation. The asymmetric < vs <= operators match.

• postcss version alignment — Both vinext and Vite resolve to the same postcss@8.5.10 instance at runtime. The bundled postcss-modules plugins __require("postcss") at runtime, so AST nodes created by vinext's Loader are fully compatible.

• Bare specifier resolution anchor — nodeRequire is anchored at vinext's source directory via createRequire(import.meta.url), which mirrors the original FileSystemLoader behavior (anchored at Vite's dist directory via __require). Both fall back to path.resolve(root, relativeDir, specifier) when require.resolve fails, which is the correct user-project-relative resolution. Not a regression.

• Config lifecycle — The getResolvedConfig closure correctly threads the resolved config (captured in configResolved) to the Loader (constructed in config()). The #readSource guard handles the edge case.

• Both Loader installation paths (lines 1635-1692) correctly handle the presence/absence of postcssOverride/sassPreprocessorOptions, ensuring the Loader is always installed.

Clean. Ship it.

[ask-bonk]

Review posted on PR #1393. Here's a summary:

All four items from the previous review have been addressed correctly in commit 6791600:

1. require.resolve ESM fix — uses createRequire(import.meta.url)
2. replace → replaceAll with function replacement — prevents $&/$1 interpretation
3. Promise.all divergence documented
4. User-supplied Loader detected and warned (override skipped)
5. Test findBundledCss replaced with robust readAllBundledCss

Deep re-review found no new issues. Verified:

• substitute function's shared regex + lastIndex adjustment is correct (faithful port of icss-utils)
• traceKeySorter is character-identical to the original postcss-modules implementation
• postcss version alignment — same 8.5.10 instance at runtime, no AST incompatibility
• Bare specifier resolution anchor mirrors the original FileSystemLoader behavior
• Config lifecycle wiring is correct

Approved.

github run