fix(config): honor custom TypeScript config parity#2247
Conversation
|
/bigbonk review for issues |
commit: |
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
/bigbonk reviewReviewed the custom Findings1. 2. Redundant double-parse of the custom config — 3. 4. CodeQL alert appears already resolved Things verified as correct (no action needed)
Overall this looks good to merge once the |
|
Posted the review to PR #2247. Summary of what I found: Two actionable items:
One minor note: Confirmed resolved/correct:
The implementation is well-tested and |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
/bigbonk review
Focused on the custom typescript.tsconfigPath parity work. The resolver itself (path/baseUrl precedence, circular-extends detection, package exports/tsconfig-field resolution, escape guards, dev watcher reload) is well-built and well-tested. I verified the previous /bigbonk review's findings against the actual Next.js 16.2.7 install in this repo's node_modules, and one of them is backwards — the real divergence is the opposite of what was reported. Details below.
Finding 1 (correcting the prior review): tsconfigPath: "" / null should warn-and-fall-back, not hard-error
The earlier review claimed Next.js's schema is z.string().optional() and therefore accepts "". That is not the schema shipped in next@16.2.7. The actual schema is:
// next/dist/server/config-schema.js
tsconfigPath: _zod.z.string().min(1).optional()So Next.js does flag "" — but the consequence is not a hard error. Two things happen:
-
Validation is non-fatal.
validateConfigSchema(next/dist/server/config.js:1496) splits Zod issues intowarningsandfatalErrorsvianormalizeNextConfigZodErrors(:103). Onlyimageserrors and specificexperimental.turbopack*keys are fatal; a.min(1)violation ontsconfigPathlands inwarnings. Next.js prints a warning and continues — it does not abort the build. -
The runtime falls back to the default.
load-jsconfig.js:117:const tsConfigFileName = config.typescript.tsconfigPath || 'tsconfig.json';
With
""(and withnull), the||selects'tsconfig.json'. So a Next.js build withtsconfigPath: ""warns once, then loads the defaulttsconfig.jsonand proceeds.
vinext instead throws a hard error and aborts the build — resolveTsconfigPath() (config/next-config.ts:1149-1157) throws on empty string, and even if it didn't, selectTsconfig() (index.ts:874) would throw Cannot read file because path.join(root, "") is a directory. The test at tests/next-config.test.ts:1765-1771 codifies this throw-on-""/null behavior, so the divergence is deliberate-by-test, not accidental — but it's the opposite of Next.js.
Per AGENTS.md ("If Next.js and vinext should behave differently ... it must be a deliberate, documented decision, not an accidental divergence"), this should either:
- match Next.js (treat falsy
tsconfigPathas unset → fall back to defaulttsconfig.json, optionally with a warning), or - be documented as an intentional stricter-than-Next.js choice, with the test comment linking to
load-jsconfig.js:117to explain why we reject what Next.js tolerates.
Rejecting non-string types (42) is more defensible since Next.js's 42 || 'tsconfig.json' → path.join(dir, 42) throws a TypeError at runtime anyway. But "" and null specifically resolve cleanly to the default in Next.js.
Finding 2 (prior review item 2 looks addressed): double-parse now has a rationale comment
The prior review flagged a "redundant double-parse" at the old index.ts:1852-1864. In the current head, the second loadTsconfigPathAliases(...) call (index.ts:1856-1858) is preceded by a comment explaining it is intentional (custom configs are owned by the paths plugin; this validates without materializing stale aliases). The drift risk between the two parsers (loadCustomTsconfigResolution vs loadTsconfigPathAliases) the prior review noted is still real — they are independent parsers that must agree on what is valid — but the call is now justified in-code. Reasonable to leave as-is; worth a focused test asserting both paths reject the same malformed configs if you want to lock the invariant down.
Finding 3 (agree with prior review, minor): dependencyRealRootsByProject is never invalidated
index.ts:804 memoizes the dependency-root list per project for the process lifetime. The custom-tsconfig watcher already does moduleGraph.invalidateAll() + full-reload on config change (index.ts:1535-1539); clearing this Map in the same handler would keep a dependency added mid-session from being misclassified as a project-root baseUrl candidate by isDependencyImporter (:806). Edge case, non-blocking.
Verified correct (no action)
- Path → installed-package → baseUrl resolution order in the custom resolver (
resolveId,index.ts:1566-1596) matches TS/Next.js semantics, including the baseUrl-vs-package disambiguation viacreateRequire(importerFile).resolve(id). - Wildcard precedence tiebreak in
matchCustomTsconfigPath(index.ts:790-801) keeps the longer fixed prefix and is stable on ties. - Export-target escape guards (
resolveTsconfigExportTarget,index.ts:382-393) runnode_modules/../ dot-segment checks after*substitution, and usereplaceAll("*", ...)(CodeQL alert 217 is moot on the current head). skipSelf: trueon everythis.resolveprevents recursion into this same plugin.loadCustomTsconfigreturns freshwatchedFilesSet copies from the completed cache (index.ts:733,:780) so callers can't mutate cached entries.
The implementation is strong; Finding 1 is the one I'd want resolved (or explicitly documented) before merge, since the PR's stated goal is parity and the prior review's parity assessment of that exact item was inverted.
|
Review posted to PR #2247. SummaryI reviewed the custom Finding 1 (corrects prior review) —
Finding 2 — the "redundant double-parse" the prior review flagged now carries an in-code rationale comment; reasonable to keep, though the two-parser drift risk remains. Finding 3 — agreed with prior review: I also confirmed the resolution order, wildcard tiebreak, export escape guards, |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review
Reviewed the custom typescript.tsconfigPath parity work against the current head (a901f31). The previous /bigbonk reviews were posted against earlier commits, so I re-checked their findings against the code as it stands now. Two of the three prior findings are now resolved; one parity gap remains.
Finding 1 (partially fixed — null still diverges from Next.js)
The prior review's headline item was that tsconfigPath: "" hard-errored instead of falling back. Commit a901f31 fixed exactly that — resolveTsconfigPath (config/next-config.ts:1149-1157) now returns undefined for "", and tests/next-config.test.ts:1765 codifies the fall-back. Good.
But null is still rejected (tests/next-config.test.ts:1773, it.each([42, null])("rejects invalid values")), and that remains a parity divergence:
- Next.js's schema is
z.string().min(1).optional()(next/dist/server/config-schema.js:716).nullfails it. - That failure is a non-fatal warning —
normalizeNextConfigZodErrors(next/dist/server/config.js:103-137) only marksimagesand specificexperimental.turbopack*keys as fatal; everything else (including atsconfigPathviolation) lands inwarnings, so the build continues. - At runtime
load-jsconfig.js:117doesconfig.typescript.tsconfigPath || 'tsconfig.json', sonull || 'tsconfig.json'→ falls back to the default tsconfig.
Net: Next.js warns once and proceeds with the default for null; vinext throws and aborts. Since "" was deliberately changed to match Next.js, null should get the same treatment for consistency (both are falsy values that Next.js resolves to the default via ||). Either fold null into the "" branch (if (value === undefined || value === "" || value === null) return undefined;) or, if the stricter behavior is intentional, document it per AGENTS.md with a comment linking load-jsconfig.js:117.
Rejecting 42 is more defensible — Next.js's path.join(dir, 42) throws a TypeError at runtime anyway — so I'd leave that as-is.
Finding 2 (resolved): redundant double-parse is gone
The prior review flagged a redundant second parse of the custom config purely for validation. In the current head that's gone: for custom configs tsconfigPathAliases is {} and the config is parsed once via loadCustomTsconfigResolution (index.ts:1908-1917). No second validating parse on the custom path. No action.
Finding 3 (still present, minor): dependencyRealRootsByProject is never invalidated
index.ts:866 memoizes the per-project dependency-root list for the process lifetime, and the custom-tsconfig watcher (index.ts:1590-1608) already does moduleGraph.invalidateAll() + full-reload on config change. Clearing this Map in the same handler would keep a dependency added mid-dev-session from being misclassified by isDependencyImporter (index.ts:868). Edge case, non-blocking.
Verified correct (no action)
- Path → installed-package → baseUrl resolution order (
resolveId,index.ts:1616-1659) matches TS/Next.js semantics, including the baseUrl-vs-package disambiguation viacreateRequire(importerFile).resolve(id). - Wildcard substitution uses
replaceAll("*", ...)atindex.ts:423andindex.ts:1631— CodeQL alert 217 ("replaces only the first occurrence") is moot on the current head. - Export-target escape guards (
resolveTsconfigExportTarget,index.ts:414-462) runnode_modules/../ dot-segment checks after*substitution, gate on.jsonextension, and reject backslashes on POSIX. skipSelf: trueon everythis.resolveprevents recursion into this same plugin.selectTsconfigcorrectly receives the resolved (""→undefined) path, so an emptytsconfigPathfalls through to default detection (index.ts:930-959).
Implementation is strong and well-tested. The one item I'd resolve (or document) before merge is the null divergence in Finding 1, since the PR's stated goal is parity and "" was already brought into line.
|
Review posted to PR #2247. SummaryI reviewed the custom Finding 1 — partially fixed;
Finding 2 — resolved. The redundant double-parse the prior review flagged is gone; custom configs are parsed once via Finding 3 — still present, minor. Verified correct: resolution order, wildcard tiebreak, export escape guards ( The implementation is strong and well-tested. The one item worth resolving (or documenting) before merge is the |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review
Reviewed against the current head (468969f), which is one commit newer than the prior /bigbonk reviews (a901f31). The new commit fix(config): align tsconfig selection warnings resolves the last open parity item and adds a typescript-presence gate. Verified the resolver against the actual next@16.2.7 install in node_modules (build/load-jsconfig.js, server/config-schema.js). Net: the prior reviews' Findings 1 and 2 are now resolved, Finding 3 remains. No new blocking issues.
Finding 1 (now resolved) — tsconfigPath: "" / null parity
The prior reviews flagged that "" (and later null) hard-errored instead of warning + falling back to the default. Commit 468969f aligns both with Next.js. resolveTsconfigPath (config/next-config.ts:1149-1167) now:
- returns
undefinedforundefined, - warns and returns
undefinedfornull/"", with messages that mirror the Zod issues Next.js surfaces ("Expected string, received null"/"String must contain at least 1 character(s)"), - throws only for non-string types (e.g.
42).
This matches next@16.2.7: tsconfigPath: z.string().min(1).optional() (config-schema.js:716) treats ""/null as non-fatal warnings, and load-jsconfig.js:117 does config.typescript.tsconfigPath || 'tsconfig.json', so falsy values fall back to the default. Rejecting 42 is a defensible divergence — Next.js's path.join(dir, 42) throws a TypeError at runtime anyway. Tests at tests/next-config.test.ts:1765-1793 codify all three branches. Good.
Finding 2 (resolved) — redundant double-parse is gone
Custom configs now parse once via loadCustomTsconfigResolution; tsconfigPathAliases is {} on the custom path (index.ts:1926-1928) and resolution happens dynamically in resolveId. No second validating parse. No action.
Finding 3 (still present, minor) — dependencyRealRootsByProject is never invalidated
index.ts:866 memoizes the per-project dependency-root list for the process lifetime, and isDependencyImporter (index.ts:868) reads it. The custom-tsconfig watcher (index.ts:1601-1619) already does moduleGraph.invalidateAll() + full-reload on config change but does not clear this Map. A dependency added mid-dev-session could be misclassified (treated as a project-root baseUrl candidate rather than a dependency). Edge case, non-blocking — clearing the entry for projectRoot in the same watcher handler would close it.
New in 468969f: hasProjectTypeScript gate — verified correct
selectTsconfig (index.ts:939-970) now gates on whether typescript resolves from the project (hasProjectTypeScript, index.ts:930-937). This faithfully reproduces load-jsconfig.js:
- TS present + custom path + file exists → custom (
useTypeScript = true). ✓ - TS present + custom path missing → falls back to
jsconfig.jsononly, never the defaulttsconfig.json— matches Next.js exactly (useTypeScript = false→if (!useTypeScript && existsSync(jsConfigPath))). Covered bytsconfig-paths-vite8.test.ts:858. ✓ - TS not installed → ignores
tsconfigPathentirely, usesjsconfig.json(useTypeScriptrequirestypeScriptPath). Covered bytsconfig-paths-vite8.test.ts:164. ✓
hasProjectTypeScript resolving from path.join(projectRoot, "package.json") is safe even when package.json is absent (createRequire only uses the path as a resolution base), matching hasNecessaryDependencies(dir, …).
Verified correct (no action)
- Export-target escape guards (
resolveTsconfigExportTarget,index.ts:414-462):replaceAll("*", …)(CodeQL alert 217 moot),./..segment rejection after substitution,node_modulesrejection,.jsonextension gate, POSIX backslash rejection. - Path → installed-package → baseUrl resolution order, wildcard tiebreak, and
skipSelf: truerecursion prevention all match TS/Next.js semantics. vp check(changed files),tests/next-config.test.ts,tests/tsconfig-paths-vite8.test.ts(44),tests/nextjs-compat/custom-tsconfig.test.ts(7), andvp run vinext#buildall pass locally.
Minor note (non-blocking)
- The
tsconfigPathwarning fires once per plugin instance. A hybrid app+pages build instantiatesvinext()more than once (see the build-ID comment atindex.ts:1872-1881), so a misconfiguredtsconfigPathcould warn twice. Next.js dedups via a module-level flag. Cosmetic only.
Implementation is strong and well-tested. The only carried-over item is the minor Finding 3 cache invalidation; nothing blocking.
|
Review posted to PR #2247. SummaryI reviewed the custom Findings:
New Verified correct: export escape guards ( No blocking issues. The implementation is strong and well-tested. |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review
Reviewed against the current head (1d5785f), which is two commits newer than the prior /bigbonk reviews (468969f). The two new commits are fix(config): require project TypeScript compiler (2a3243e) and fix(config): reject mixed tsconfig package exports (1d5785f). The earlier parity findings (""/null fallback, redundant double-parse) remain resolved. I found one new parity divergence introduced by 1d5785f, verified against the actual typescript@5.9.3 resolver via tsc --showConfig.
Finding (new, introduced by 1d5785f) — mixed package exports rejects the main export, diverging from TypeScript
resolveTsconfigPackageExport (packages/vinext/src/index.ts:481-483) now returns null for any exports object whose keys mix subpath keys (./…) with non-subpath keys (conditions like node, default):
if (subpathKeys.length > 0 && subpathKeys.length !== exportKeys.length) {
return null;
}This is correct for subpath access (extends: "preset/base"), but it over-rejects the main export (extends: "preset"). TypeScript only requires allKeysStartWithDot for subpath resolution; for the main export it falls back to exports["."] when present, even if other (condition) keys are mixed in. From node_modules/typescript/lib/typescript.js loadModuleFromExports:
if (subpath === ".") {
// string | array | noKeyStartsWithDot → use as mainExport
// else if hasProperty(exports, ".") → use exports["."]
} else if (allKeysStartWithDot(exports)) { /* resolve subpath */ }Repro — node_modules/preset/package.json:
{ "name": "preset", "exports": { ".": "./main.json", "node": "./node.json" } }tsconfig.json{ "extends": "preset" }→ TypeScript resolves./main.json(tsc --showConfigappliesmain.json's options). vinext throwsCannot read file 'preset'.← divergence.tsconfig.json{ "extends": "preset/base" }(with{ "./base": …, "node": … }) → TypeScript errorsFile 'preset/base' not found; vinext also rejects. ✓ matches (this is the case the commit's new test covers).
So the new test (tsconfig-paths-vite8.test.ts:822, "rejects package exports that mix subpaths and conditions") only exercises the subpath path; the main-export path was inadvertently broken. Per AGENTS.md this is an accidental, undocumented divergence from Next.js/TypeScript.
Suggested fix: scope the mixed-keys rejection to subpath access only — when packageSubpath === "" and exports["."] exists, resolve it. e.g. gate the early return null on packageSubpath (the main-export Object.hasOwn(exportsMap, exportKey) branch at :490 already handles . correctly once the early return is bypassed). Add a regression test for extends: "preset" against a mixed-keys exports object.
2a3243e (hasProjectTypeScript → typescript/lib/typescript.js) — verified correct
Switching the resolution probe from typescript/package.json to typescript/lib/typescript.js (index.ts:932) more faithfully reproduces Next.js load-jsconfig.js's hasNecessaryDependencies check, which resolves the compiler entry, not just metadata. The new test ("uses jsconfig when the app has TypeScript metadata without its compiler", tsconfig-paths-vite8.test.ts:200) correctly covers the "package.json present but no compiler" case, and the setupProject helper now writes the real lib/typescript.js. Good.
Carried-over items
- Finding (prior) —
dependencyRealRootsByProjectnever invalidated (index.ts:870, read at:880): still memoized for the process lifetime and not cleared in the custom-tsconfig watcher (index.ts:1615-1616, which doesmoduleGraph.invalidateAll()+full-reload). Minor edge case; clearing theprojectRootentry there would close it. Non-blocking. ""/nullparity (config/next-config.ts:1149-1167) and the redundant double-parse remain resolved.
Verified (no action)
- Export-target escape guards (
resolveTsconfigExportTarget,index.ts:414-462):replaceAll("*", …), dot-segment /../node_modulesrejection after substitution,.jsongate, POSIX backslash rejection — all correct. - Path → installed-package → baseUrl order, wildcard tiebreak,
skipSelf: truerecursion prevention. tests/tsconfig-paths-vite8.test.ts,tests/next-config.test.ts,tests/nextjs-compat/custom-tsconfig.test.ts(258 total) all pass locally on1d5785f.
The one item worth fixing before merge is the mixed-exports main-export divergence in 1d5785f, since the PR's stated goal is parity and the new commit narrowed acceptance beyond what TypeScript does.
|
Review posted to PR #2247. SummaryReviewed against the current head ( New finding (introduced by
TypeScript only requires Verified correct:
Carried over (minor, non-blocking): |
|
/bigbonk review for issues |
/bigbonk reviewReviewed against the current head ( Net: all four prior findings are now resolved or confirmed correct; only the minor cache-invalidation item carries over. No blocking issues. Finding (prior review's mixed-exports divergence) — now RESOLVED
if (packageSubpath && subpathKeys.length > 0 && subpathKeys.length !== exportKeys.length) {
return null;
}This matches TypeScript's
Finding 1 (
|
|
Review posted to PR #2247. SummaryI reviewed the custom Net: no blocking issues. All four prior findings are resolved or confirmed correct; only one minor non-blocking item carries over.
|
2 participants
Summary
typescript.tsconfigPathacross App, Pages, and middleware buildsValidation
vp test run tests/tsconfig-paths-vite8.test.ts(24 passed)vp checkvp run vinext#buildNext.js reference:
v16.2.6(ee6e79b1792a4d401ddf2480f40a83549fe8e722).