Skip to content

fix(build): support dynamic package subpath imports - #2746

Merged
james-elicx merged 2 commits into
mainfrom
codex/fix-package-dynamic-imports
Jul 28, 2026
Merged

fix(build): support dynamic package subpath imports#2746
james-elicx merged 2 commits into
mainfrom
codex/fix-package-dynamic-imports

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Summary

  • transform variable bare-package imports through wildcard package.json exports while preserving Node export precedence, conditions, invalid-subpath rules, and array semantics
  • generate Vite glob maps that support workspace packages, physical node_modules packages, nested captures, and literal glob metacharacters
  • mirror nodejs.org's next-intl dynamic locale-package pattern in the ecosystem fixture while using next-intl's public setRequestLocale API

Downstream reproduction: https://github.com/nodejs/nodejs.org/compare/main...james-elicx:nodejs.org:james/vinext?expand=1

Validation

  • vp check packages/vinext/src/plugins/extensionless-dynamic-import.ts tests/extensionless-dynamic-import.test.ts tests/ecosystem.test.ts tests/fixtures/ecosystem/next-intl
  • vp test run tests/extensionless-dynamic-import.test.ts (30 tests)
  • vp run vinext#build
  • vp test run tests/ecosystem.test.ts -t next-intl
  • npx --no-install vinext build in the next-intl fixture
  • independent review loop: clean after all findings were addressed

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@pkg-pr-new

pkg-pr-new Bot commented Jul 28, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2746
npm i https://pkg.pr.new/create-vinext-app@2746
npm i https://pkg.pr.new/@vinext/types@2746
npm i https://pkg.pr.new/vinext@2746

commit: 8298bf2

@github-actions

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

Comment thread packages/vinext/src/plugins/extensionless-dynamic-import.ts Fixed
Comment thread packages/vinext/src/plugins/extensionless-dynamic-import.ts Fixed
Comment thread packages/vinext/src/plugins/extensionless-dynamic-import.ts Fixed
Comment thread packages/vinext/src/plugins/extensionless-dynamic-import.ts Fixed
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared 8298bf2 against base 73f56ef using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 134.4 KB 134.4 KB ⚫ -0.0%
Client entry size (gzip) vinext 122.0 KB 122.0 KB ⚫ 0.0%
Dev server cold start vinext 2.99 s 2.99 s ⚫ -0.2%
Production build time vinext 3.27 s 3.29 s ⚫ +0.7%
RSC entry closure size (gzip) vinext 106.4 KB 106.4 KB ⚫ -0.0%
Server bundle size (gzip) vinext 181.9 KB 182.0 KB ⚫ +0.0%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@ask-bonk

ask-bonk Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review the current PR diff for concrete correctness, security, and downstream compatibility issues. Be concise and post the verdict before the workflow timeout.

@ask-bonk

ask-bonk Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

UnknownError: ProviderInitError

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review the current PR diff for concrete correctness, security, and downstream compatibility issues. Be concise and post the verdict before the workflow timeout.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review only packages/vinext/src/plugins/extensionless-dynamic-import.ts in this PR for concrete correctness and security issues. Do not run tests or inspect unrelated files. Post a concise verdict immediately.

@ask-bonk

