fix(build): support fetched import-meta assets - #2884
Conversation
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
/bigbonk review — PR #2884 (fix(build): support fetched import-meta assets)
Reviewed the new vinext:asset-import-meta-url plugin, its wiring in index.ts, and all four test surfaces (unit transform, prod integration, dev invalidation, Nitro Node/Cloudflare presets, plus the Workerd E2E spec). Ran the targeted suites locally:
tests/asset-import-meta-url.test.ts— 14/14 passtests/edge-blob-assets.test.ts— 5/5 pass (incl. Node preset +cloudflare-modulepreset)vp checkon the new source + test files — clean (format, lint, types)
Overall: solid. The design is coherent and the coverage is unusually thorough for this class of change. No correctness bugs found. A few observations below, none blocking.
Strengths
- Correct dev/prod/Nitro parity. The second commit's refinement of
isWorkerTargetfromhasNitroPlugin→hasNodelessNitroTarget(nitro.options.node === false) is the right call: it preserves realfile:URL semantics (.pathname,fileURLToPath()) for plain Node and default Nitro output, and only does the constructor-wide data-URL rewrite for nodeless targets whereimport.meta.urlisn't a usable base. The Nitro Node-preset test assertsprotocol: "file:"and a realfilePath, which locks this in. - Proper scope analysis. Reusing
ast-scope/ast-utils(functions, blocks, loops, switch, catch, static blocks) and theURL/fetchshadowing guards is much more robust than a regex. Theconst-only binding restriction for the bound-fetch path is a sound safety choice (a reassignablelet/varcouldn't be trusted). - Watch registration.
this.addWatchFile(file)for the embedded (non-graph) asset correctly closes the dev-invalidation gap, and the dev test exercises it via a realwatcher.emit("change", ...). - Containment guard. Routing relative and bare specifiers through
OgAssetOwnership.resolveContainedAsset(with the "does not read outside the owning project" test) gives good defense-in-depth against path traversal. - Plugin ordering. Keeping this after
createOgInlineFetchAssetsPluginis correct — the OG plugin's narrowerfetch(...).then(arrayBuffer())IIFE shape is preserved, and this plugin handles the plainfetch(new URL(...))remainder. They're complementary, not overlapping.
Non-blocking observations
-
isWorkerTargettiming depends on Nitro'ssetuprunning beforetransform.hasNodelessNitroTargetis assigned inside the Nitrosetuphook and read lazily at transform time. Thecloudflare-moduletest confirms the ordering holds for production builds, so this is fine in practice — but it is an implicit lifecycle dependency. Worth a one-line comment at the closure (isWorkerTarget: () => ...) noting thathasNodelessNitroTargetis populated by the Nitrosetuphook, so the two must not be reordered. -
addWatchFileruns before the read can fail.this.addWatchFile(file)(line 337) is called beforefs.promises.readFile(line 343). If the read throws wecontinue, but the file is already watched. Harmless (watching a transiently-unreadable path is a no-op consequence), just slightly out of order — could move it after a successful read if you want strictness. -
Node-mode
let/var-bound fetch inputs are intentionally not rewritten. Onlyconstbindings feed the bound-fetch path, solet url = new URL(...); fetch(url)stays a runtimefile:fetch (which fails on Node's fetch). This is stricter than necessary but a defensible safety choice and still strictly more capable than the prior OG-only handling. Flagging only so it's a conscious decision — no change requested.
Nice work — the parity testing across Node/Nitro/Workerd is exactly the bar this kind of build transform should clear.
|
Review posted to PR #2884. SummaryI reviewed PR #2884 (
Verdict: solid, no blocking issues, no correctness bugs found. Key positives:
Three non-blocking observations flagged in the review:
|
|
CI status note: App Router shard 3/3 has been rerun three times and each run hard-failed only the out-of-scope The product-specific gates for this PR are clean: targeted Next.js parity 5/5, focused Node/Nitro/Worker/watcher coverage, real Wrangler/Workerd execution, two independent reviews, and Big Bonk with no blocking findings. Keeping the PR draft because the required CI shard is still red; no claim of fully green CI. |
|
/bigbonk review for issues |
|
Independent exact-head re-review of Reviewed the committed shared Independent validation:
Fresh CI and paired performance results were still running when this review was posted. |
There was a problem hiding this comment.
/bigbonk review — PR #2884 (fix(build): support fetched import-meta assets)
Reviewed the new vinext:asset-import-meta-url plugin, the index.ts wiring (shared OgAssetOwnership, hasNodelessNitroTarget), the og-assets.ts ownership-injection change, and all four test surfaces. Ran the targeted suites locally on the head branch:
tests/asset-import-meta-url.test.ts— 15/15 passtests/edge-blob-assets.test.ts— 5/5 pass (Node preset +cloudflare-modulepreset, real prod server)
Verdict: solid, no blocking correctness issues found. My independent pass reached the same conclusion as the prior automated review; I confirmed a few things it did not spell out.
Confirmed correctness properties
- No double-inlining vs the OG plugin. The OG transform runs first and rewrites its narrow
fetch(new URL(...)).then(r => r.arrayBuffer())shape into an IIFE that no longer containsnew URL(...), so this plugin cannot re-match it. Where the OG regex fails to match (reformatted source), this plugin still rewrites the innernew URLto a data URL in worker mode — a strict improvement, not a conflict. - No overlapping
MagicStringoverwrites. Worker mode returns early after the firstNewExpressionmatch (no child recursion), and the Node bound-fetch path rewrites the reference identifier (range = input) while leaving theconstdeclaration intact, so sibling/duplicate fetches map to disjoint ranges. - Shared-ownership wiring is single-pass. With a shared
ownership,managesOwnershipis false, so the new plugin drops itsconfigResolved/buildStart/resolveIdhooks and defers configure/reset/import-recording to the OG plugin (asserted by the "shares ownership tracking" test:configure/resetcalled once,plugin.resolveId === undefined). - Lifecycle deps hold.
hasCloudflarePluginis set inconfigResolvedandhasNodelessNitroTargetin the Nitrosetuphook — both before buildtransformruns — so the lazyisWorkerTarget()closure reads populated values. - Guards verified: literal-only first arg, protocol/
//rejection,URL/fetchshadowing (both worker and node),@vite-ignore, andresolveContainedAssetpath-traversal containment.
Non-blocking observations
-
Worker-mode constructor rewrite changes
.pathname/fileURLToPath()semantics. In a nodeless target, everynew URL("./x", import.meta.url)becomes adata:URL, even one used only for.pathnameorfileURLToPath()rather thanfetch. This is documented as intentional (workerimport.meta.urlis not a usable base, so the original would throw anyway) and matches the Node-mode restriction to the fetch path. Flagging only so the divergence stays a conscious decision — no change requested. -
Data-URL media type contains a space (
data:text/plain; charset=utf-8;base64,...) becausecontentTypeForPath(".txt")is"text/plain; charset=utf-8". It is not strictly RFC 2397 clean, but workerd and Nodefetchboth parse it (the real-Workerd E2E returns 200 with the expected bytes), so it is fine in practice. -
addWatchFilebefore the read that can fail (line 351 vs 357) — harmless, already noted upstream. -
Node-mode
let/var-bound fetch inputs are intentionally not rewritten — deliberate safety choice for reassignable bindings; still strictly more capable than the prior OG-only handling.
Nice work — the Node/default-Nitro/nodeless-Nitro/Workerd/dev-watcher parity coverage is exactly the bar this class of build transform should clear.
|
Review posted to PR #2884. SummaryI reviewed PR #2884 (
Verdict: solid, no blocking correctness bugs. Beyond the prior automated review, I independently verified:
Four non-blocking observations flagged: worker-mode |
1 participant
Summary
new URL(..., import.meta.url)when they are fetched by server codeFailing job
This addresses the three local-asset failures in run 31439707085, job 93624401572:
allows to fetch text assetsallows to fetch image assetsallows to assets from node_modulesThe two remote-URL controls in that suite already passed and remain unchanged.
Validation
test/e2e/edge-compiler-can-import-blob-assets/index.test.ts— 5/5 passed, retry 0/0fileURLToPath()and.pathnamepreservationcloudflare-modulebuild/runtime passed with constructor-wide asset rewriting/api/blob-assetreturned 200 with the expected bytes