feat(init): detect css modules and auto-install compatibility workarounds - #3059
NriotHrreion wants to merge 23 commits into
Conversation
commit: |
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1114197494
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
/bigbonk review for issues. 8 min time limit. |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Author identity unknown *** Please tell me who you are. Run git config --global user.email "you@example.com" to set your account's default identity. fatal: empty ident name (for runner@runnervmgx7h7.re4o1eegj32epalimfikus0oth.dx.internal.cloudapp.net) not allowed |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
…is element spreading
|
/bigbonk review for issues — time limit: 10 minutes; please return a concise clean verdict or actionable in-scope findings. |
|
Posted 2 actionable inline findings. Targeted tests and |
|
/bigbonk review for issues — time limit: 10 minutes; please return a concise clean verdict or actionable in-scope findings. |
|
Posted 3 actionable inline findings. Targeted tests and |
|
/bigbonk review for issues — time limit: 10 minutes; please return a concise clean verdict or actionable in-scope findings. |
|
Posted 3 actionable inline findings. |
|
Posted 2 actionable inline findings. Targeted init tests and |
|
/bigbonk review for issues — time limit: 10 minutes; please return a concise clean verdict or actionable in-scope findings. |
|
Posted 4 actionable inline findings. |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues — time limit: 10 minutes; please return a concise clean verdict or actionable in-scope findings. |
|
Posted 2 actionable inline findings. |
|
Addressed both timed Big Bonk findings in |
|
/bigbonk review for issues — hard time limit: 10 minutes; review exact head ba59f32 after resolving the hidden-source-directory scan and variable-bound ESM/CommonJS defineConfig callback findings. Be verdict-first and return explicit LGTM/no findings or only concise actionable in-scope findings before the deadline. |
|
1 actionable inline finding posted. |
|
Addressed the callback-shadowing finding in |
|
/bigbonk review for issues — hard time limit: 10 minutes; final review of exact head 42fab6a after resolving callback-shadowed helper imports/requires. Be verdict-first: verify only hidden-source CSS Module discovery and static config callback resolution/binding safety for ESM and CommonJS, then return explicit LGTM/no findings or concise actionable findings before the deadline. |
|
Not LGTM: 1 actionable CommonJS binding-safety finding posted inline. Hidden-source discovery and ESM handling had no findings; 237 targeted tests pass. |
|
Addressed the CommonJS directive-prologue/TDZ finding in |
|
/bigbonk review for issues — hard time limit: 10 minutes; final exact-head review of a96c284 after placing CommonJS helper aliases after their source declarations. Verdict first: review only the cumulative hidden-source CSS Module scan and static ESM/CommonJS config callback resolution/binding safety, then post explicit LGTM/no findings or concise actionable findings before the deadline. |
|
Not LGTM: 1 actionable hidden-source scan finding posted inline. ESM/CommonJS callback resolution and binding safety had no findings. |
|
Addressed the hidden-filename finding in |
|
/bigbonk review for issues — hard time limit: 10 minutes; final exact-head review of 1645f3b after adding root and nested dot-prefixed CSS Module filename discovery. Verdict first: inspect only the cumulative hidden-source scan and static ESM/CommonJS config callback resolution/binding safety, then post explicit LGTM/no findings or concise actionable findings before the deadline. |
| if (firstArgument.type === "SpreadElement") return undefined; | ||
| const argumentObject = unwrapObject(firstArgument); | ||
| if (argumentObject) return argumentObject; | ||
| if (firstArgument.type === "Identifier") { |
There was a problem hiding this comment.
P2: Resolve identifiers bound to static config callbacks
A valid static form such as const makeConfig = () => ({ plugins: [] }); export default defineConfig(makeConfig) still aborts init. This identifier is sent to findVariableObject(), but that resolver only accepts object/identifier/defineConfig initializers, so the arrow/function initializer returns undefined; the CommonJS equivalent fails the same way. Resolve function-valued bindings through findCallbackObject() before rejecting the config.
| if (callback.type === "FunctionExpression" && callback.id) bindings.add(callback.id.name); | ||
| for (const parameter of callback.params) collectPatternBindings(parameter, bindings); | ||
| if (callback.body.type === "BlockStatement") { | ||
| for (const statement of callback.body.body) { |
There was a problem hiding this comment.
P2: Include nested function-scoped var bindings
Only direct callback-body declarations are recorded here, but var declarations in nested statements are hoisted to the callback scope. For example, with an existing patchCssModules import, defineConfig(() => { if (false) { var patchCssModules; } return { plugins: [] }; }) reuses that imported name even though every generated reference resolves to the hoisted local undefined; nested var createHash/path break the scoped-name method similarly. Recursively collect function-scoped var declarations (without crossing nested functions), or allocate aliases conservatively.
| commonJs, | ||
| ) | ||
| : commonJs | ||
| ? ensureNamedRequire( |
There was a problem hiding this comment.
P2: Preserve CommonJS directive prologues when adding requires
For the common input "use strict"; const { defineConfig } = require("vite"); ..., this new helper require is inserted at offset 0 because requireInsertionOffset() stops on the leading expression statement. That moves "use strict" out of the directive prologue and silently changes the config runtime semantics; the later crypto/path requires do the same. Skip directive-prologue statements before scanning leading requires.
|
Not LGTM: 3 actionable inline findings posted. Hidden-source discovery passed review; all 239 targeted tests pass. |
2 participants
Closes #2992
Overview
Vite scopes CSS Modules before running user PostCSS plugins, which differs from Next.js and can silently break transformations that depend on original selectors, such as
postcss-extend-rule.vite-css-modulesrestores the expected processing order, but its default class-name hashing can produce different results across vinext's separate server and client environments (see #2992 (comment)).This PR makes CSS Modules projects work consistently by automatically setting up
vite-css-modulesand deterministic, project-relative class names, preserving PostCSS behavior while preventing server/client class-name mismatches in development and production builds.What changed
vinext init, scan the original Next.js project folder for css modules, if there is, enable the css modules compatibility setup path.vite-css-moduleswill be installed as a devDependency.vite-css-moduleswill be added to vite config as a plugin, along with thegenerateScopedName()config.Verification
vite-css-modulesis verified in the PR test(css): css module compatibility issue and workaround verification #3000pnpm test tests/init.test.ts tests/init-cloudflare.test.tsQuestion
Should we add an option (maybe a cli flag) to let developers decide whether to apply the css modules compatibility setup?