fix(config): preserve native TypeScript dynamic imports#2353
fix(config): preserve native TypeScript dynamic imports#2353james-elicx wants to merge 7 commits into
Conversation
|
/bigbonk review for issues |
|
|
/bigbonk review for issues |
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: native TypeScript dynamic imports
I verified the fix resolves the Vite module runner has been closed failure (reverting next-config.ts to main and running the new test reproduces the error, and the fix makes it pass). The direction is correct and matches Next.js's intent. But there is one correctness regression and a few parity gaps worth addressing before merge.
1. (blocking) Silent config loss for CJS-style .ts/.mts configs under "type": "module"
The new branch takes priority for all .ts/.mts configs, and only falls back to the Vite path when the native import() throws. A CJS-style config (module.exports = {...}) inside a "type": "module" package does not throw under Node's native type-stripping loader — it silently produces an empty module namespace ({ default: undefined }). unwrapConfig then returns mod.default ?? mod = the empty namespace, so the user's config is dropped with no error.
Reproduction with real Node (not Vitest):
proj/package.json -> {"type":"module"}
proj/next.config.ts -> module.exports = { env: { VIA: "module.exports" } };
PR would return config = {} // expected { env: { VIA: "module.exports" } }
Before this PR, that config loaded correctly via the Vite path + cjsGlobalsInjectorPlugin. The repo explicitly supports CJS-style .ts configs (see test "exposes a CommonJS module/exports object inside next.config.ts", and the loadNextConfig docstring notes the "type": "module" + CJS shape is "the common shape after vinext init").
This is not caught by the test suite because Vitest intercepts import() with Vite's own pipeline, so the new test never exercises Node's real native loader. (You can confirm: the module.exports-in-.ts test only passes because its tmpdir has no "type":"module", and the dynamic-import test only passes because Vite handles the inner import().)
Suggested fix: only accept the native result when it actually produced a config — e.g. treat a namespace with no usable export as a fall-through, not a success:
if (nativeModule !== undefined) {
const ns = nativeModule as Record<string, unknown>;
// A CJS `.ts` under "type":"module" yields an empty namespace under
// native type-stripping; fall through to the Vite path in that case.
if (ns.default !== undefined || Object.keys(ns).length > 0) {
return await unwrapConfig(nativeModule, phase);
}
}Even better: mirror Next.js v16.2.6, which only uses native import as a fast path and otherwise transpiles (handleCJS). See transpileConfig — native import is gated behind __NEXT_NODE_NATIVE_TS_LOADER_ENABLED === 'true' AND process.features.typescript, and any non-throwing-but-empty / require-based config still routes through SWC. A focused test with "type":"module" + module.exports would lock this down.
2. Parity: native gate is broader than Next.js
Next.js gates the native path on both process.env.__NEXT_NODE_NATIVE_TS_LOADER_ENABLED === 'true' and process.features.typescript (transpile-config.ts:148-156). This PR gates only on process.features?.typescript. On Node 22.18+ the feature is on by default, so the PR will take the native path for every .ts/.mts, whereas Next.js still requires the env opt-in. This widens the surface where the regression in (1) bites and diverges from Next.js parity. Worth matching the env gate, or at least documenting the deliberate divergence per AGENTS.md.
3. Parity / interop: .cts is excluded
The branch handles .ts and .mts but not .cts. Node's native TS support covers .cts too. Confirm whether this is intentional — if .cts is meant to keep flowing through the Vite/CJS path, a one-line comment explaining why would help (the existing comments at lines 984/993 mention .cts for the Vite path, so the omission reads as accidental).
4. unwrapConfig vs Next.js interopDefault
Next.js consistently extracts .default from the native module (config.ts:1722 interopDefault, transpile-config.ts:156 .default). unwrapConfig's mod.default ?? mod is looser and is what enables the silent-empty-config issue in (1). Tightening this (only use .default, and treat its absence as a fall-through) would align with Next.js and fix the regression in one place.
Minor
- The native path uses raw
configPathforpathToFileURL(configPath), while the rest of the function usessafeRealpath(path.resolve(configPath))(normalizedConfigPath) for symlink-correct comparisons. Not a correctness bug here (import resolves the path itself), just noting the inconsistency. - The empty
catch {}swallows all native-import errors, including genuine syntax/type errors in the config, which then silently re-run through Vite. Next.js surfaces these (warnOnce+ rethrow on real failure). Consider distinguishing "native loader unavailable" from "user config is broken".
Validation done
- Reverted impl to
main: new test fails withVite module runner has been closed— confirms the test is meaningful and the fix works in-suite. vp test run tests/next-config.test.ts— 211 passed.vp checkon both files — clean.- Standalone Node repro confirms the CJS-
.ts-under-type:moduleregression (returns{}).
Recommendation: address (1) before merge (it's a silent data-loss regression for a documented supported config shape); (2)–(4) are parity/robustness improvements that would be good to fold in while here.
|
/bigbonk review for issues |
|
Review posted to PR #2353. Summary of what I found: Findings1. (Blocking) Silent config loss regression — The new native-import branch takes priority for all This is a documented supported config shape (per the 2. Parity — Next.js gates native import on both 3. 4. Plus two minor notes (realpath inconsistency, empty I verified the fix is genuinely effective (reverting the impl reproduces the original |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: native TypeScript dynamic imports
I re-reviewed against the current head (c550645), which is past the earlier ask-bonk review. The three follow-up commits (retain CJS fallback, detect native CJS references precisely, cover CJS native loader fallback) add AST-based referencesUnboundCjsGlobals gating, which resolves the previously-blocking silent-config-loss concern. I verified the core behavior in real Node 24 (not just Vitest, where import() is intercepted), and the fix is sound.
Verified working (standalone Node, process.features.typescript === 'strip')
| Scenario | Native import() |
Routing | Result |
|---|---|---|---|
dynamic-import async config (await import('./foo.ts')) under type:module |
succeeds, runner stays alive | native | ✅ { FOO: "foo" } — the bug this PR fixes |
module.exports = {...} .ts under type:module |
throws module is not defined |
AST detects unbound module → Vite path |
✅ config preserved |
pure-ESM .ts |
succeeds | native | ✅ |
tsconfig @/foo alias config |
throws ERR_MODULE_NOT_FOUND |
catch → Vite path |
✅ |
extensionless sibling import (./helper) |
throws ERR_MODULE_NOT_FOUND |
catch → Vite path |
✅ |
require mentioned only in a comment/string |
n/a | AST finds no unbound ref → native | ✅ (PR test covers this) |
So the earlier blocking finding (silent empty config for CJS-.ts-under-type:module) does not reproduce on the current code: native strip throws cleanly for module.exports, and even before the throw, referencesUnboundCjsGlobals proactively routes those configs to the Vite path. vp check and tests/next-config.test.ts (214) both pass.
On the earlier parity notes
I pulled Next.js transpile-config.ts (canary). Two clarifications on the prior review's parity items:
-
__NEXT_NODE_NATIVE_TS_LOADER_ENABLEDgate — Next.js sets this env var to'true'unconditionally inpackages/next/src/bin/next.tsat CLI startup. It's an internal kill-switch (flipped to'false'after a failure within a session), not a user opt-in. So gating only onprocess.features.typescript, as this PR does, is behaviorally equivalent to Next.js's default path. Not a real divergence. A one-line comment noting this would preempt future confusion, but it's not blocking. -
CJS pre-check is a deliberate (good) divergence — Next.js does not inspect for CJS globals; it just runs
import()and falls back on throw. vinext's AST pre-check is stricter and necessary, because vinext's Vite fallback path supports CJS-global injection that Next.js handles via SWC. This is the right call and matches the AGENTS.md "deliberate, documented divergence" bar.
Minor (non-blocking)
.ctsnot on the native path (only.ts/.mts). Node's native loader supports.cts, but.ctsis CJS-by-extension and almost always referencesmodule/exports, so it'd be routed to Vite anyway. Behavior is correct; a brief comment on the extension list would make the omission read as intentional rather than accidental.- Double read of the config file for TS configs (
fs.readFileSynchere, then again inside the Vite plugin transform on fallback). Negligible, only on the TS path. catch {}scope — I confirmed it only wraps theimport()call (module load/parse), not the async-config invocation inunwrapConfig. Genuine load failures still surface via the Vite fallback'swarnConfigLoadFailure, and runtime errors thrown by an async config function propagate normally. No errors are silently lost.
Verdict
No blocking issues on the current head. The implementation is correct, well-tested, and matches Next.js's effective default behavior. Recommend merge after CI green; the minor comments above are optional polish.
|
Review posted to PR #2353. SummaryI reviewed the current head ( Key findings:
Verdict: No blocking issues on the current head. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: native TypeScript dynamic imports
Reviewed the current head (6acc0b0 fix(config): harden native CJS detection), which is one commit past the last ask-bonk review. I verified the core behavior in real Node 24 (process.features.typescript === 'strip'), where import() is not intercepted by Vitest, and re-checked the CJS-detection logic against a battery of edge cases. No blocking issues on the current head.
What I verified in real Node (not Vitest)
| Scenario | Native import() |
Detection routes to | Result |
|---|---|---|---|
async config doing await import('./foo.ts') under type:module |
succeeds, runner stays alive | native | ✅ { VALUE: "from-dynamic-import" } — the bug this PR fixes |
module.exports = {...} .ts under type:module |
does not throw — yields empty namespace {} |
AST detects unbound module → Vite path |
✅ config preserved |
Important correction to the earlier review thread: on Node 24, module.exports = {...} in a .ts under "type":"module" does not throw module is not defined — native type-stripping returns an empty module namespace silently. So the safety here rests entirely on referencesUnboundCjsGlobals routing it to Vite before native import is attempted. It does so correctly, which is what closes the original blocking concern — but the throw-based fallback (catch {}) would not have caught this case on its own.
CJS detection is robust (probed for false negatives)
A false negative (CJS config wrongly routed to native → silent empty config) would be the dangerous failure mode. I ran the detection logic against the parser and confirmed it correctly routes to the Vite path for: module.exports assignment, bare require(), require in a default parameter, typeof require, module.foo member access, bare exports, __dirname, require in template literals, spread ...require(), decorators, getters, labeled/static blocks, with statements, and module inside TSAsExpression/TSNonNullExpression. It correctly allows the native path for genuinely-shadowed bindings (catch param, for-of binding, destructure alias) and for CJS tokens that only appear in comments/strings or as TS type names (interface module, type require). The new isErasedTypeScriptNode + default-parameter visitation from this last commit are sound.
Parity confirmed against Next.js transpile-config.ts
__NEXT_NODE_NATIVE_TS_LOADER_ENABLEDis set to'true'unconditionally inpackages/next/src/bin/next.tsat CLI startup (it's an internal kill-switch flipped to'false'after a failure, not a user opt-in). So gating only onprocess.features.typescriptis behaviorally equivalent to Next.js's default. The earlier parity concern was overstated.- Next.js does
(await import(...)).defaultand falls back to SWC (handleCJS) on throw. The PR'scatch {} → Vite pathmirrors this. The AST pre-check is a deliberate, stricter divergence required because vinext's Vite fallback does CJS-global injection that Next.js does via SWC — this is the right call and meets the AGENTS.md "deliberate, documented divergence" bar.
Non-blocking notes (optional polish)
-
Silent
catch {}diverges from Next.js'swarnOnce. Next.js emits a one-time warning when native import fails before falling back; the PR swallows the error silently. For a config with a genuine error (not a tsconfig-alias / extensionless-import fallback), the user gets no signal from the native attempt — the Vite path's own error reporting is the only surface. Consider a debug-level log to aid diagnosis. Confirmed thiscatchonly wraps theimport()call, so runtime errors thrown by an async config function still propagate. -
unwrapConfig'smod.default ?? modis looser than Next.js's strict.default. It's harmless on the native path only because CJS configs are pre-filtered to Vite, so the native branch always has adefaultexport. A short comment noting that invariant would prevent a future refactor from reintroducing the silent-empty-config risk if the pre-filter is ever weakened. -
.ctsis intentionally not on the native path (only.ts/.mts). This is correct —.ctsis CJS-by-extension and would be routed to Vite anyway — but the extension list reads as accidental. A one-line comment would help, matching the existing.ctscomments on the Vite path (lines ~1161). -
Double file read on the TS path (
fs.readFileSynchere, then again in the Vite transform on fallback). Negligible, TS-path only.
Validation
- Real-Node repro of the dynamic-import fix: ✅ returns the imported value.
- Real-Node repro of
module.exports-under-type:module: empty namespace, but AST correctly routes to Vite → config preserved. referencesUnboundCjsGlobalsprobed against ~25 edge cases — no false negatives found.vp test run tests/next-config.test.ts— 216 passed.
Verdict: Correct, well-tested, and matches Next.js's effective default. Recommend merge after CI green; notes above are optional.
Reviewed at 6acc0b0.
|
Review posted to PR #2353. SummaryI reviewed the current head ( Key findings:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: preserve native TypeScript dynamic imports
Reviewed the current head (1c0d28d fix(config): resolve async configs inside runner), which is past all three earlier ask-bonk reviews in this thread. Those reviews covered the native-import() + referencesUnboundCjsGlobals AST approach (commits ab161c1..6acc0b0). That entire approach was reverted in the final commit. The current code contains no native import(), no process.features.typescript gate, and no AST CJS detection — so the prior parity discussion, the silent-empty-config concern, and the .cts/catch {} notes are all stale and no longer apply.
What the current head actually does
Instead of native import, it now loads a thin virtual module (virtual:vinext-resolved-next-config) through Vite's runnerImport. That virtual module import * as's the real config, applies the CJS/ESM unwrap rules, and awaits a function-form config before runnerImport returns — which is what keeps the Vite runner alive across a deferred await import('./foo.ts'). Everything stays inside Vite's pipeline (cjsGlobalsInjectorPlugin + vite-plugin-commonjs), so the native type-stripping loader is never involved.
This is strictly more robust than the abandoned native approach, and it sidesteps every concern raised earlier in the thread. No blocking issues.
Verified in real Node 24 (process.features.typescript === 'strip'), runner not stubbed
| Scenario | Result |
|---|---|
async config doing await import('./foo.ts') under type:module |
✅ { FOO: "foo" } — the bug this PR fixes |
module.exports = {...} .ts under type:module |
✅ { VIA: "module.exports" } — the prior "blocking regression" does not reproduce; goes through Vite + injector |
function-form module.exports = (phase) => ({...}) |
✅ phase passed correctly |
async config that throws |
✅ error propagates (and warnConfigLoadFailure fires) — not silently swallowed |
| pure-ESM named-exports-only (namespace fallback) | ✅ { ONLY_NAMED }, no __vinext_cjs_* leakage |
default + extra named exports |
✅ prefers default, named exports don't pollute config |
module.exports mentioned only in a string (false positive) |
✅ falls through to ESM default (sentinel identity check works through the virtual module) |
extensionless sibling import (./helper) |
✅ — failed under the old native loader, works now |
tsconfig @/* alias import |
✅ |
__dirname + fs.readFileSync (node-api-cjs style) |
✅ |
| fresh evaluation across two loads of a mutated config | ✅ 1 → 2, no staleness despite the fixed virtualConfigId |
vp test run tests/next-config.test.ts — 216 passed. vp check on the changed file — clean. Reverting next-config.ts to main reproduces Vite module runner has been closed, confirming the test is meaningful and the fix works in-suite (and, unlike the native approach, the test now genuinely exercises the runner because Vitest's runnerImport is the runner under test).
One non-blocking maintainability note
The unwrap logic is now duplicated. The main path inlines the CJS/ESM resolution + function invocation as a generated string in the virtual module's load hook (lines ~1011–1016):
const cjsValue = cjsExports != null && (cjsExports !== cjsInitial || (typeof cjsExports === "object" && Object.keys(cjsExports).length > 0)) ? cjsExports : undefined;
const value = cjsValue ?? configModule.default ?? configModule;
export default typeof value === "function" ? await value(<phase>, { defaultConfig: {} }) : value;while unwrapConfig (lines 654–672, still used by the loadConfigViaRequire CJS fallback at lines 896/899) and resolveConfigValue express the same rules in TypeScript. I confirmed the two are currently behaviorally equivalent (including the cjsInitial identity sentinel and function-form invocation), but they now have to be kept in sync by hand, and the inline copy is an untyped template string that won't be caught by vp check if it drifts. Per AGENTS.md ("generated entry modules should stay thin"), consider having the virtual module import and call a small shared helper (e.g. export the unwrap rules from a typed module and reference them) rather than re-encoding the sentinel logic as a string. Optional, not blocking.
Verdict
Correct, well-tested, and a cleaner approach than the reverted native-import path. The earlier blocking finding in this thread targeted code that no longer exists. Recommend merge after CI green; the duplicate-unwrap note above is optional polish.
Reviewed at 1c0d28d.
|
Review posted to PR #2353. SummaryI reviewed the current head ( What the new code does: loads a thin virtual module that imports the real config, applies the CJS/ESM unwrap rules, and Verification (real Node 24, runner not stubbed):
No blocking issues. One optional maintainability note: the unwrap logic is now duplicated between |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
…onfig-dynamic-import
1 participant
Summary
Next.js parity
Fixes both failures from run 28143992598:
test/e2e/app-dir/next-config-ts-native-ts/dynamic-import-esm/next-config-ts-dynamic-import-esm.test.tstest/e2e/app-dir/next-config-ts-native-mts/dynamic-import-esm/next-config-ts-dynamic-import-esm.test.tsVinext previously loaded the module namespace with Vite's temporary
runnerImport()and invoked the exported async config only after the runner had closed. A laterawait import('./foo.ts')from the config function then failed withVite module runner has been closed.The fix loads a thin virtual module that imports the real config, resolves its ESM/CommonJS export shape, and awaits function-form configs before returning from
runnerImport().Validation
vp test run tests/next-config.test.ts— 216 passedvp check— clean.tsdeploy suite — 1 passed.mtsdeploy suite — 1 passedFull suites are deferred to CI.