fix(app-router): support extensionless variable imports - #1958
Conversation
commit: |
|
|
Independent review hardened extension ordering/custom extensions/index resolution and prevented false-positive rewrites. CI is rerunning on commit 3926b53. |
|
/bigbonk |
PR #1958 Thread SummaryTitle: fix(app-router): support extensionless variable imports Author: james-elicx | Status: OPEN | Base: main ← What it doesFixes App Router support for relative, extensionless variable Changes (6 files, +238/-0)
Next.js parityPorts the focused regression fixture from Next.js's Validation
Thread activity
No outstanding review comments or change requests in the thread yet. |
|
/bigbonk review for correctness issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for correctness issues |
1 similar comment
|
/bigbonk review for correctness issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
/bigbonk correctness review — extensionless variable imports
Focused on the new transform in packages/vinext/src/plugins/extensionless-dynamic-import.ts. The core approach (rewrite extensionless relative variable import() into an import.meta.glob map + a runtime resolver that tries file extensions, then /index extensions) is sound and matches the existing require-context.ts pattern. The fixtures exercise the file-vs-directory precedence, JSON, and static-prefix cases well. A few correctness gaps stood out.
1. Transform is not scoped to user code — node_modules deps are rewritten too
transform() has no id filter and no node_modules guard. Any dependency module that contains a relative extensionless variable import() (e.g. an i18n lib doing import(\./${locale}`)) will be rewritten into a import.meta.glob("./**/*{…}")` over that package's own directory tree.
transform(code, id) {
if (!/\bimport\s*\(/.test(code)) return null;
const lang = langForId(id);
if (!lang) return null;
// …no node_modules / id filtering…The sibling plugins in this repo deliberately guard against this — dynamic-preload-metadata.ts uses exclude: /node_modules/ and an explicit id.includes("node_modules") bail, and client-reference-dedup.ts filters on id. require-context.ts gets away without a guard only because its trigger (require.context) is extremely narrow; this plugin triggers on a very common syntax (import(\./${x}`)`).
Deps that go through optimizeDeps won't hit this (esbuild/rolldown prebundle doesn't run Vite transform), but SSR/RSC-externalized and non-optimized deps do pass through transform hooks, so a third-party lib's own dynamic resolution can be silently replaced. Recommend adding a node_modules bail (and ideally restricting to the project root) so the transform only applies to user code, matching how the other plugins scope themselves.
2. Multi-interpolation templates only validate the first and last quasi
parseExtensionlessImport checks first for glob chars (line 117) and last for [.?#] (line 118), but never inspects the middle quasis of a multi-${} template. So these slip past the guards:
import(`./${a}*/${b}`) // middle quasi "*/" contains a glob metachar
import(`./${a}.bak/${b}`) // middle quasi ".bak/" contains a dot
import(`./${a}?x/${b}`) // middle quasi "?x/" contains a query-like charThese are mostly benign (the runtime key lookup just misses and falls into Promise.reject), but the intent of lines 117–118 is to refuse to rewrite anything with glob metacharacters or query/hash/extension semantics — and that intent isn't enforced for interpolated paths with more than one ${}. Worth either validating every quasi or documenting that only the boundary quasis are checked.
3. Glob scope drops the static filename prefix and sub-path segments
The glob is anchored only at the directory portion of the first quasi (everything up to the last /):
const directoryEnd = first.lastIndexOf("/") + 1;
const directory = first.slice(0, directoryEnd);
globPattern: `${directory}**/*{${moduleExtensions.join(",")}}`,For import(\./prefixed-${slug}`)the staticprefixed-prefix is dropped, so the glob becomes.//{…}over the entire sibling tree rather than./prefixed-. Likewise import(`./${a}/${b}`)globs.//*even though the resolvable set is narrower. Runtime correctness is preserved (the exact__vinextPathkey lookup still selects the right module), but every module in the subtree gets registered as a lazy chunk, which is broader than Webpack's contextual-import scoping and can bloat the chunk graph. Worth scoping the glob tighter (include the static prefix, restrict depth when no/` appears in the dynamic portion) to better match Webpack/Next.js behavior and avoid over-emitting chunks.
Minor
import(\./file.${ext}`)` (variable is the extension) is transformed; it happens to work when the resolved file is a module the glob captured, but it's another case the boundary-only quasi checks don't catch.
None of these block the happy path the PR targets, and the ported Next.js regression fixture passes. #1 (node_modules scoping) is the one I'd most want addressed before merge, since it can change the behavior of third-party code that the user never intended to transform.
|
Review posted to PR #1958. Summary of the correctness findings: Correctness review of PR #1958The core transform approach (rewrite extensionless relative variable
Plus a minor note that |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
/bigbonk review for correctness issues |
There was a problem hiding this comment.
/bigbonk correctness review — extensionless variable imports
Re-reviewed the transform after the hardening commits (3926b53…68a522c). The earlier round of concerns has been substantively addressed, so this pass focuses on what remains.
Previously-raised concerns — now resolved
node_modulesscoping — fixed.transformnow bails viaisDependencyId(id)(line 37), which normalizes separators and checks for/node_modules/. Covered by the "leaves dependency imports unchanged" test.- Glob over-scoping / dropped static prefix — fixed. When a filename prefix is present the glob is now anchored to it (
[${first},${first}/**/*], line 137), and there's a dedicated test assertingimport.meta.glob(["./components/prefixed-*","./components/prefixed-*/**/*"]). - Multi-interpolation quasi validation — fixed. The glob-metachar guard now runs over all quasis (
texts.some(...), line 124) rather than only the boundary quasis, andtexts.slice(1)rejects any dot in a non-leading quasi (line 125). Theit.eachtable covers./${a}*/${b},./${a}.bak/${b},./${a}?query/${b}, and./file.${ext}. - Regex backtracking (CodeQL alert #197) — fixed in
68a522c. The vulnerable^(?:\s|\/\*[\s\S]*?\*\/|...)*$pattern was replaced with the linearisImportPrefixscanner, and there's a regression test that feeds 10k repetitions of*//*. (The CodeQL alert predates this commit and should clear on the next scan.)
The runtime resolver order (exact key → path + ext → path + "/index" + ext) is correct and matches the file-vs-directory precedence the fixtures exercise (resolver-priority.tsx wins over resolver-priority/index.js). The ported Next.js regression test and the transform unit tests all pass locally.
Remaining minor observations (non-blocking)
-
Extension-priority parity differs from Next.js when both
foo.tsandfoo.jsexist. The runtime resolver iteratesconfig.resolve.extensions, whose vinext default is[".mjs", ".js", ".mts", ".ts", ".jsx", ".tsx", ".json"](JS before TS). For an extensionless variable import that could resolve to eitherfoo.jsorfoo.ts, vinext selects.js; Webpack/Next.js resolve.tsfirst. This is internally consistent with how vinext resolves everything else (it's Vite's order), so it's arguably correct for this toolchain — just worth being aware it's a deliberate divergence rather than a match. -
Extensionless-file keys depend on the bundler emitting them. The
suffixlessfixture relies onimport.meta.glob("./**/*")producing a"./suffixless"key (the exact-key branch). This works in dev.require-context.tsexplicitly documents that the prod glob "can drop extensionless keys that Webpack would include." The PR notes theVINEXT_BUILD=1deploy suite passed, so this is presumably fine — but it's the one behavior I'd want a prod-build assertion for, since the unit tests only check the generated string and the integration test only runs the dev server. A build-mode test (or a smoke check on a deployed preview) covering thesuffixlesscase would close the dev/prod-parity gap that AGENTS.md calls out. -
Nested
import()inside an interpolation isn't transformed.collectExtensionlessImportsreturns early after matching anImportExpression(line 96–97) and doesn't descend into the source's${}expressions. Soimport(./${(await import(./${x})).y})would leave the inner call untransformed. This is a contrived case and the inner call still works for static specifiers, so it's a missed-transform, not a miscompile.
Nothing here blocks the targeted happy path; the prior high-priority items are all addressed. Observation #2 (prod-build coverage for the extensionless-key case) is the only follow-up I'd actively suggest.
|
Review posted to PR #1958. Summary of correctness reviewI re-reviewed the transform in
The runtime resolver order (exact → Three non-blocking observations remain:
The PR is in good shape — the prior high-priority items are addressed and nothing blocks the targeted happy path. |
2 participants
Summary
import()calls into lazyimport.meta.globmodule mapsNext.js reference
test/e2e/app-dir/dynamic-import/dynamic-import.test.tsValidation
vp checkvp test run tests/extensionless-dynamic-import.test.ts tests/nextjs-compat/dynamic.test.tsVINEXT_BUILD=1 NEXT_TEST_CONCURRENCY=1 ./scripts/run-nextjs-deploy-suite.sh .nextjs-ref --retries 0 -c 1 test/e2e/app-dir/dynamic-import/dynamic-import.test.tsPPR/cache behavior is intentionally out of scope.