fix(build): inline new URL(asset, import.meta.url) blob imports for edge (#1824)#1833
fix(build): inline new URL(asset, import.meta.url) blob imports for edge (#1824)#1833james-elicx wants to merge 2 commits into
Conversation
…dge (#1824) Edge/worker routes that reference static assets via `new URL("./asset", import.meta.url)` and `fetch(url)` failed at runtime. Vite's built-in `vite:asset-import-meta-url` plugin only runs in the `client` environment, so the URL was left untransformed in the worker bundle; and on Cloudflare Workers `import.meta.url` is the literal string `"worker"` (not a URL), so `new URL(...)` throws `TypeError: Invalid URL`. The whole upstream `edge-compiler-can-import-blob-assets` suite (5 tests) was red. Adds `vinext:edge-asset-import-meta-url`, which rewrites the expression to an inline `data:` URL computed from the referenced file at build time. A data URL is a valid absolute URL (no dependency on the runtime value of `import.meta.url`), can be bound to a variable and `fetch()`ed later, and is fetchable in both workerd and Node. Bare specifiers (node_modules assets like `my-pkg/hello/world.json`) resolve through the bundler. Mirrors the existing `vinext:og-inline-fetch-assets` plugin, which already base64-inlines `fetch(new URL(...))` assets for the same reason, and runs immediately after it so the OG inliner's verbatim-pattern match is preserved. Complements the still-open SSR work in #1346/#1640, which takes a Node-only `file://` emit approach that does not work on Workers. Closes #1824
|
/bigbonk review for issues |
commit: |
|
There was a problem hiding this comment.
Review: fix(build): inline new URL(asset, import.meta.url) blob imports for edge
Solid, well-documented fix. It matches the upstream edge-compiler-can-import-blob-assets fixture patterns exactly (relative text/binary assets + bare-specifier node_modules asset, leaving absolute/remote URLs alone), vp check is clean, and the new test plus tests/og-assets.test.ts / tests/import-meta-url.test.ts are green. The enforce: "pre" ordering relative to vinext:og-inline-fetch-assets is correct, and since the existing vinext:import-meta-url plugin is enforce: "post", this pre plugin runs ahead of it and rewrites the new URL(...) before import.meta.url would be normalized — no conflict there.
A few things worth considering before merge:
1. Plugin runs in the Node SSR environment too, not just the edge/worker — possible regression (medium)
applyToEnvironment admits every non-client environment (consumer !== "client"), so this also runs in the plain Node SSR build path (index.ts:2216, shouldInjectPlainPagesEnvironments, consumer: "server"). On Node, import.meta.url is a valid file:// URL, so new URL("./asset", import.meta.url) already worked natively there.
For the fetch(url) use case the data-URL rewrite is harmless (the body's "fetchable in both workerd and Node" claim holds). But the rewrite changes the semantics of the resulting URL object for any code that does not just fetch it:
fileURLToPath(new URL("./x", import.meta.url))throws on Node after the rewrite:fileURLToPath("data:...")→The URL must be of scheme file.new URL("./x", import.meta.url).pathnamereturns the data-URL body (text/plain;base64,...) instead of a filesystem-like path.
The vinext:og-inline-fetch-assets plugin handles the verbatim readFileSync(fileURLToPath(new URL(...))) shape and runs first, so the common single-expression form is covered. But the split form — const u = new URL("./x", import.meta.url); readFileSync(fileURLToPath(u)) — is not matched by the OG inliner, so this plugin would rewrite the standalone new URL and break a previously-working Node SSR module.
The PR body frames the Node SSR path (PR #1640) as "complementary, no overlap on main," but by running in all non-client environments this plugin does affect Node SSR behavior today. Consider scoping it to the edge/worker environment (or gating on the edge runtime) so the Node SSR path keeps the native file:// URL. If the broad scope is intentional, it's worth a one-line note in the plugin doc explaining why Node SSR is deliberately included.
2. No end-to-end / build test — only the transform hook is exercised (medium)
The new test calls the transform handler directly with a hand-rolled this context (makeCtx), which bypasses applyToEnvironment, the transform.filter, and the real enforce: "pre" ordering against vinext:og-inline-fetch-assets and vinext:import-meta-url. The PR body notes a throwaway harness verified a real edge build but it was removed.
Given the ordering between three pre/post plugins is the most fragile part of this change (and the explicit motivation for the array placement), an integration test that builds a small Pages-Router edge API fixture using the upstream edge.js shape and asserts the worker output contains the inlined data: URL with no residual import.meta.url would protect against silent ordering regressions. A fixture under tests/fixtures/pages-basic/pages/api/ would be a natural home.
3. Relative specifiers with a query/hash are not stripped before resolving (low)
new URL("./a.png?inline", import.meta.url) captures spec = "./a.png?inline", and the relative branch does path.resolve(moduleDir, spec) without stripping the query, so readFile misses and the expression is (gracefully) left untouched. The bare-specifier branch already strips the query via resolved?.id?.split("?")[0] (edge-asset-import-meta-url.ts:140). For consistency and to actually inline query-suffixed relative assets, strip ?/# from spec in the relative branch too (edge-asset-import-meta-url.ts:134-135). Vite's own asset plugin strips queries here. Low severity since it degrades safely.
4. Minor parity gap: import.meta?.url not matched (low)
ASSET_IMPORT_META_URL_RE (edge-asset-import-meta-url.ts:67) matches import.meta.url but not the optional-chained import.meta?.url. The existing vinext:import-meta-url plugin does handle the chained form (import-meta-url.ts:544-549). Unlikely in real code; noting only for completeness.
Nits
- The
transform.filterand the iteration regex are the same source pattern, with a fresh/gcopy built per call — correct, and the comment explaining thelastIndexstatefulness is appreciated. - The doc claims a trailing
.href/.pathnameaccessor is "preserved." Syntactically true (the member access stays valid), but.pathnameon the resulting data URL is meaningless (see #1). Worth a half-sentence clarifying that only.href/fetchconsumers behave as before.
|
Review posted to PR #1833. SummaryI reviewed the edge I raised four issues:
Plus nits on the |
Bonk review follow-ups on #1833: - Scope to the bundled worker build (Cloudflare/Nitro) only. The plugin now takes an `isWorkerTarget` getter and `applyToEnvironment` returns false in a plain Node SSR build, where `import.meta.url` is already a valid file:// URL and rewriting to a data: URL would break `fileURLToPath(new URL(...))` and `.pathname` for the split `const u = new URL(...); readFileSync(...)` form that the OG inliner does not catch. - Add an end-to-end build test that runs a real Pages Router edge-API route through the full Vite pipeline (filter + applyToEnvironment + plugin ordering), plus a negative test asserting no rewrite in a plain Node SSR build. - Match the optional-chained `import.meta?.url` form (parity with the existing vinext:import-meta-url plugin). - Strip `?query`/`#hash` from relative specifiers before resolving, matching the bare-specifier branch. - Fix the doc nit about trailing accessors.
|
Thanks for the review — addressed all four findings:
Also fixed the trailing-accessor doc nit. /bigbonk review for issues |
Re-review: all four findings addressedRe-reviewed at 1. Node SSR scope ✅
This is the right discriminator: in a Cloudflare/Nitro build the rsc/ssr environments are the worker bundle (everything bundled, 2. End-to-end test ✅The new 3. Query/hash specifiers ✅The relative branch now strips 4.
|
|
Posted the re-review to PR #1833. Summary: All four findings (plus the doc nit) are correctly addressed in commit
Verification: 10 tests pass (incl. both e2e build cases), |
Summary
Edge/worker routes that reference static assets via
new URL("./asset", import.meta.url)andfetch(url)failed at runtime. Vite's built-invite:asset-import-meta-urlplugin only runs in the client environment, so the URL was left untransformed in the worker bundle. Worse, on Cloudflare Workersimport.meta.urlis the literal string"worker"(not a URL), sonew URL("./x", import.meta.url)throwsTypeError: Invalid URL. The whole upstreamedge-compiler-can-import-blob-assetssuite (5 tests) was red.Adds a
vinext:edge-asset-import-meta-urlVite plugin that, in non-client (server/worker) environments, rewrites the entirenew URL("<spec>", import.meta.url)expression to an inlinedata:URL literal computed from the referenced file at build time.A
data:URL:new URL(...)never throws and never depends on the runtime value ofimport.meta.url;fetch()ed later — matching the fixture'sconst url = new URL(...); return fetch(url)shape, which thefetch(new URL(...))-only OG inliner does not cover;Bare specifiers (node_modules assets like
my-pkg/hello/world.json) resolve through the bundler'sthis.resolve. Absolute/remote URLs (new URL("https://..."),new URL("/", "https://...")) and/* @vite-ignore */-annotated specifiers are left untouched.This mirrors the existing
vinext:og-inline-fetch-assetsplugin, which already base64-inlinesfetch(new URL(...))font/wasm assets for the same "import.meta.urlis not a URL in workerd" reason. The new plugin is registered immediately after the OG inliner (both areenforce: "pre", so array order sequences them) so the OG inliner's verbatim-pattern match is preserved.Relation to #1346 / #1640
PR #1640 (still open, for #1346) targets the Node SSR path: it emits the asset to disk and rewrites the URL to a chunk-relative
file://path. That strategy does not work on Cloudflare Workers (no filesystem;import.meta.urlis"worker"), which is why the edge path inlines instead. The two are complementary; #1640 is not merged, so there is no overlap onmain.Test plan
tests/edge-asset-import-meta-url.test.tsdrives the plugin'stransformhook with the exact patterns from the upstreamedge.jsfixture (relative text asset, binary image with correct mime, bare-specifier node_modules asset) and asserts each is rewritten to the expecteddata:URL; also asserts absolute/remote URLs and missing files are left untouched, and that the plugin does not run in the client environment.new URL(\data:text/plain;base64,...`)with no residualimport.meta.url`.tests/og-assets.test.tsandtests/import-meta-url.test.tsstill green (no regression from plugin ordering).vp checkclean on all changed files.Closes #1824