Repository navigation
fix(check): only flag config options used as property keys - #1778
Conversation
vinext check reported `webpack` as an unsupported config option even when the project had no custom webpack config. The analyzeConfig heuristic matched the bare word `webpack` anywhere in next.config (a comment, or a value substring like `react-server-dom-webpack`), producing false positives. Match the option name only when it appears as an actual object property key (`opt:`, method shorthand `opt(`, assignment `opt =`, or quoted `"opt":`).
commit: |
|
/bigbonk review for issues |
|
Review: fix(check): only flag config options used as property keysThe core change is a clear improvement. Switching from That said, the new heuristic does not fully deliver on the PR's stated goal of not flagging options that appear in comments — it only fixes comments that happen to lack a 1. Comments that mention the option followed by
|
|
To https://github.com/cloudflare/vinext.git |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
Addresses review: the boundary+follower regex alone only fixed comments
without a trailing :/(/= and still flagged options mentioned in comments
(// webpack: removed) or embedded in string values ("webpack:1234").
Add a single-pass, lexer-grade pre-pass (modeled on hasFreeCjsGlobal)
that removes comments and empties string/template-literal values before
running the per-option regexes. It is regex-literal aware so a quote
inside /['"]/ cannot hijack string state, and quoted property keys
("webpack":) are preserved. Runs in O(n) time so it is safe on large
files, consistent with the #1776 large-file fix.
Adds tests for the reviewer's comment and string-value cases plus
scanner edge cases (regex literals, template interpolation, division).
|
Thanks for the thorough review — both false-positive classes are now fixed properly rather than partially. Instead of relying solely on the boundary+follower regex (which, as you noted, still matched
On your minor points:
Added tests for both reviewer cases (comment-with-punctuation, string-value) plus scanner edge cases (regex literal not hijacking, template interpolation scanned as code, division not misread as regex). |
|
/bigbonk re-review for issues |
Re-review: fix(check): only flag config options used as property keysThe
Tests pass ( Findings1. Dot-notation child check still decouples parent and child (pre-existing, not a regression). The nested-option block (check.ts:898-906) tests module.exports = { experimental: { inlineCss: true }, foo: ppr } // -> flags experimental.ppr
module.exports = { i18n: { locales: ["en"] }, images: { domains: ["x.com"] } } // -> flags i18n.domainsThe second case is realistic: 2. Documented limitation is real but acceptable. The ternary key-position case ( 3. Behavior change vs old Minor / nits
Overall the core fix is solid and the new tests cover the important cases. The remaining false-positive surface is confined to the pre-existing dot-notation check, which is appropriate to handle as a separate follow-up. |
|
Re-review posted to PR #1778. SummaryThe Confirmed fixed:
Findings (none blocking):
Tests pass (137) and |
…keys
Addresses Bonk re-review findings:
- Dot-notation options (e.g. i18n.domains) were detected by testing the
parent block and child name as two independent regexes anywhere in the
file, so a config with both `i18n: {…}` and `images: { domains: [...] }`
wrongly reported `i18n.domains`. Extract the parent block body (naive
brace balancing is safe on already-stripped content) and require the
child to appear inside it.
- Escape interpolated option/key names before building RegExps via a new
escapeRegExp helper, future-proofing against keys with metacharacters.
Adds tests for both scoping cases.
|
Addressed the re-review findings:
139 check tests pass, /bigbonk re-review for issues |
Re-review: dot-notation decoupling + escape helperThe
The brace-balancing in Findings (none blocking)1. const a = { experimental: { inlineCss: true } };
module.exports = { experimental: { ppr: true } }; // experimental.ppr NOT flaggedOn 2. Quoted parent keys are still not matched (pre-existing, not introduced here). 3. Sub-object over-match (intentional, minor). VerdictThe core fix is solid and the new tests ( |
|
Re-review posted to PR #1778. SummaryThe Confirmed fixed:
Findings (none blocking):
The brace-balancing is safe since it runs on stripped content. The trade (cross-block precision over whole-file child matching) is the right call. Net improvement, safe to ship. |
Closes the two non-blocking findings from Bonk re-review:
- extractBlockBody only inspected the first matching parent block, so a
child key in a later same-named block (two `experimental: {}` objects)
was missed. Renamed to extractBlockBodies, iterating every match.
- The dot-notation opener now allows an optional closing quote, so quoted
parent keys (`"experimental": {}`) are matched, consistent with how the
per-option regex handles quoted keys.
Adds tests for both cases.
|
Closed both non-blocking findings from the re-review:
141 check tests pass, /bigbonk re-review for issues |
Re-review: scan all parent blocks + match quoted parent keysBoth non-blocking findings from the last round are now properly closed, and finding 3 is correctly left as documented intentional behavior. I verified each change by direct probing against the stripped-content pipeline plus the full test file. Finding 1 (first-block-only) — fixed ✅
Finding 2 (quoted parent keys) — fixed ✅The opener gained an optional closing quote ( No over-match regression from the optional quote: a string value Finding 3 (sub-object over-match) — left as-is ✅
Other notes
141 check tests pass and |
|
Re-review posted to PR #1778. SummaryBoth non-blocking findings from the last round are properly closed, and finding 3 is correctly left as documented intentional behavior. Verified each change by direct probing plus the full test file. Finding 1 (first-block-only) — fixed. Finding 2 (quoted parent keys) — fixed. Opener now allows an optional closing quote ( Finding 3 (sub-object over-match) — left as-is. Conservative-correct, documented, right default. 141 check tests pass, |
Replace the hand-rolled comment/string scanner + regex heuristics with a proper AST parse via oxc-parser (the project's own toolchain). analyzeConfig now reads option keys directly off the exported config object, which is exact: comments, string values, regex literals, template literals, and identifier substrings can never be mistaken for a config key. This deletes stripCommentsAndStrings, extractBlockBodies, and escapeRegExp (~115 net lines) and also handles cases the text scan couldn't: configs wrapped in plugin calls (withMDX(config)), variable-referenced configs, and TS as/satisfies annotations. The dot-notation child lookup is now a true parent/child relationship from the AST rather than scoped text matching. Adds oxc-parser as a dependency (already in the install tree via vite-plus).
|
Reworked the approach entirely per feedback that a hand-rolled scanner for a config file is overkill.
Deleted
Dot-notation options (
/bigbonk re-review for issues |
Vite already re-exports parseAst (its bundled oxc/rolldown parser), so
there's no need to add oxc-parser as a separate dependency. Parse with
{ lang: "ts" } so .ts configs with type annotations / as / satisfies parse
the same as .js. Types come from Vite's exported ESTree namespace.
Drops the oxc-parser dependency and catalog entry added in the previous
commit; vite is already a required peer dependency of vinext.
|
Good catch — dropped the
|
|
/bigbonk re-review for issues |
Re-review: AST-based
|
|
Re-review posted to PR #1778. SummaryThe AST-based rewrite ( One blocking finding — function-form config regression:
module.exports = (phase) => ({ webpack: (c) => c, basePath: "/app" });now reports nothing (
So the one config shape where Suggested fix: extend Non-blocking: parse-failure silent fallback (documented, acceptable) and |
Bonk re-review caught a regression vs main: function-form configs — a
documented Next.js pattern that vinext supports at runtime
(config/next-config.ts) — reported no options under the AST rewrite
because resolveObject did not look through functions.
Extend resolveObject to unwrap arrow/function configs: a concise arrow
body is the config object directly; a block body resolves the first
top-level return statement's argument. Covers both
`(phase) => ({…})` and `function(phase, { defaultConfig }){ return {…} }`.
Adds tests for both forms.
|
Fixed the blocking function-form regression.
So On the non-blocking notes: the parse-failure silent fallback is intentional (documented in the JSDoc), and /bigbonk re-review for issues |
|
/bigbonk re-review for issues |
4 similar comments
|
/bigbonk re-review for issues |
|
/bigbonk re-review for issues |
|
/bigbonk re-review for issues |
|
/bigbonk re-review for issues |
resolveObject handled FunctionExpression (module.exports = function(){…})
and arrow forms but not FunctionDeclaration, which is what
`export default function (phase) { return {…} }` parses to. That form
silently reported no options.
Add FunctionDeclaration to the function-form branch and widen the param
type to ESTree.Function. Adds a regression test.
|
Fixed one more function-form gap found in review: /bigbonk re-review for issues |
|
/bigbonk re-review for issues |
1 similar comment
|
/bigbonk re-review for issues |
Re-review: AST-based
|
|
Re-review posted to PR #1778. SummaryThe AST-based Confirmed working: concise arrow, block-body function, One finding (non-blocking, narrow false-negative):
Other notes (no action): top-level spread keys aren't followed (delta vs Verdict: Net improvement, safe to merge; the multi-phase miss is reasonable to track as a follow-up. |
resolveObject took only the first top-level return of a function body, so the canonical next/constants multi-phase form — where the phase-specific config is an early return nested in an if and the default is the trailing return — only reported the default branch's keys. Resolve all object literals a config expression can denote and merge their keys: collect every reachable return arg (descending through if/switch/try but not into nested functions) and handle ternary branches. Covers the PHASE_* if/return form and the ternary function form. Adds tests for both.
|
Closed the multi-phase finding. For function block bodies it collects every reachable module.exports = (phase, { defaultConfig }) => {
if (phase === PHASE_DEVELOPMENT_SERVER) return { trailingSlash: true }; // now reported
return { webpack: (c) => c };
};Also handles the ternary function form ( Remaining notes left as documented/accepted: top-level spread sources aren't followed (statically hard), and parse-failure → empty report is intentional. /bigbonk re-review for issues |
|
/bigbonk re-review for issues |
1 similar comment
|
/bigbonk re-review for issues |
Re-review: multi-phase branch merging + AST option detectionThe Confirmed working
Behavior deltas vs
|
|
Re-review posted to PR #1778. SummaryThe final round ( Confirmed fixed (the headline finding): the canonical Everything else verified working: function-via-variable, Behavior deltas (all acceptable):
No blocking findings. The AST approach is exact-by-construction, kills every prior false-positive class, and the remaining gaps are narrow conservative false-negatives on an advisory check. 140 tests pass, |
Problem
vinext checkreportedwebpackas an unsupported config option even when the project had no custom webpack config.analyzeConfigused the heuristicnew RegExp(\\b${opt}\b`), which matches the option name *anywhere* innext.config. So a project was flagged forwebpackif the file merely contained a comment (// removed our custom webpack config) or a value with the substring ("react-server-dom-webpack").webpackis the one users notice because it's the onlyunsupported` entry in the scanned list, but the loose matching affected every option.Fix
Two layers:
stripCommentsAndStringspre-pass — a single-pass, lexer-grade scanner (modeled on the existinghasFreeCjsGlobal) that runs before detection:"react-server-dom-webpack"→""), preserving quoted property keys ("webpack":) by peeking for a trailing:./['"]/can't hijack string state and swallow following code.Property-key regex — match the option only as an actual key (
opt:, method shorthandopt(, assignmentopt =, quoted"opt":), guarding against identifier substrings.Both the per-option and nested dot-notation checks run against the stripped content.
Tests
tests/check.test.ts:stripCommentsAndStringsunit tests: comments removed, string values emptied, quoted keys preserved,////*inside strings not treated as comments, escaped quotes, regex literal not hijacking, template interpolation scanned as code, division not misread as regex.analyzeConfig: webpack in a comment (with and without trailing punctuation) → not flagged; webpack/headers embedded in string values → not flagged (real keys still detected); webpack as method shorthand / quoted key → flagged.All
checktests pass;vp checkis clean.Notes
Accepted limitations (documented in the helper JSDoc, mirroring
hasFreeCjsGlobal): a value-position regex after a block}is read as division, and a string in ternary key position (cond ? "x" : y) is preserved. Both are rare in next.config and the check is advisory.