fix(build): bundle dynamic URL imports - #2893
Conversation
commit: |
|
|
/bigbonk review for issues |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(build): bundle dynamic URL imports
Reviewed the new vinext:dynamic-import-url pre-transform plugin, the index.ts wiring, the ported react-version fixtures/E2E, and the compatibility-support reclassification. This is a focused, well-tested change and I could not find any correctness issues. Targeted tests pass locally (vp test run tests/import-meta-url.test.ts tests/compatibility-support.test.ts → 52 passed) and vp check on the plugin is clean.
What I verified (all correct)
- Plugin ordering is the crux of the fix and is right.
createDynamicImportUrlPlugin()isenforce: "pre"and is placed in the plugin array beforecreateIgnoreDynamicRequestsPlugin(alsopre), so the literalnew URL(...).hrefimport is normalized to a bare relative specifier before the generic dynamic-request fallback can replace it with theMODULE_NOT_FOUNDruntime error. Array position between twopreplugins is what determines this, and it's correct here. - Specifier guarding is conservative and semantically safe.
getDynamicImportUrlSpecifieronly rewrites when the firstnew URLarg is a string literal starting with.//../, there are exactly 2 args, and the base isimport.meta.url(or the optional-chained form). Bare specifiers ("foo/bar.js") and absolute specifiers ("/style.css") are correctly rejected — both would change resolution semantics if rewritten toimport(...). The "preserves non-literal and absolute" test locks this in. new URLdetection matches Vite's own scope.isNewUrlExpressiononly matches bareURL, notglobalThis.URL/window.URL, consistent with Vite's asset detection and the existingimport.meta.urlhandling in this file.- Parser-language selection is consistent with the repo.
parserLanguageForModuleId(jsx fallback, ts for.ts/.mts/.cts, tsx for.tsx) mirrorsparserLanguageinmiddleware-export-validation.ts. The addedit.eachraw-ts/tsx cases guard the4ab0128fix where parsing plain-.jsas the default failed on type-annotated.ts/JSX sources. - CSS fixture placement is safe.
pages/api/react-conditions.csswill not be picked up as an API route — the pages/api scanner filters throughcreateValidFileMatcher(pageExtensions), which excludes.css.
Non-blocking observations
collectDynamicImportUrlSpecifiersearly-returns without visitingimport.meta.urlinside the matchednew URL. This is fine (a matched literal specifier can't contain a nested rewritable dynamic import), just noting the intent: once anImportExpressionmatches, its subtree is skipped. Nestedimport()inside a non-matching expression is still visited viaforEachAstChild, which is what we want.- Import attributes are preserved by construction. Because only the source-argument node range (
.source) is overwritten, animport(new URL(...).href, { with: { type: "json" } })keeps its options. No handling needed, but worth being aware there's nonode.optionsguard here (unlikeextensionless-dynamic-import.ts), and that's acceptable. - Documented semantic shift (URL import → bundled specifier).
import(new URL(...).href)resolves a runtime URL string in Next.js, whereasimport("./x")goes through the bundler graph. That's exactly the intended behavior for a bundled Workers deployment and the code comment explains it well — flagging only so future readers don't mistake it for an accidental divergence.
Suggestion (optional)
The compatibility reclassification (react-version → supported, NON_SUPPORTED_SUITES 33→32, needs-vite-equivalent 3→2) is asserted in tests/compatibility-support.test.ts, but those hardcoded counts are brittle across unrelated future edits. Not a change to make here — just a standing maintenance cost of the count-based assertions.
LGTM. CI is green.
|
Review posted to PR #2893. SummaryI reviewed the changes in PR #2893 ( What the PR does: Adds a new Key things I verified as correct:
Non-blocking notes I raised: the intentional URL→bundled-specifier semantic shift (well-documented in the code), the absence of an Verified locally: targeted tests pass (52 passed), |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(build): bundle dynamic URL imports
I independently reviewed the new vinext:dynamic-import-url pre-transform, the index.ts wiring, the ported react-version fixtures/E2E, and the compatibility reclassification. No correctness or security issues found — this is a focused, well-tested change. Targeted suites pass locally (tests/import-meta-url.test.ts 49 passed, tests/compatibility-support.test.ts 4 passed), and I verified the edge-case behavior with an out-of-band script.
Verified correct
- Ordering is the fix and it is right.
createDynamicImportUrlPlugin()(enforce: "pre") is placed beforecreateIgnoreDynamicRequestsPlugin(alsopre) in the array, so the literalnew URL(...).hrefimport is normalized to a relative specifier before the generic dynamic-request fallback can replace it with theMODULE_NOT_FOUNDruntime error. Between twopreplugins, array position decides, and it is correct here. - Specifier guarding is conservative and safe.
getDynamicImportUrlSpecifierrewrites only when arg0 is a string literal starting with.//../, there are exactly 2 args, and arg1 isimport.meta.url(or the optional-chained form). Bare and absolute (/style.css) specifiers are correctly rejected — both would change resolution semantics. The "preserves non-literal and absolute" test locks this in. - Import options are preserved by construction. Only the
.sourcenode range is overwritten, soimport(new URL(...).href, { with: { type: "json" } })keeps its options. I confirmed this directly:import("./a.js", { with: { type: "json" } }). - Nested and multiple imports handled. I confirmed
foo(import(new URL(...).href))andPromise.all([import(...), import(...)])both rewrite every occurrence.collectDynamicImportUrlSpecifiersearly-returning after a match only skips a matched literal's subtree (which can't contain a rewritable import), and still visits siblings. parserLanguageForModuleIdmatches the repo pattern inmiddleware-export-validation.ts, and the added raw-ts/tsxit.eachcases guard the4ab0128parse-by-language fix.- CSS fixture placement is safe.
pages/api/react-conditions.csswill not be picked up as an API route — the pages/api scanner filters throughcreateValidFileMatcher(pageExtensions), which excludes.css.
Non-blocking observations
codefilter missesimport(/* comment */ new URL(...)). The filter regex/\bimport\s*\(\s*new\s+URL\s*\(/does not tolerate a comment betweenimport(andnew URL((newlines are fine). In that case the pre-transform is skipped andignore-dynamic-requests— which parses the AST and ignores comments — would still replace the expression with theMODULE_NOT_FOUNDfallback. The underlying functionrewriteDynamicImportUrlshandles the comment form correctly (I verified), so only the filter is narrower than the implementation. This is an unusual source pattern and not a regression, just an incomplete corner of the fix. Consider relaxing the filter to/\bimport\s*\(/(still cheap) if you want the fix to cover it.- Client-environment behavior now changes too. The new plugin has no environment guard, so it rewrites user client code as well. Previously
ignore-dynamic-requestsleft user client code untouched (it only transforms node_modules there). Rewritingimport(new URL(...).href)→import("./x")in the client build is consistent with the intended bundle-the-module semantics and matches what the server does, so this is reasonable — flagging only so the uniform behavior is a conscious choice rather than incidental. - Documented semantic shift (URL import → bundled specifier).
import(new URL(...).href)resolves a runtime URL string in Next.js, whereasimport("./x")goes through the bundler graph. That is exactly the intended behavior for a bundled Workers deployment and the code comment explains it well. - Brittle count assertions. The compatibility reclassification asserts hardcoded totals (
NON_SUPPORTED_SUITES33→32,needs-vite-equivalent3→2) intests/compatibility-support.test.ts. Not a change to make here — a standing maintenance cost of count-based assertions across unrelated future edits.
LGTM.
|
Review posted to PR #2893. SummaryI reviewed PR #2893 ( What the PR does: Adds an Key things I verified as correct:
Non-blocking notes I raised:
LGTM. |
|
Independent review of immutable head Validation evidence:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(build): bundle dynamic URL imports
Independently reviewed the new vinext:dynamic-import-url pre-transform, the index.ts wiring, the ported react-version fixtures/E2E, and the compatibility reclassification. No correctness or security issues found — the fix is well-scoped and well-tested.
Verified locally on head 03f3980:
tests/import-meta-url.test.ts+tests/compatibility-support.test.ts→ 53 passedvp check packages/vinext/src/plugins/import-meta-url.ts→ clean- Out-of-band edge-case probing of
rewriteDynamicImportUrls(see below)
Verified correct
- Ordering is the fix and it's right.
createDynamicImportUrlPlugin()(enforce: "pre") is placed in the plugin array beforecreateIgnoreDynamicRequestsPlugin(alsopre), so the literalnew URL(...).hrefimport is normalized to a relative specifier before the generic dynamic-request fallback replaces it with theMODULE_NOT_FOUNDruntime error. Between twopreplugins array position decides, and it's correct. - Conservative, correctly-scoped rewriting. I probed the matcher directly. It rewrites only
import(new URL("./x", import.meta.url).href)with a.//../string literal, exactly 2 args, andimport.meta.url(orimport.meta?.url) base. It correctly leaves alone:?.hrefoptional-chained property,.toString(), computed["href"], 3-argnew URL, template-literal specifiers, non-literal specifiers, and absolute/xspecifiers. Critically, it does not touch barenew Worker(new URL("./worker.js", import.meta.url))orconst u = new URL(...).href; import(u)— only the exactimport(<newURL>.href)shape. Import options are preserved (import("./a.js", { with: { type: "json" } })), and nested/multiple occurrences (Promise.all([...])) all rewrite. - Comment-form covered.
import(/* webpackMode: "eager" */ new URL(...).href)is handled by both thecode: /\bimport\s*\(/filter andrewriteDynamicImportUrls; the build-integration test locks this in. parserLanguageForModuleIdmatches the repo pattern inmiddleware-export-validation.ts; the raw-ts/tsxit.eachcases guard the4ab0128parse-by-language fix.- Fixtures are a genuine integration test.
pages/api/pages-react-conditions.tscarries the realimport(new URL("./react-conditions.css", import.meta.url).href), and thecloudflare-encoded-pathsPlaywright project buildscf-app-basicwithvp build+wrangler dev, so the fix is exercised end-to-end against Workers. The.csssibling won't be misread as an API route (pages/api scanner filters throughcreateValidFileMatcher(pageExtensions)).
Non-blocking observations
-
Plugin lacks the environment/scope guards its sibling has — flagging for a conscious decision, not a bug.
createIgnoreDynamicRequestsPlugingates viashouldTransformVeryDynamicRequests: on the client consumer it only transformsnode_modules(minus transpiled packages), and it skips vinext source +@vitejs/plugin-rsc. The newcreateDynamicImportUrlPluginhas none of these — it runs on all JS/TS modules in every environment. In practice this is fine (I grepped: no vinext source or plugin-rsc module uses theimport(new URL(...).href)shape, so nothing new gets rewritten there), and rewriting user client code to a bundledimport("./x")is consistent with the intended semantics. But it is a real behavior change for client user code thatignore-dynamic-requestspreviously left untouched, so it should be a deliberate choice rather than incidental. -
No transform cache. Unlike both sibling
pre/posttransforms (ignore-dynamic-requestsusescreateTransformCache,import-meta-urlhas its own cache), the new plugin re-runsparseAston every module matchingcode: /\bimport\s*\(/on every transform. Given AGENTS.md's emphasis on keeping the common request/transform path light in large route graphs, consider reusingcreateTransformCache. -
Documented URL→bundled-specifier semantic shift.
import(new URL(...).href)resolves a runtime URL string in Next.js, whereasimport("./x")goes through the bundler graph. That's the intended behavior for a bundled Workers deployment and the code comment (lines 267-274) explains it well. -
Brittle count assertions.
tests/compatibility-support.test.tsasserts hardcoded totals (NON_SUPPORTED_SUITES33→32,needs-vite-equivalent3→2). Standing maintenance cost across unrelated future edits — not a change for this PR.
LGTM.
|
Review posted to PR #2893. SummaryI independently reviewed PR #2893 ( What the PR does: Adds an Verified locally:
Substantive non-blocking observation the prior reviews only partially covered: the new plugin lacks the environment/scope guards its sibling Overall: LGTM. |
james-elicx
left a comment
james-elicx
left a comment
There was a problem hiding this comment.
Review of exact head 03f39802439956e5eeee8b7dc9b3f44704ebfa21:
[P2] Keep the native dynamic-URL prescan narrow. createDynamicImportUrlPlugin() now uses code: /\bimport\s*\(/, which sends every JS/TS module containing any dynamic import — including dependencies — through rewriteDynamicImportUrls() and a full parseAst() walk. Only literal import(/* comments */ new URL(..., import.meta.url).href) forms can change, so this widens common-path work in exactly the performance-sensitive plugin path that the repository guidelines require filtering before JavaScript. The webpack-comment regression does not require that widening: I independently built with Rolldown using a comment-aware native filter (/\bimport\s*\(\s*(?:(?:\/\*[\s\S]*?\*\/|\/\/[^\r\n]*(?:\r?\n|$))\s*)*new\s+URL\s*\(/) and it invoked the handler for the commented target while skipping an ordinary import("virtual:dep"). Please narrow the native code filter to candidate dynamic-URL imports and retain the ordering regression before the ignore fallback.
Functional verification on this SHA is otherwise clean: focused tests 53/53, exact Next.js react-version.test.ts 5/5, Cloudflare Worker react-conditions.spec.ts 5/5, and vp check passed.
|
/bigbonk review for issues |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(build): bundle dynamic URL imports
Focused re-review at head ed0ca7a, after the perf(build): narrow dynamic URL import filter commit that addressed the earlier [P2]. The core fix (pre-plugin ordering + conservative AST rewrite) remains correct and well-tested. However, the narrowed code filter introduced in ed0ca7a has a catastrophic-backtracking (ReDoS) vulnerability that can hang the build.
[P1] The narrowed code filter regex has catastrophic backtracking
packages/vinext/src/plugins/import-meta-url.ts:72
code: /\bimport\s*\(\s*(?:\/(?:\*[\s\S]*?\*\/|\/[^\r\n]*(?:\r?\n|$))\s*)*new\s+URL\s*\(/,The construct \s*(?:comment\s*)* has overlapping whitespace consumers (\s* after (, and the \s* that trails each comment inside the repeated group). When the input has a run of comment-like sequences that never reaches new URL(, the engine backtracks exponentially trying every partition of the whitespace/comments. Since this filter runs against the full module source of every JS/TS module in every environment, a single module with a import(/* a */ /* b */ ... expr) prologue is enough to stall the build.
Measured with the exact production regex (import( + N /*x*/ blocks + a non-new URL token):
| N comments | time |
|---|---|
| 15 | 3 ms |
| 20 | 20 ms |
| 25 | 600 ms |
Roughly 30x per 5 comments (2^N). At ~35 comments the filter alone runs for minutes; a pathological/minified vendor file (or a deliberately crafted dependency) turns a build-time filter into a DoS. This is a regression introduced by ed0ca7a — the previous /\bimport\s*\(/ filter was linear.
Suggested fix — replace the comment-grammar with a bounded, linear lazy gap:
code: /\bimport\s*\([\s\S]{0,200}?\bnew\s+URL\s*\(/,This is O(n) (10 ms on a 2M-char input vs. a hang), still matches every relevant form — plain, block-comment, line-comment, and multiple magic comments between import( and new URL( — and still rejects ordinary import("dep"). The {0,200} cap keeps it from bridging an unrelated import(...) to a far-away new URL( while comfortably covering real webpack-magic-comment prologues; the AST-based rewriteDynamicImportUrls remains the source of truth for whether a rewrite actually happens, so a slightly looser filter is harmless. (I verified all the current comment fixtures still match and import("virtual:dep") / bare new URL(...) still skip.)
Please add a ReDoS guard test (e.g. assert the filter returns within a tight time budget on "import(" + "/*x*/ ".repeat(50) + "x)"), since the existing it.each comment cases don't exercise the failing-to-match backtracking path.
Everything else verified correct
- Plugin ordering (the crux) is right.
createDynamicImportUrlPlugin()(enforce: "pre") is placed beforecreateIgnoreDynamicRequestsPlugininindex.ts, so the literalnew URL(...).hrefimport is normalized before the generic dynamic-request fallback replaces it withMODULE_NOT_FOUND. getDynamicImportUrlSpecifieris conservative and semantically safe — only.//../string literals, exactly 2 args,import.meta.url(or optional-chained) base; bare/absolute specifiers and non-literal forms correctly rejected.- Import options preserved by construction (only
.sourcerange overwritten); nested/multiple imports handled. parserLanguageForModuleIdmatches themiddleware-export-validation.tspattern; the raw-ts/tsx cases guard the4ab0128fix.pages/api/react-conditions.csswon't be misread as an API route (pages/api scanner filters viacreateValidFileMatcher(pageExtensions)).
Non-blocking
- The new plugin lacks the environment/scope guards its sibling
createIgnoreDynamicRequestsPluginhas (client → node_modules only, skips vinext source + plugin-rsc) and has no transform cache. Harmless in practice (no vinext/plugin-rsc source uses this shape) but a deliberate uniform-behavior choice worth noting — already raised in prior reviews. - Brittle hardcoded compatibility counts in
tests/compatibility-support.test.ts(33→32,3→2).
Blocking on the [P1] ReDoS only.
|
Review posted to PR #2893. SummaryI re-reviewed PR #2893 ( [P1] ReDoS in the narrowed
|
| N comments | time |
|---|---|
| 15 | 3 ms |
| 20 | 20 ms |
| 25 | 600 ms |
~30x per 5 comments. Since the filter runs against the full source of every JS/TS module, a single pathological/minified/crafted module hangs the build. The prior /\bimport\s*\(/ filter was linear, so this is a regression from ed0ca7a.
Fix suggested: a bounded lazy gap, which I verified is O(n) (10 ms on 2M chars) and still matches every comment form while rejecting ordinary imports:
code: /\bimport\s*\([\s\S]{0,200}?\bnew\s+URL\s*\(/,Also recommended adding a ReDoS-guard test, since the existing it.each cases only exercise the matching path, not the exponential failing-to-match path.
Everything else (plugin ordering, conservative AST rewrite, import-option preservation, parser-language selection, fixture placement) verified correct. Non-blocking items (missing env/scope guards, no transform cache, brittle count assertions) noted but already raised in prior reviews.
I posted this as a request-changes review blocking only on the ReDoS.
|
Independent CodeQL/ReDoS and native-filter review of immutable head I re-reviewed the complete dynamic URL import filter follow-up, including the prior false-negative report. Security and correctness checks:
Independent validation on this exact SHA:
The local and remote heads still match and the worktree is clean. |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
james-elicx
left a comment
james-elicx
left a comment
There was a problem hiding this comment.
Review of exact head 08819bc9328cadaea27b9527f2b8e47e37953109:
NO FINDINGS.
The earlier performance and parser-boundary findings are resolved. The native code filter now performs only a bounded prefix check: direct import(new URL( candidates and leading-comment candidates reach AST validation, ordinary no-comment dynamic imports do not. Real-build coverage proves an ordinary import followed by an unrelated new URL(...) invokes the handler zero times, while commented ordinary imports take the documented one-parse validation path. Block comments, mixed comments, and LF/CRLF/CR/U+2028/U+2029 line terminators are all exercised through real Vite builds before the ignore fallback. Direct adversarial JavaScript-filter probes are linear/bounded, and CodeQL is green.
Independent validation on this SHA: focused import-meta/compatibility tests 62/62; exact Next.js test/e2e/react-version/react-version.test.ts 5/5 via the targeted wrapper; Cloudflare Worker react-conditions.spec.ts 5/5; vp check; and git diff --check. The currently failed E2E (app-router-client-cache) check is in the separately excluded cache-behavior domain and is not attributable to this change.
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(build): bundle dynamic URL imports
Independent review at head 08819bc. No correctness, security, or performance issues found. The prior [P1] ReDoS (from ed0ca7a) and [P2] filter-widening findings are both resolved by the current coarse prefix filter. The change is well-scoped and thoroughly tested.
Verified correct
-
Plugin ordering (the crux) is right.
createDynamicImportUrlPlugin()(enforce: "pre") sits beforecreateIgnoreDynamicRequestsPlugin(alsopre) in the plugin array (index.ts), soimport(new URL("./x", import.meta.url).href)is normalized toimport("./x")before the generic dynamic-request fallback would replace it with theMODULE_NOT_FOUNDruntime error. Between twopreplugins, array position decides — and it's correct. -
The AST rewrite is precise and conservative. I probed
rewriteDynamicImportUrlsdirectly (11 cases via the test harness). It rewrites only the exactimport(new URL("./"|"../", import.meta.url|import.meta?.url).href)shape, and correctly skips every near-miss:import(new URL("./x", import.meta.url)?.href)— optional-chained.href→ skipimport(new URL("./x", import.meta.url)["href"])— computed access → skipimport(new URL("./x", import.meta.url).toString())→ skip- template-literal specifier, absolute
/abs, bare"bare", missing base,globalThis.URL→ all skip import(new URL("./x", import.meta?.url).href)→ rewrite (optional-chained base handled)
-
Import options preserved by construction (only the
.sourcerange is overwritten); nested/multiple imports handled. -
ReDoS resolved (linear filter). The current
code: /\bimport\s*\(\s*(?:new\s+URL\s*\(|\/)/is a bounded prefix check with no overlapping/nested quantifiers. I stress-tested the exact production regex against the old backtracking vectors —import(+ 500k/*x*/,import(/+ 500k\r\n//, 2M-char non-matching prefixes — all complete in <8ms. CodeQL findings 282/283 were against the removed comment-grammar regex ined0ca7a, not this one. -
Coarse-filter tradeoff is documented and tested. The filter now admits
import(/* … */ "./ordinary")too, which costs one extraparseAst(no rewrite). Theonly parses commented ordinary dynamic imports for AST validationtest locks this to exactly one parse, anddoes not invoke the URL import transform for ordinary dynamic importsverifies zero handler calls for uncommented ordinary imports. -
parserLanguageForModuleIdmatches themiddleware-export-validation.tspattern; the raw-ts/tsxit.eachcases guard the4ab0128parse-by-language fix. -
Fixture is a genuine end-to-end test.
pages/api/pages-react-conditions.tscarries the realimport(new URL("./react-conditions.css", import.meta.url).href), and thecloudflare-encoded-pathsPlaywright project buildscf-app-basicand runs it under Wrangler, so the fix is exercised against Workers. The.csssibling inpages/api/won't be misread as an API route — the pages/api scanner filters throughcreateValidFileMatcher(pageExtensions), which excludes.css(confirmed inrouting/pages-router.ts). -
Compatibility reclassification is consistent. Moving
react-versionfromVITE_RUNTIME_CONDITIONS(needs-vite-equivalent) to supported reducesneeds-vite-equivalent3→2 andNON_SUPPORTED_SUITES33→32; the asserted totals match.
Non-blocking observations (already raised in prior reviews)
-
No env/scope guard on the new plugin. Unlike its sibling
createIgnoreDynamicRequestsPlugin(which viashouldTransformVeryDynamicRequestsrestricts the client consumer tonode_modules, and skips vinext source +@vitejs/plugin-rsc+ build output), the new plugin runs on all JS/TS user modules in every environment. I independently greppedpackages/vinext/srcandnode_modules/@vitejs/plugin-rsc— no source uses theimport(new URL(...).href)shape, so nothing new gets rewritten there in practice. Rewriting user client code to a bundledimport("./x")is consistent with the intended semantics, so this is fine — flagging only so the uniform behavior is a conscious choice. -
No transform cache. Both sibling transforms cache (
ignore-dynamic-requestsusescreateTransformCache;import-meta-urlhas its own). The new plugin re-parses on every matching module. The coarse filter keeps the matching set small (only modules withimport(immediately followed bynew URL(or a comment), so impact is minimal, but reusingcreateTransformCachewould align it with the repo's hot-path guidance. -
Documented URL→bundled-specifier semantic shift.
import(new URL(...).href)resolves a runtime URL string in Next.js, whereasimport("./x")goes through the bundler graph. That's the intended behavior for a bundled Workers deployment and the code comment (lines 267-274) explains it well. -
Brittle hardcoded compatibility counts in
tests/compatibility-support.test.ts(33→32,3→2) — a standing maintenance cost, not a change for this PR.
Validation performed locally
vp test run tests/import-meta-url.test.ts tests/compatibility-support.test.ts→ 62 passed- Out-of-band edge-case probe of
rewriteDynamicImportUrls→ 11/11 behave conservatively - Adversarial ReDoS stress of the production filter → linear (<8ms on 2M-char inputs)
vp checkonimport-meta-url.ts+index.ts→ clean
LGTM.
|
Review posted to PR #2893. SummaryI independently reviewed PR #2893 ( What the PR does: Adds an Key things I verified myself (not just trusting prior reviews):
Local validation: 62 tests pass, edge probe 11/11, ReDoS stress linear, Non-blocking notes (already raised in prior reviews): missing env/scope guard vs. the sibling plugin, no transform cache, the documented URL→bundled semantic shift, and brittle hardcoded compatibility counts. LGTM. |
Addressed on 08819bc; fresh Big Bonk run 31465181133 reports no findings.
2 participants
Summary
import(new URL("./module", import.meta.url).href)dependencies before vinext's generic dynamic-request fallbackreact-versionsuite as supported now that all assertions run and passRoot cause
The generic dynamic-request transform ran before Vite could recognize the literal module URL. It replaced the import with vinext's deterministic
MODULE_NOT_FOUNDfallback, so the Next.js suite failed during prerender before any React-condition assertion ran.Validation
vp test run tests/import-meta-url.test.ts tests/compatibility-support.test.ts(50 passed)PLAYWRIGHT_PROJECT=cloudflare-encoded-paths pnpm run test:e2e -- react-conditions.spec.ts(5 passed against Wrangler)REPO="$(pwd)" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" NEXT_TEST_CONCURRENCY=1 ./scripts/run-targeted-nextjs-e2e.sh test/e2e/react-version/react-version.test.ts(5 passed)vp check(clean)vp run knip(clean)Refs the non-cache failures from Actions run 31439707085.