ask-bonk Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review: extensionless-dynamic-import.ts (PR #2746)

Scoped to the single file requested. Verdict: no blocking correctness or security issues found. The design is sound; the flagged items below are minor/informational.

Security model (re: CodeQL "improper code sanitization" ×4) — false positives

The four CodeQL alerts flag runtime "code construction depends on an improperly sanitized value." In practice the dynamic value only ever indexes a build-time-fixed import.meta.glob object (__vinextModules[__vinextResolvedPath]). There is no eval/dynamic import(<expr>); a lookup can only resolve to a module the glob already enumerated within the resolved prefix. On top of that static boundary the PR layers correct defense-in-depth:

  • Build-time: isWithinPackage on both absoluteGlob and absolutePrefix, node_modules/./../separator rejection in decodePackageTarget + decodeRequestCapturePattern, and a !<prefix>**/node_modules/** negative glob.
  • Runtime: __vinextInvalidSubpath rejects ?, #, ., .., node_modules, and embedded //\ per decoded segment; decodeURIComponent failures reject.

The runtime __vinextInvalidSubpath check runs over __vinextSegments (the whole subpath after the package name), which is a superset of the dynamic __vinextCapture range, so a .. injected via ${...} is caught before lookup. This is the correct model — CodeQL doesn't model the static glob map.

Correctness — sound

  • __vinextCapture = __vinextPath.slice(requestPrefix.length, -requestSuffix.length) does not re-verify prefix/suffix presence, but requestPrefix/requestSuffix originate from the literal quasi text that was already matched against the export-key wildcard at transform time, so those bytes are guaranteed present at runtime. Only interpolated segments are dynamic. Sound.
  • Empty requestSuffix correctly maps to captureEnd = undefined (slice-to-end) via the falsy check, avoiding slice(start, -0.length)slice(start, 0).
  • Node export precedence (comparePatternKeys: longest base, then longest key; insertion-order independent) plus hasHigherPrecedenceExportIntersection correctly bail when a variable request could cross into a more-specific or exact export (incl. null targets) — well covered by the new tests.
  • resolveExportTargets correctly takes only the first array element and never skips to a later target with different semantics.
  • Multi-wildcard: export keys are restricted to a single * via splitSingleWildcard; only the capture pattern expands (bounded to ≤4 wildcards). Consistent between build glob and runtime.

Minor / informational (non-blocking)

  1. isViteInternalAlias mutates a shared RegExp's lastIndex. It resets to 0 before and after .test(), so it's safe under single-threaded execution, but mutating a config-owned RegExp is a fragile pattern if that regex is ever /g and used concurrently elsewhere. Consider new RegExp(find.source, find.flags.replace("g","")) for a non-mutating test. Cosmetic.
  2. buildPackageGlobPatterns caps wildcard expansion at 4 and returns null (native import) beyond that — reasonable, but silent. Fine as-is given it degrades safely to the un-transformed import.
  3. escapeGlobLiteral branches on process.platform at call time. Correct for the current single-target build, but note the produced glob keys are platform-specific; not an issue since transform runs on the build host.

No changes required for correctness or security. LGTM.

@ask-bonk

ask-bonk Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Posted the verdict to PR #2746.

Verdict: no blocking correctness or security issues in packages/vinext/src/plugins/extensionless-dynamic-import.ts.

Key points:

  • CodeQL's 4 "improper code sanitization" alerts are false positives. The dynamic value only indexes a build-time-fixed import.meta.glob object — there's no eval or dynamic import(<expr>). A lookup can only resolve to a pre-enumerated module within the resolved prefix. On top of that static boundary the PR layers correct build-time (isWithinPackage, node_modules/./.. rejection, !**/node_modules/** negative glob) and runtime (__vinextInvalidSubpath, decode-failure rejection) defenses. The runtime check runs over a superset of the dynamic capture range, so .. injected via ${...} is caught.
  • Correctness is sound: the capture prefix/suffix come from already-matched literal quasi text (guaranteed present at runtime); empty suffix correctly maps to slice-to-end; Node export precedence and cross-export intersection guards are correct and well-tested; single-wildcard export keys keep the build glob and runtime consistent.
  • Only minor/cosmetic notes (shared-RegExp lastIndex mutation in isViteInternalAlias, silent wildcard-count cap) — all non-blocking with safe degradation to native imports.

LGTM.

github run

@james-elicx
james-elicx marked this pull request as ready for review July 28, 2026 20:12
@james-elicx
james-elicx merged commit 795855e into main Jul 28, 2026
110 of 113 checks passed
@james-elicx
james-elicx deleted the codex/fix-package-dynamic-imports branch July 28, 2026 20:12
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.

2 participants