fix(module-loader): resolve dynamic import() specifiers for SSR (depends on #2999)

[mattboon]

Summary

await import("@/lib/x") and await import("../../lib/x.ts") inside getServerData both 500'd:

Module not found "file:///_vf_modules/lib/uses-crypto.js"

The SSR module loader discovers a module's local dependencies, transforms each to a temp file, and rewrites the importer to point at those files. That discovery used findStaticImportFromSpans, which deliberately skips import( (source-spans.ts:218). Dynamic specifiers were therefore never resolved; they fell through to the alias rewrite, which produced a /_vf_modules/… path that means nothing to the runtime's own resolver, and the render died.

Dynamic imports with a literal specifier are now collected alongside static ones. Their span covers just the quoted string, so the rewrite replaces the specifier and leaves await import(...) intact. Non-literal specifiers (import(ctx.query.get("m"))) are left alone — their target is only knowable at runtime.

Bugs 3 and 4 are this one defect, as the reproducer's fix hypothesis anticipated ("Likely a shared fix"). Shipping them together rather than splitting one change across two PRs.

Reproduction

• Routes: e-server-dynamic-at-alias.tsx (bug 3), f-server-dynamic-relative.tsx (bug 4)
• Tests: src/rendering/orchestrator/module-loader/dependency-resolver.test.ts

Test evidence

ok | 2 passed (7 steps) | 0 failed

Four new cases: dynamic @/ alias, dynamic relative carrying a .ts extension, mixed static+dynamic in one module, and a non-literal specifier that must be ignored. One asserts the rewritten output directly:

assertStringIncludes(rewritten, `await import("file:///tmp/out/lib/uses-crypto.abc.js")`)

Wider suite: deno task test:unit → 2525 passed | 0 failed.

SSR evidence

/test/e-server-dynamic-at-alias   500 -> 200
/test/f-server-dynamic-relative   500 -> 200

Not merely a 200 — the dynamically imported module actually executes. The page renders the real SHA-256 prefix of "hello":

SSR computed hash: <code …>2cf24dba</code>

Client evidence

PASS /test/e-server-dynamic-at-alias
PASS /test/f-server-dynamic-relative
2/2 routes hydrated clean

Related

• Bugs 3 and 4 of the reproducer matrix. These were the escape hatch for keeping server-only code out of the client bundle, so they were effectively unavailable on the pages router.

---

Chain: this PR is part of a 13-PR chain fixing the bugs catalogued in
veryfront-router-testing.
Its base is the previous PR in the chain, so the diff shows only this fix.
The root of the chain is #2999 (fix/ssr-lazy-import-graceful-degrade) — merge #2999 first, then
rebase the chain onto main.

Regression gate: deno task test:unit → 2525 passed | 0 failed; deno task lint,
deno task fmt:check and deno task typecheck all clean. The reproducer's full 56-route
matrix (ROUTES.txt + sweep.sh) was re-run after every fix: 7 routes improved, 0 regressed.
A 46-route Chromium hydration sweep (client-sweep.mjs) backs the client-side claims.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 80e8fe8e74

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Skip partially dynamic import() arguments

When the argument starts with a string but continues as an expression, for example import("./foo" + suffix), this treats the prefix as a literal dependency because it never checks that the token after the closing quote ends the first argument. If ./foo exists, the module loader rewrites only that substring to a file:// temp path and leaves + suffix, so runtime imports a bogus path instead of preserving the non-literal dynamic import. Please skip these partially dynamic specifiers unless the first argument is just a literal.

Useful? React with 👍 / 👎.

[kwakayama]

Score: 65/100

Requesting changes. This PR currently fails local type diagnostics.

Blocking issue:

• src/rendering/orchestrator/module-loader/dependency-resolver.test.ts:78 and :86 construct TransformedModuleDependency fixtures without the new required isDynamic field. deno check fails with TS2322 because ResolvedModuleDependency now requires isDynamic.

Fix by adding isDynamic: false to the existing static fixture dependencies, or by changing the fixture helper to default static dependencies to false.

Additional watch item: findDynamicImportSpans handles literal import("...") cases, but remains a source scanner. Please consider coverage for valid dynamic-import syntax variants, including comment/whitespace forms, so SSR resolution does not silently diverge again.

This PR is also stacked on lower open PRs and only shows CLA in the status rollup, so it is not merge-ready.

[mattboon]

Both points addressed, and the second one turned up real bugs.

Type error. Fixed by making the fixture helper stamp the field, so static fixtures do not repeat it at every call site: withSpan is now withStaticSpan, returning T & { start: number; end: number; isDynamic: false }. deno check src/rendering/orchestrator/module-loader/dependency-resolver.test.ts is clean (was 2x TS2322).

Dynamic-import syntax variants. Your watch item was justified: probing before writing the tests found four cases the scanner got genuinely wrong, all from using bare whitespace skipping where comments can appear.

• import(/* webpackChunkName */ "./a.js") was missed entirely
• import /* c */ ("./a.js") was missed
• import("./a.js" /* c */) was missed, the worst of the four: a whole-argument literal was misclassified as not-whole
• a comment on its own line inside the parentheses was missed

Fixed with a skipWhitespaceAndComments helper (reusing the existing comment handling, and deliberately not treating strings as skippable) applied at the paren lookup, the specifier lookup and the after-specifier check.

Coverage added for the variants: whitespace and newline forms, the four comment placements, await import(...) nested in an arrow function, import(...).then(...), ?query and #hash suffixes, a dynamic import inside a block comment, and a replaceSourceSpans round trip asserting the span still covers only the quoted specifier when comments surround it.

The deliberate skip rule is preserved throughout: a literal that is only the start of the argument (import("./foo" + suffix), .concat(...), a ternary, a template substitution) is still skipped rather than rewritten, including the new import("./a.js" /* then */ + suffix) form.

Verification: deno check clean on both test files; source-spans and dependency-resolver suites 33 steps passing; full src/transforms/mdx/esm-module-loader/ sweep 383 steps passing; deno fmt/deno lint clean.

[kwakayama]

Follow-up after fixes: approving. Dynamic and static specifier rewrite metadata is now preserved, including . Local verification: \running 1 test from ./src/transforms/import-rewriter/strategies/alias-strategy.test.ts
AliasStrategy ...
matches ...
should match @/ imports ... ok (1ms)
should not match scoped packages ... ok (0ms)
should not match relative imports ... ok (0ms)
matches ... ok (1ms)
rewrite ...
should rewrite @/ to relative path from root-level file ... ok (0ms)
should rewrite @/ from nested file ... ok (0ms)
should keep existing extension for known extensions ... ok (0ms)
should add .js extension when no known extension ... ok (0ms)
should rewrite @/ to /_vf_modules/ path for SSR target ... ok (0ms)
should rewrite @/ with nested path to /_vf_modules/ for SSR ... ok (0ms)
should normalize extension for SSR ... ok (0ms)
moduleServerUrl path ...
should use absolute path when moduleServerUrl is configured ... ok (0ms)
should handle file index path mismatch with moduleServerUrl ... ok (0ms)
moduleServerUrl path ... ok (0ms)
CSS file imports (issue #453) ...
should NOT append .js to .css imports for SSR ... ok (0ms)
should NOT append .js to .css imports with moduleServerUrl ... ok (0ms)
should NOT append .js to .css imports in browser fallback ... ok (0ms)
CSS file imports (issue #453) ... ok (1ms)
relative path fallback (no moduleServerUrl) ...
should handle file at components/elements depth correctly ... ok (0ms)
relative path when file index has different structure (known limitation) ... ok (0ms)
should rewrite an explicit .ts extension to .js ... ok (0ms)
should rewrite an explicit .tsx extension to .js ... ok (0ms)
should not double-append .js to an explicit .js extension ... ok (0ms)
relative path fallback (no moduleServerUrl) ... ok (1ms)
source extensions in moduleServerUrl and ssr targets ...
should rewrite .ts to .js with moduleServerUrl ... ok (0ms)
should rewrite .tsx to .js for ssr ... ok (0ms)
source extensions in moduleServerUrl and ssr targets ... ok (0ms)
rewrite ... ok (4ms)
AliasStrategy ... ok (7ms)
running 1 test from ./src/transforms/import-rewriter/strategies/relative-strategy.test.ts
RelativeStrategy ...
matches ...
should match ./ imports ... ok (1ms)
should match ../ imports ... ok (0ms)
should not match bare specifiers ... ok (0ms)
should not match absolute paths ... ok (0ms)
matches ... ok (1ms)
rewrite ...
should resolve to module server URL for SSR when moduleServerUrl is available ... ok (1ms)
should normalize .tsx extension to .js for SSR when no moduleServerUrl ... ok (0ms)
should normalize .ts extension to .js for SSR when no moduleServerUrl ... ok (0ms)
should return null for .js in SSR when no moduleServerUrl (no change needed) ... ok (0ms)
should resolve to module server URL for browser ... ok (0ms)
should return normalized specifier when no moduleServerUrl ... ok (0ms)
rewrite ... ok (2ms)
RelativeStrategy ... ok (3ms)

ok | 2 passed (40 steps) | 0 failed (76ms). Score: 93/100. Next step: merge after base stack and checks are green.

[kwakayama]

Clean follow-up after the approval above:

Score: 93/100.

Verification:

• deno test --allow-all src/transforms/import-rewriter/strategies/alias-strategy.test.ts src/transforms/import-rewriter/strategies/relative-strategy.test.ts

Next step: merge after the base stack and refreshed checks are green.

The base branch was changed.

[kwakayama]

Follow-up approval after #3004 merged and #3005 was rebased onto main.

Score: 92/100.

Rationale: dynamic import specifiers are now resolved for SSR without broadening static import behavior, and the focused resolver/module-loader regressions still pass after the rebase.

Verification:

• deno task audit
• combined targeted regression suite: 21 tests, 334 steps, 0 failed

Next step: wait for refreshed GitHub checks and required reviewer gate, then merge when green.