Skip to content

Extend SSR import rewriter to cover side-effect and dynamic import forms - #3175

Merged
kojiwakayama merged 2 commits into
mainfrom
fix/ssr-import-coverage
Jul 30, 2026
Merged

kojiwakayama merged 2 commits into
mainfrom
fix/ssr-import-coverage

Conversation

@kojiwakayama

Copy link
Copy Markdown
Contributor

Split out of #3114 so the dependency-pinning PR's "flag-off is behavior-identical to main" guarantee becomes literal. This carries the un-flagged SSR import-rewriting correctness fixes on their own, so they merge and soak independently of the 243-file pinning diff.

What changes

  • rewritePathAliases / rewriteRelativeImports now also match side-effect imports (import "@/x.js") and dynamic imports (import("@/x.js")), which main silently left unrewritten — a correctness bug in SSR module resolution.
  • The async path (rewriteSSRImportsCompatAsync) is rewritten from regex replaceAsync helpers to the parser-based parseImportEdits/applyImportEdits pass, which discovers all import forms in one walk.
  • shouldKeepBareSpecifier matches protocols case-insensitively and keeps protocol-relative (//) URLs external.
  • rewriteBareImports tolerates zero whitespace between the keyword and specifier (\s+ → \s*).

Every hunk is byte-identical to the corresponding code on phase0/dependency-pinning, so when this merges and main is merged back into that branch, these hunks drop out of #3114's diff cleanly and its remaining flag-off surface is unchanged from main.

Tests

New ssr-adapter.test.ts covering all six import forms (named / side-effect / dynamic × alias / relative) against both sync and async entrypoints, plus focused per-form unit tests. Full pre-push suite passed on push.

Part of veryfront-issue-inbox#240 Phase 0. Prerequisite for #3114.

Previously ALIAS_IMPORT_PATTERNS and RELATIVE_IMPORT_PATTERNS only
matched `from "..."` syntax, so `import "@/x"`, `import "./y.js"`,
`import("@/x")`, and `import("../z.js")` passed through unrewritten in
SSR bundles.

This change:
- Expands ALIAS_IMPORT_PATTERNS and RELATIVE_IMPORT_PATTERNS to cover
  all three import forms: named (from), side-effect (import "..."), and
  dynamic (import("...")).
- Replaces the replaceAsync + two-pass regex approach in
  rewriteSSRImportsCompatAsync with the parser-based
  rewriteInternalModuleImportsAsync (using the already-present
  parseImportEdits/applyImportEdits from import-edit.ts), which
  discovers all import forms in one pass.
- Tightens shouldKeepBareSpecifier to use a compact regex and also
  catches protocol-relative URLs (//cdn.example.com/...).
- Tightens rewriteBareImports regex from \s+ to \s* to handle the
  zero-whitespace edge case.

Split out of #3114 (phase0/dependency-pinning) so the pinning PR's
claim that flag-off is byte-identical to main becomes literal once this
lands. Part of veryfront-issue-inbox#240 Phase 0.
@kojiwakayama
kojiwakayama requested a review from kwakayama as a code owner July 30, 2026 04:11
Copilot AI review requested due to automatic review settings July 30, 2026 04:11
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the SSR import rewrite adapter to correctly rewrite additional import forms (side-effect and dynamic imports) and refactors the async rewrite path to use the shared parser-based import edit pass for more complete import discovery.

Changes:

  • Expand alias and relative rewriting to cover side-effect imports (import "@/x.js") and dynamic imports (import("@/x.js")).
  • Refactor the async SSR rewrite path to use parseImportEdits/applyImportEdits instead of regex-based async replacement.
  • Adjust bare-import rewriting helpers (protocol detection and from"..." whitespace tolerance).

Verification

  • Not run in this environment.
  • Safest next step:
    • deno test --no-check --allow-all src/transforms/import-rewriter/ssr-adapter.test.ts
    • deno test --no-check --allow-all src/modules/server/ssr-import-rewriter.test.ts

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/transforms/import-rewriter/ssr-adapter.ts Adds alias/relative coverage for side-effect and dynamic imports; switches async rewriting to parser-based import edits; tweaks bare import handling.
src/transforms/import-rewriter/ssr-adapter.test.ts Adds coverage for the newly supported import forms across sync and async SSR rewrite entrypoints.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/transforms/import-rewriter/ssr-adapter.ts Outdated
Comment thread src/transforms/import-rewriter/ssr-adapter.test.ts
… edge-case tests

The bare-import matcher's first-character class ([^"'./]) means a
specifier starting with "/" can never reach shouldKeepBareSpecifier, so
the startsWith("//") branch was dead code; removed rather than widening
the matcher (which would be a behavior change). The same removal is
mirrored on phase0/dependency-pinning to keep the byte-identical merge
posture.

Adds the two review-requested tests: no-whitespace `from"..."` matching
(minified output) and mixed-case protocol URLs staying external.
Copilot AI review requested due to automatic review settings July 30, 2026 04:48
kojiwakayama added a commit that referenced this pull request Jul 30, 2026
Mirrors the same removal on fix/ssr-import-coverage (#3175 review):
the bare-import matcher's first-character class means a specifier
starting with "/" never reaches this function, so the startsWith("//")
branch was dead code. Keeping both branches byte-identical preserves
the clean merge posture once #3175 lands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src/transforms/import-rewriter/ssr-adapter.ts:276

  • Async rewriting uses /^(?:\.\.?\/|\/)[^?#]+\.js$/ to identify relative imports, which also matches protocol-relative URLs like //example.com/mod.js. That would incorrectly rewrite external imports. Make the leading / branch reject a second /.
    const rewrite = specifier.startsWith("@/")
      ? buildAliasRewrite(specifier.slice(2), options)
      : /^(?:\.\.?\/|\/)[^?#]+\.js$/.test(specifier)
      ? buildRelativeRewrite(specifier)

src/transforms/import-rewriter/ssr-adapter.ts:217

  • RELATIVE_IMPORT_PATTERNS treats any specifier starting with / as a local relative import, which also matches protocol-relative URLs like "//example.com/mod.js". That would incorrectly append ?ssr=...&v=... and break external URL imports. Exclude protocol-relative (//) here so only true root-relative paths (/foo.js) are rewritten.

This issue also appears on line 273 of the same file.

const RELATIVE_IMPORT_PATTERNS = [
  /(\bfrom\s+)["']((?:\.\.?\/|\/)[^"']+\.js)["']/g,
  /(\bimport\s+)["']((?:\.\.?\/|\/)[^"']+\.js)["']/g,
  /(\bimport\s*\(\s*)["']((?:\.\.?\/|\/)[^"']+\.js)["']/g,
];

src/transforms/import-rewriter/ssr-adapter.test.ts:86

  • There’s no test ensuring protocol-relative specifiers (//example.com/...) are left untouched. This is important because the relative-import rewrite patterns treat leading / as internal and can accidentally rewrite protocol-relative URLs. Adding sync + async assertions here would prevent regressions.
  it("keeps mixed-case protocol URLs external", () => {
    const code = `import x from "HTTPS://example.com/mod.js";`;
    assertEquals(rewriteSSRImportsCompat(code, opts), code);
  });

@kojiwakayama
kojiwakayama enabled auto-merge July 30, 2026 04:56
@kojiwakayama
kojiwakayama added this pull request to the merge queue Jul 30, 2026
Merged via the queue into main with commit a0dfecd Jul 30, 2026
29 checks passed
@kojiwakayama
kojiwakayama deleted the fix/ssr-import-coverage branch July 30, 2026 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants