Skip to content

fix(css): support namespace CSS Module imports - #2345

Merged
james-elicx merged 5 commits into
mainfrom
codex/fix-scss-node-module-css-modules
Jun 26, 2026
Merged

james-elicx merged 5 commits into
mainfrom
codex/fix-scss-node-module-css-modules

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Summary

  • rewrite static namespace imports of CSS Modules to Vite's default locals export
  • preserve Next.js's CommonJS CSS Module compatibility semantics for package styles
  • cover the hybrid App + Pages production graph used by the failing fixture

Next.js parity

Fixes both failures from test/e2e/app-dir/scss/nm-module/nm-module.test.ts in deploy-suite run 28143992598.

Next.js intentionally configures CSS Modules with esModule: false for backwards compatibility, so import * as classes from "example/index.module.scss" exposes the locals object directly. Vite exposes that object as the default export; without this compatibility rewrite the namespace access is undefined and the stylesheet is tree-shaken.

References:

Validation

  • vp test run tests/css-module-imports.test.ts tests/scss-composes.test.ts — 9/9
  • focused vp check on touched files
  • exact Next.js v16.2.6 deploy suite, concurrency 1 — 2/2

Excludes cacheComponents, PPR, resume, and fallback-shell work.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@pkg-pr-new

pkg-pr-new Bot commented Jun 25, 2026 •

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2345
npm i https://pkg.pr.new/vinext@2345

commit: abf0d71

@github-actions

Copy link
Copy Markdown
Contributor
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
workers-cache preview production
web preview production

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jun 25, 2026 •

Copy link
Copy Markdown
Contributor

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@github-actions

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared abf0d71 against base de014d4 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 117.1 KB 117.1 KB ⚫ -0.0%
Client entry size (gzip) vinext 112.5 KB 112.5 KB ⚫ -0.0%
Dev server cold start vinext 2.53 s 2.52 s ⚫ -0.3%
Production build time vinext 3.07 s 3.04 s ⚫ -0.8%
RSC entry closure size (gzip) vinext 93.0 KB 93.0 KB ⚫ -0.0%
Server bundle size (gzip) vinext 156.9 KB 156.9 KB ⚫ +0.0%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

1 similar comment
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

/bigbonk review

Reviewed the namespace CSS Module import rewrite. Verified locally: vp test run tests/css-module-imports.test.ts tests/scss-composes.test.ts → 13/13 pass, and vp check on the touched files is clean. The hybrid App+Pages nm-module fixture build genuinely emits color:red and the hashed class, so the fix addresses the reported failure.

Verification I ran

  • Confirmed via a Rolldown lib build that Vite's CSS-module namespace import already exposes both named locals and a default locals object, so rewriting import * as classes → import classes preserves classes["red-text"] access. The rewrite matches Next.js's esModule: false semantics (locals on the default), as documented in the PR.
  • Confirmed ?inline / ?raw / with { type: "css" } / import type / named / default / dynamic imports are all correctly left untouched (the anchored CSS_MODULE_RE $ vs the unanchored hint regex is the mechanism — good).
  • Confirmed rewriteCssModuleNamespaceImports returns null (not throw) on syntax errors, so a malformed module won't break the transform pipeline.
  • Confirmed AST-binding overwrite handles commented specifiers (import * /* c */ as styles).

Assessment

The implementation is well-targeted and defensive. Behavior is gated behind cheap native hook filters (id + code), it delegates real logic to a typed, unit-tested helper rather than inlining in a generated entry, and the Next.js parity reference is documented. No correctness or parity issues found.

Minor / non-blocking

  1. scriptLanguage default → jsx for .mjs/.cjs/.js/.jsx. This is safe (oxc's jsx lang parses plain JS/ESM/CJS), and tested for .js-with-JSX. Worth a one-line comment noting the intentional fallthrough so a future reader doesn't "fix" it to js and lose JSX support in .js components.
  2. CSS_MODULE_HINT_RE / CSS_MODULE_RE duplication. The hint regex is intentionally unanchored (cheap pre-filter) while the rewrite regex is $-anchored (excludes querystrings). A brief comment on why the two differ would prevent an accidental "consolidation" that would re-include ?inline/?raw.

Neither is blocking. LGTM.

@ask-bonk

ask-bonk Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Posted the review on PR #2345.

Summary

I reviewed PR #2345 (fix(css): support namespace CSS Module imports) and found no blocking issues. The fix is sound and well-targeted.

What the PR does: Rewrites static namespace CSS Module imports (import * as classes from "x.module.scss") to default-export form (import classes from "x.module.scss"), matching Next.js's esModule: false compatibility semantics. This prevents node_modules CSS Modules from being tree-shaken and fixes the two nm-module failures.

Verification I performed:

  • Ran the PR's targeted tests: 13/13 pass (css-module-imports.test.ts + scss-composes.test.ts, including the new hybrid App+Pages build test that genuinely emits color:red).
  • vp check on touched files: clean.
  • Confirmed via a Rolldown lib build that Vite's namespace already exposes both named locals and a default locals object, so the rewrite preserves classes["red-text"] access.
  • Confirmed all exclusions work correctly (?inline/?raw/import type/named/default/dynamic/with { type: "css" }).
  • Confirmed the rewrite returns null (not throw) on parse errors, and uses AST bindings to handle commented specifiers.

Strengths: Behavior is gated behind cheap native hook filters, logic lives in a typed unit-tested helper (not inlined in a generated entry, per AGENTS.md), and Next.js parity references are documented.

Minor non-blocking notes: Suggested two clarifying comments — one on the intentional jsx fallthrough in scriptLanguage and one on why the hint/rewrite regexes differ (anchored vs unanchored).

github run

@james-elicx
james-elicx marked this pull request as ready for review June 25, 2026 17:04
@james-elicx
james-elicx merged commit 4edd1e1 into main Jun 26, 2026
58 checks passed
@james-elicx
james-elicx deleted the codex/fix-scss-node-module-css-modules branch June 26, 2026 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant