diff --git a/deno.json b/deno.json index 424ca605e3..813b66a779 100644 --- a/deno.json +++ b/deno.json @@ -1,6 +1,6 @@ { "name": "veryfront", - "version": "0.1.1102", + "version": "0.1.1103", "license": "Apache-2.0", "nodeModulesDir": "auto", "minimumDependencyAge": { diff --git a/docs/api-reference/veryfront/index.md b/docs/api-reference/veryfront/index.md index 4c0044486d..9a5fc3c049 100644 --- a/docs/api-reference/veryfront/index.md +++ b/docs/api-reference/veryfront/index.md @@ -79,11 +79,11 @@ export function getServerData(ctx: DataContext) { | `mergeConfigs` | Merge multiple partial Veryfront configuration objects into one config object. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/config/define-config.ts#L19) | | `mergeConfigs` | | [source](https://github.com/veryfront/veryfront-code/blob/main/src/config/define-config.ts#L20) | | `mergeConfigs` | | [source](https://github.com/veryfront/veryfront-code/blob/main/src/config/define-config.ts#L21) | -| `notFound` | Return a 404 result from a data loader. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/data/helpers.ts#L9) | +| `notFound` | Render the 404 page from a data loader. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/data/helpers.ts#L45) | | `parseFormData` | Parse and validate multipart or URL-encoded form data. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/security/input-validation/parsers.ts#L48) | | `parseJsonBody` | Parse and validate a JSON request body. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/security/input-validation/parsers.ts#L12) | | `parseQueryParams` | Parse and validate query parameters from a request URL. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/security/input-validation/parsers.ts#L81) | -| `redirect` | Return a redirect result from a data loader. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/data/helpers.ts#L4) | +| `redirect` | Redirect the request from a data loader. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/data/helpers.ts#L34) | | `sanitizeData` | Sanitize data to prevent XSS and prototype pollution attacks. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/security/input-validation/sanitizers.ts#L2) | | `serverError` | Create a 500 Internal Server Error response. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/platform/compat/http/responses.ts#L134) | | `startServer` | Start a Veryfront server in development or production mode. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/server/index.ts#L331) | diff --git a/docs/guides/api-routes.md b/docs/guides/api-routes.md index b648bd726a..fd8f41998c 100644 --- a/docs/guides/api-routes.md +++ b/docs/guides/api-routes.md @@ -25,9 +25,7 @@ export function GET() { } ``` -Use `pages/api/**` in the pages router. Export named HTTP method handlers or a `default` fallback handler. Each handler receives an `APIContext` as `ctx`; use `ctx.request` for the raw request, `ctx.params` for route params, and `ctx.query` for query parameters. - -The `ctx.json` helper is intentionally overloaded by arity: `ctx.json()` reads the request body as JSON, while `ctx.json(data, init?)` returns a JSON `Response`. Use `ctx.request` directly when you need lower-level request APIs such as streaming, form data, or text parsing. +Use `pages/api/**` in the pages router. Export named HTTP method handlers or a `default` fallback handler. Each handler receives an `APIContext` as `ctx`; use `ctx.request` for the raw request, `ctx.params` for route params, `ctx.query` for query parameters, and `ctx.json(data)` or `Response.json(data)` to return JSON. To read a posted JSON body, use `ctx.body()`, which parses it once and answers a 400 if it is malformed. ```ts // pages/api/hello.ts @@ -36,6 +34,11 @@ import type { APIContext } from "veryfront"; export function GET(ctx: APIContext) { return ctx.json({ message: "Hello, world!" }); } + +export async function POST(ctx: APIContext) { + const { name } = await ctx.body<{ name: string }>(); + return ctx.json({ message: `Hello, ${name}!` }); +} ``` ## Basic route @@ -99,13 +102,13 @@ export async function GET(ctx: APIContext) { } export async function POST(ctx: APIContext) { - const body = await ctx.json(); + const body = await ctx.body>(); const user = { id: "user_456", ...body }; return ctx.json(user, { status: 201 }); } export async function DELETE(ctx: APIContext) { - const { id } = await ctx.json() as { id?: string }; + const { id } = await ctx.body<{ id?: string }>(); if (!id) return ctx.json({ error: "Missing id" }, { status: 400 }); return new Response(null, { status: 204 }); } diff --git a/docs/guides/data-fetching.md b/docs/guides/data-fetching.md index a5cbb7c1e0..ec93d22e0e 100644 --- a/docs/guides/data-fetching.md +++ b/docs/guides/data-fetching.md @@ -93,6 +93,27 @@ export async function getServerData({ params }: DataContext) { redirect("/new-url", true); // 301 permanent redirect ``` +Throwing works the same way. `throw notFound()` and `throw redirect(...)` behave exactly like returning them, which is useful inside a helper that has no clean way to return to the data function: + +```tsx +import { type DataContext, notFound } from "veryfront"; + +const posts = [{ slug: "hello", title: "Hello" }]; + +function requirePost(slug: string) { + const post = posts.find((item) => item.slug === slug); + if (!post) throw notFound(); + + return post; +} + +export function getServerData({ params }: DataContext) { + return { props: { post: requirePost(String(params.slug)) } }; +} +``` + +Only the objects `notFound()` and `redirect()` produce are read as control flow. Every other thrown value is an error, including an object that happens to carry a `notFound` property, such as a parsed error body from an upstream API. + ## Client-side fetching For data that loads after the page renders, fetch in a client component: diff --git a/src/data/helpers.test.ts b/src/data/helpers.test.ts index 8caaa67526..cc8e8f566d 100644 --- a/src/data/helpers.test.ts +++ b/src/data/helpers.test.ts @@ -116,5 +116,49 @@ describe("helpers.ts", () => { it("rejects notFound: false", () => { assertEquals(isDataControlResult({ notFound: false }), false); }); + + // A loader that does `throw await res.json()` against an upstream returning + // `{ notFound: true, message: "record locked" }` must reach the error + // handler. Reading it as a 404 renders the wrong page, records a circuit + // breaker success, skips the log, and caches the bogus 404. + it("rejects an unbranded object that only looks like notFound()", () => { + assertEquals(isDataControlResult({ notFound: true }), false); + assertEquals( + isDataControlResult({ notFound: true, message: "record locked", requestId: "abc" }), + false, + ); + }); + + it("rejects an unbranded object that only looks like redirect()", () => { + assertEquals(isDataControlResult({ redirect: { destination: "/login" } }), false); + assertEquals( + isDataControlResult({ redirect: { destination: "/login", permanent: true } }), + false, + ); + }); + + it("recognises a control result rebuilt from the same public brand", () => { + // Project code runs against its own copy of the helpers. The brand is a + // registered symbol so it matches across module instances and realms. + const rebuilt = { notFound: true }; + Object.defineProperty(rebuilt, Symbol.for("veryfront.dataControlResult"), { value: true }); + + assertEquals(isDataControlResult(rebuilt), true); + }); + }); + + describe("returned-result contract", () => { + it("keeps the brand off the serialized shape", () => { + assertEquals(JSON.stringify(notFound()), '{"notFound":true}'); + assertEquals( + JSON.stringify(redirect("/login")), + '{"redirect":{"destination":"/login","permanent":false}}', + ); + }); + + it("keeps the brand off enumerable keys", () => { + assertEquals(Object.keys(notFound()), ["notFound"]); + assertEquals(Object.keys(redirect("/login")), ["redirect"]); + }); }); }); diff --git a/src/data/helpers.ts b/src/data/helpers.ts index feebc8b696..058a2c6e7d 100644 --- a/src/data/helpers.ts +++ b/src/data/helpers.ts @@ -1,13 +1,49 @@ import type { DataResult } from "./types.ts"; -/** Return a redirect result from a data loader. */ +/** + * Brand marking an object as produced by {@link notFound} or {@link redirect}. + * + * A registered symbol, so a result built by one copy of this module is + * recognised by another. Project code and the framework do not always share a + * module instance, and isolated data fetching crosses a realm boundary. + * + * Symbols are dropped by `structuredClone`, so the brand does not survive + * `postMessage`. Worker-side code normalises a thrown control result before it + * is posted back, while the object is still in-realm. + */ +const DATA_CONTROL_RESULT = Symbol.for("veryfront.dataControlResult"); + +/** + * Mark a result as framework-produced control flow. + * + * The brand is non-enumerable, so it stays out of `Object.keys`, + * `JSON.stringify`, and the `DataResult` schema. A returned control result + * behaves exactly as it did before the brand existed. + */ +function brandDataControlResult(result: DataResult): DataResult { + Object.defineProperty(result, DATA_CONTROL_RESULT, { value: true }); + return result; +} + +/** + * Redirect the request from a data loader. + * + * Return it or throw it. `throw redirect("/login")` behaves exactly like + * `return redirect("/login")`. + */ export function redirect(destination: string, permanent = false): DataResult { - return { redirect: { destination, permanent } }; + return brandDataControlResult({ redirect: { destination, permanent } }); } -/** Return a 404 result from a data loader. */ +/** + * Render the 404 page from a data loader. + * + * Return it or throw it. `throw notFound()` behaves exactly like + * `return notFound()`, which is useful deep inside a helper that has no clean + * way to return to the loader. + */ export function notFound(): DataResult { - return { notFound: true }; + return brandDataControlResult({ notFound: true }); } /** @@ -18,19 +54,19 @@ export function notFound(): DataResult { * naturally and is what people coming from other frameworks reach for. Thrown, * the plain object is not an `Error`, so the SSR error handler stringified it * to `[object Object]` and returned a 500 instead of the intended 404 or - * redirect. Recognising the shape lets a thrown result behave like a returned + * redirect. Recognising the brand lets a thrown result behave like a returned * one. + * + * The check is on the brand, never on the shape. A loader that does + * `throw await response.json()` against an upstream answering + * `{ notFound: true, message: "record locked" }` is reporting a failure, and + * reading that as a 404 would render the wrong page, log nothing, and cache a + * 404 the site never asked for. */ export function isDataControlResult(value: unknown): value is DataResult { if (value === null || typeof value !== "object") return false; - if (value instanceof Error) return false; - - const candidate = value as { notFound?: unknown; redirect?: unknown }; - - if (candidate.notFound === true) return true; - const destination = (candidate.redirect as { destination?: unknown } | undefined)?.destination; - return typeof destination === "string"; + return (value as Record)[DATA_CONTROL_RESULT] === true; } /** diff --git a/src/data/server-data-fetcher.test.ts b/src/data/server-data-fetcher.test.ts index 82a239a546..1b589184ca 100644 --- a/src/data/server-data-fetcher.test.ts +++ b/src/data/server-data-fetcher.test.ts @@ -2,9 +2,11 @@ import "#veryfront/schemas/_test-setup.ts"; import { assertEquals, assertExists, assertRejects } from "#veryfront/testing/assert.ts"; import { afterEach, describe, it } from "#veryfront/testing/bdd.ts"; import { ServerDataFetcher } from "./server-data-fetcher.ts"; -import type { DataContext, PageWithData } from "./types.ts"; +import type { DataContext, DataResult, PageWithData } from "./types.ts"; import { notFound, redirect } from "./helpers.ts"; import { __resetPoolForTests } from "#veryfront/security/sandbox/worker-pool.ts"; +import { runWithExactSourceIntegrationPolicy } from "#veryfront/integrations/source-policy-context.ts"; +import { join } from "node:path"; describe("ServerDataFetcher", () => { function createContext(overrides: Partial = {}): DataContext { @@ -445,6 +447,120 @@ describe("ServerDataFetcher", () => { } }); + // Worker isolation is the configuration operators are told to use for + // untrusted project code, so the in-process path alone is not enough. A + // control result thrown inside the worker is a plain object, and the worker + // error path serialized it with String(), producing "[object Object]" and a + // 500 on the host. + describe("under worker isolation", () => { + let projectDir: string | null = null; + + afterEach(async () => { + try { + Deno.env.delete("WORKER_ISOLATION_ENABLED"); + } catch { /* ok */ } + try { + Deno.env.delete("WORKER_ISOLATION_DATA"); + } catch { /* ok */ } + __resetPoolForTests(); + + if (projectDir) { + await Deno.remove(projectDir, { recursive: true }).catch(() => {}); + projectDir = null; + } + }); + + async function writeIsolatedPage(source: string): Promise< + { modulePath: string; projectDir: string } + > { + const dir = await Deno.realPath(await Deno.makeTempDir({ prefix: "vf-isolated-data-" })); + projectDir = dir; + const modulePath = join(dir, "page.ts"); + await Deno.writeTextFile(modulePath, source); + + Deno.env.set("WORKER_ISOLATION_ENABLED", "1"); + Deno.env.set("WORKER_ISOLATION_DATA", "1"); + __resetPoolForTests(); + + return { modulePath, projectDir: dir }; + } + + // The worker cannot import the framework helpers: its read permission is + // scoped to the project directory. `notFound()` brands its result with a + // registered symbol precisely so a result built anywhere is recognised + // everywhere, so the fixture rebuilds the same public brand. + const BRAND_SOURCE = + `Object.defineProperty(result, Symbol.for("veryfront.dataControlResult"), { value: true });`; + + function isolatedFetch( + modulePath: string, + dir: string, + ): Promise { + const fetcher = new ServerDataFetcher(); + const pageModule: PageWithData = { + default: () => null, + getServerData: () => ({ props: {} }), + }; + + return runWithExactSourceIntegrationPolicy( + { schemaVersion: 1, mode: "unrestricted" }, + () => + fetcher.fetch(pageModule, createContext(), { + modulePath, + projectDir: dir, + }), + ); + } + + it("treats a thrown notFound() as a 404 result", async () => { + const { modulePath, projectDir: dir } = await writeIsolatedPage( + `export function getServerData() { + const result = { notFound: true }; + ${BRAND_SOURCE} + throw result; + } + export default function Page() { return null; }`, + ); + + const result = await isolatedFetch(modulePath, dir); + + assertEquals(result.notFound, true); + assertEquals(result.redirect, undefined); + }); + + it("treats a thrown redirect() as a redirect result", async () => { + const { modulePath, projectDir: dir } = await writeIsolatedPage( + `export function getServerData() { + const result = { redirect: { destination: "/login", permanent: true } }; + ${BRAND_SOURCE} + throw result; + } + export default function Page() { return null; }`, + ); + + const result = await isolatedFetch(modulePath, dir); + + assertEquals(result.redirect?.destination, "/login"); + assertEquals(result.redirect?.permanent, true); + assertEquals(result.notFound, undefined); + }); + + it("still propagates a genuine Error thrown in the worker", async () => { + const { modulePath, projectDir: dir } = await writeIsolatedPage( + `export function getServerData() { + throw new Error("intentional test error from isolated getServerData"); + } + export default function Page() { return null; }`, + ); + + await assertRejects( + () => isolatedFetch(modulePath, dir), + Error, + "intentional test error from isolated getServerData", + ); + }); + }); + it("still opens the circuit breaker on repeated genuine errors", async () => { const fetcher = new ServerDataFetcher(); const context = createContext({ diff --git a/src/data/static-data-fetcher.test.ts b/src/data/static-data-fetcher.test.ts index 9ddfd8211c..1d257bf3db 100644 --- a/src/data/static-data-fetcher.test.ts +++ b/src/data/static-data-fetcher.test.ts @@ -341,6 +341,118 @@ describe("StaticDataFetcher", () => { }); }); + describe("background revalidation", () => { + async function settleRevalidation(): Promise { + for (let i = 0; i < 20; i++) { + await new Promise((resolve) => setTimeout(resolve, 1)); + } + } + + // A background revalidation must never replace a live page with a control + // result. The entry would be stored with `revalidate: undefined`, which + // never qualifies for revalidation again, so every request served a 404 + // until the entry aged out of the cache. + it("keeps the cached page when a revalidation throws notFound()", async () => { + await withProductionContext(async () => { + const { cache, fetcher } = createFetcher(); + + const pageModule: PageWithData<{ version: number }> = { + default: () => null, + getStaticData: () => { + throw notFound(); + }, + }; + + const context = createContext({ url: new URL("http://localhost/isr-not-found") }); + const cacheKey = cache.createCacheKey(context); + assertExists(cacheKey); + + cache.set(cacheKey, { + data: { props: { version: 1 }, revalidate: 0 }, + timestamp: Date.now() - 10_000, + revalidate: 0, + }); + + const served = await fetcher.fetch(pageModule, context); + assertEquals((served.props as { version: number }).version, 1); + + await settleRevalidation(); + + const entry = cache.get(cacheKey); + assertExists(entry); + assertEquals((entry.data.props as { version: number }).version, 1); + assertEquals(entry.data.notFound, undefined); + // Still a number, so the entry stays eligible for the next revalidation. + assertEquals(entry.revalidate, 0); + + const next = await fetcher.fetch(pageModule, context); + assertEquals((next.props as { version: number }).version, 1); + assertEquals(next.notFound, undefined); + }); + }); + + it("keeps the cached page when a revalidation throws redirect()", async () => { + await withProductionContext(async () => { + const { cache, fetcher } = createFetcher(); + + const pageModule: PageWithData<{ version: number }> = { + default: () => null, + getStaticData: () => { + throw redirect("/login"); + }, + }; + + const context = createContext({ url: new URL("http://localhost/isr-redirect") }); + const cacheKey = cache.createCacheKey(context); + assertExists(cacheKey); + + cache.set(cacheKey, { + data: { props: { version: 1 }, revalidate: 0 }, + timestamp: Date.now() - 10_000, + revalidate: 0, + }); + + await fetcher.fetch(pageModule, context); + await settleRevalidation(); + + const entry = cache.get(cacheKey); + assertExists(entry); + assertEquals((entry.data.props as { version: number }).version, 1); + assertEquals(entry.data.redirect, undefined); + assertEquals(entry.revalidate, 0); + }); + }); + + it("still replaces the cached page on a successful revalidation", async () => { + await withProductionContext(async () => { + const { cache, fetcher } = createFetcher(); + + const pageModule: PageWithData<{ version: number }> = { + default: () => null, + getStaticData: () => ({ props: { version: 2 }, revalidate: 60 }), + }; + + const context = createContext({ url: new URL("http://localhost/isr-success") }); + const cacheKey = cache.createCacheKey(context); + assertExists(cacheKey); + + cache.set(cacheKey, { + data: { props: { version: 1 }, revalidate: 0 }, + timestamp: Date.now() - 10_000, + revalidate: 0, + }); + + await fetcher.fetch(pageModule, context); + await settleRevalidation(); + + const entry = cache.get(cacheKey); + assertExists(entry); + assertEquals((entry.data.props as { version: number }).version, 2); + assertEquals(entry.revalidate, 60); + }); + }); + }); + describe("thrown control results", () => { function throwing(error: unknown): PageWithData { return { @@ -370,6 +482,15 @@ describe("StaticDataFetcher", () => { assertEquals(result.notFound, true); }); + it("treats a thrown redirect() as a redirect without a cache context", async () => { + const { fetcher } = createFetcher(); + + const result = await fetcher.fetch(throwing(redirect("/login")), createContext()); + + assertEquals(result.redirect?.destination, "/login"); + assertEquals(result.redirect?.permanent, false); + }); + it("treats a thrown redirect() as a redirect with a production cache context", async () => { const { fetcher } = createFetcher(); diff --git a/src/data/static-data-fetcher.ts b/src/data/static-data-fetcher.ts index 080b8f09bf..cc5deb495d 100644 --- a/src/data/static-data-fetcher.ts +++ b/src/data/static-data-fetcher.ts @@ -278,6 +278,25 @@ export class StaticDataFetcher { `getStaticData revalidation for ${pathname}`, ); + // A background revalidation refreshes a page that is already being + // served. A notFound or redirect is not a refreshed page, so keep + // the entry that is live and let the next revalidation try again. + // Storing it would serve a 404 for the previously healthy page, and + // a control result carries no revalidate interval, so the entry + // would never revalidate again either. + if (result.notFound || result.redirect) { + serverLogger.warn( + "DATA_REVALIDATION_CONTROL_RESULT background revalidation returned a control result, keeping the cached entry", + { + pathname, + durationMs: Math.round(performance.now() - start), + cacheKey, + control: result.notFound ? "notFound" : "redirect", + }, + ); + return; + } + this.storeCacheEntry(cacheKey, result); } catch (error) { const durationMs = Math.round(performance.now() - start); diff --git a/src/html/hydration-script-builder/templates/renderer.test.ts b/src/html/hydration-script-builder/templates/renderer.test.ts index 00fbd8a838..704a049fef 100644 --- a/src/html/hydration-script-builder/templates/renderer.test.ts +++ b/src/html/hydration-script-builder/templates/renderer.test.ts @@ -190,26 +190,51 @@ describe("hydration-script-builder/templates/renderer", () => { }); }); - describe("isModuleNotFoundError", () => { - // Evaluate the helper out of the emitted browser script so the behaviour - // itself is under test, not just the presence of a substring. Only the - // helper is extracted — the rest of the script touches `window`. - function helperSource(): string { - const script = getRendererScript(); - const start = script.indexOf("function isModuleNotFoundError(error) {"); - assertEquals(start >= 0, true, "isModuleNotFoundError not found in renderer script"); - const end = script.indexOf("\n }", start); - assertEquals(end > start, true, "could not find end of isModuleNotFoundError"); - return script.slice(start, end + "\n }".length); - } - - function isModuleNotFoundError(error: unknown): boolean { - return new Function( - "error", - `${helperSource()}\nreturn isModuleNotFoundError(error);`, - )(error) as boolean; - } + // Evaluate helpers out of the emitted browser script so the behaviour itself + // is under test, not just the presence of a substring. Only the helpers are + // extracted; the rest of the script touches `window`. + function extractFunction(declaration: string): string { + const script = getRendererScript(); + const start = script.indexOf(declaration); + assertEquals(start >= 0, true, declaration + " not found in renderer script"); + const end = script.indexOf("\n }", start); + assertEquals(end > start, true, "could not find end of " + declaration); + return script.slice(start, end + "\n }".length); + } + + function isModuleNotFoundError(error: unknown): boolean { + return new Function( + "error", + extractFunction("function isModuleNotFoundError(") + + "\nreturn isModuleNotFoundError(error);", + )(error) as boolean; + } + + type ImportModule = (url: string) => Promise; + + function loadPageModuleWithIndexFallback( + basePath: string, + pageSlug: string, + pageModuleError: unknown, + importModule: ImportModule, + ): Promise { + const source = [ + extractFunction("function isModuleNotFoundError("), + extractFunction("function preferReachedModuleError("), + extractFunction("async function loadPageModuleWithIndexFallback("), + ].join("\n"); + + return new Function( + "basePath", + "pageSlug", + "pageModuleError", + "importModule", + source + + "\nreturn loadPageModuleWithIndexFallback(basePath, pageSlug, pageModuleError, importModule);", + )(basePath, pageSlug, pageModuleError, importModule) as Promise; + } + describe("isModuleNotFoundError", () => { it("treats a failed fetch as module-not-found", () => { assertEquals( isModuleNotFoundError( @@ -244,6 +269,19 @@ describe("hydration-script-builder/templates/renderer", () => { assertEquals(isModuleNotFoundError(new ReferenceError("x is not defined")), false); }); + it("does not treat app errors that merely mention loading as module-not-found", () => { + assertEquals(isModuleNotFoundError(new Error("Failed to load user profile")), false); + assertEquals(isModuleNotFoundError(new TypeError("Failed to fetch /api/session")), false); + }); + + it("recognizes the browser wordings for a module that could not be fetched", () => { + assertEquals(isModuleNotFoundError(new TypeError("Importing a module script failed.")), true); + assertEquals( + isModuleNotFoundError(new TypeError("Failed to load module script: unexpected MIME type")), + true, + ); + }); + it("handles null and non-Error values", () => { assertEquals(isModuleNotFoundError(null), false); assertEquals(isModuleNotFoundError(undefined), false); @@ -251,27 +289,161 @@ describe("hydration-script-builder/templates/renderer", () => { }); }); - describe("page module fallback", () => { - it("only retries the /index.js path for module-not-found errors", () => { - const script = getRendererScript(); - assertEquals(script.includes("isModuleNotFoundError(error)"), true); - assertEquals(script.includes("canRetryAsIndex"), true); + describe("loadPageModuleWithIndexFallback", () => { + const notFound = (url: string) => + new TypeError("Failed to fetch dynamically imported module: " + url); + const linkError = () => + new SyntaxError( + "The requested module '/_vf_modules/_veryfront/platform/polyfills/node-noop.js' " + + "does not provide an export named 'createHash'", + ); + + async function captureRejection(promise: Promise): Promise { + try { + await promise; + } catch (error) { + return error; + } + throw new Error("expected the fallback to reject"); + } + + it("loads the module from /index.js when .js is missing", async () => { + const requested: string[] = []; + const pageModule = { default: "docs-index" }; + + const loaded = await loadPageModuleWithIndexFallback( + "http://modules/pages/docs", + "docs", + null, + (url) => { + requested.push(url); + if (url.endsWith("/docs.js")) return Promise.reject(notFound(url)); + return Promise.resolve(pageModule); + }, + ); + + assertEquals(loaded, pageModule); + assertEquals(requested, [ + "http://modules/pages/docs.js", + "http://modules/pages/docs/index.js", + ]); + }); + + it("retries at /index.js for rejections the classifier does not recognize", async () => { + // A proxy that rewrites a module miss into an HTML shell surfaces as a + // SyntaxError. Gating the retry on error wording turned that into a blank + // page for routes that load fine from /index.js. + const requested: string[] = []; + const pageModule = { default: "docs-index" }; + + const loaded = await loadPageModuleWithIndexFallback( + "http://modules/pages/docs", + "docs", + null, + (url) => { + requested.push(url); + if (url.endsWith("/docs.js")) { + return Promise.reject(new SyntaxError("Unexpected token '<'")); + } + return Promise.resolve(pageModule); + }, + ); + + assertEquals(loaded, pageModule); + assertEquals(requested.length, 2); }); - it("rethrows the original error rather than the fallback's", () => { - const script = getRendererScript(); - assertEquals(script.includes("throw pageModuleError"), true); + it("throws the original error when both the route and its index are missing", async () => { + const original = notFound("http://modules/pages/docs.js"); + + const thrown = await captureRejection( + loadPageModuleWithIndexFallback( + "http://modules/pages/docs", + "docs", + null, + (url) => Promise.reject(url.endsWith("/index.js") ? notFound(url) : original), + ), + ); + + assertEquals(thrown, original); }); - it("prefers the retry's error when the retry reached a module", () => { - // A real /index.tsx page 404s on .js first, so the retry is - // the load that matters and its link error must not be replaced by the - // expected 404. - const script = getRendererScript(); - assertEquals( - script.includes("throw isModuleNotFoundError(indexError) ? pageModuleError : indexError;"), - true, + it("throws the link error from .js rather than the retry's 404", async () => { + const original = linkError(); + + const thrown = await captureRejection( + loadPageModuleWithIndexFallback( + "http://modules/pages/docs", + "docs", + null, + (url) => Promise.reject(url.endsWith("/index.js") ? notFound(url) : original), + ), + ); + + assertEquals(thrown, original); + }); + + it("throws the retry's link error when .js was merely missing", async () => { + const indexLinkError = linkError(); + + const thrown = await captureRejection( + loadPageModuleWithIndexFallback( + "http://modules/pages/docs", + "docs", + null, + (url) => Promise.reject(url.endsWith("/index.js") ? indexLinkError : notFound(url)), + ), + ); + + assertEquals(thrown, indexLinkError); + }); + + it("does not retry when the slug is already an index route", async () => { + const requested: string[] = []; + const original = notFound("http://modules/pages/index.js"); + + const thrown = await captureRejection( + loadPageModuleWithIndexFallback("http://modules/pages/index", "index", null, (url) => { + requested.push(url); + return Promise.reject(original); + }), ); + + assertEquals(thrown, original); + assertEquals(requested, ["http://modules/pages/index.js"]); + }); + + it("prefers a link error over a stale hydration-data fetch failure", async () => { + // Hydration data can carry a pagePath that no longer resolves. That 404 + // must never outrank an error proving the fallback reached a module. + const staleFetchFailure = notFound("http://modules/pages/old-name.js"); + const indexLinkError = linkError(); + + const thrown = await captureRejection( + loadPageModuleWithIndexFallback( + "http://modules/pages/index", + "index", + staleFetchFailure, + () => Promise.reject(indexLinkError), + ), + ); + + assertEquals(thrown, indexLinkError); + }); + + it("keeps the hydration-data error when nothing else reached a module", async () => { + const original = notFound("http://modules/pages/old-name.js"); + + const thrown = await captureRejection( + loadPageModuleWithIndexFallback( + "http://modules/pages/docs", + "docs", + original, + (url) => Promise.reject(notFound(url)), + ), + ); + + assertEquals(thrown, original); }); }); }); diff --git a/src/html/hydration-script-builder/templates/renderer.ts b/src/html/hydration-script-builder/templates/renderer.ts index 3b4c0f3f43..94422423e9 100644 --- a/src/html/hydration-script-builder/templates/renderer.ts +++ b/src/html/hydration-script-builder/templates/renderer.ts @@ -3,17 +3,59 @@ export const getRendererScript = () => ` // True when a dynamic import failed because the module could not be // fetched (404 / network), as opposed to being fetched and then failing to - // link or evaluate. Browsers report the former as a TypeError with a - // "dynamically imported module" message; link failures are SyntaxErrors and - // evaluation failures are whatever the module threw. + // link or evaluate. Browsers word this as a TypeError naming the dynamic + // import itself; link failures are SyntaxErrors and evaluation failures are + // whatever the module threw. The wording is matched against the dynamic + // import phrases only, so app code throwing "Failed to load user profile" + // at module scope is not mistaken for a missing module. function isModuleNotFoundError(error) { if (!error) return false; if (error instanceof SyntaxError) return false; const message = String((error && error.message) || error); - return /(?:Failed to fetch|error loading|Importing a module script failed|Failed to load)/i + return /(?:dynamically imported module|Importing a module script failed|Failed to load module script)/i .test(message); } + // Picks the error that describes the failure best. An error that proves a + // module was reached (link or evaluation) always beats one that only proves + // a URL could not be fetched, because a 404 on a path that was never + // expected to exist explains nothing. Otherwise the earlier error wins: it + // names the module the router actually intended to load. + function preferReachedModuleError(earlier, later) { + if (!earlier) return later; + if (!later) return earlier; + if (isModuleNotFoundError(earlier) && !isModuleNotFoundError(later)) return later; + return earlier; + } + + // Loads a Pages Router page, retrying at /index.js because both + // pages/about.tsx and pages/about/index.tsx are valid sources for the same + // route. The retry is unconditional: gating it on the wording of the + // first rejection turned any unrecognized wording into a blank page. Error + // selection, not the retry, is what must stay precise. + async function loadPageModuleWithIndexFallback( + basePath, + pageSlug, + pageModuleError, + importModule, + ) { + try { + return await importModule(basePath + '.js'); + } catch (error) { + const routeError = preferReachedModuleError(pageModuleError, error); + + // An index slug already resolves to /index.js, so the retry + // would ask for /index/index.js. + if (pageSlug === 'index' || pageSlug.endsWith('/index')) throw routeError; + + try { + return await importModule(basePath + '/index.js'); + } catch (indexError) { + throw preferReachedModuleError(routeError, indexError); + } + } + } + async function renderPage(pathname) { const resolvedPathname = (() => { const input = typeof pathname === 'string' ? pathname : window.location.pathname; @@ -128,30 +170,12 @@ export const getRendererScript = () => ` const prefix = pageSlug.startsWith('@/') ? '' : '/pages'; const basePath = MODULE_SERVER_URL + prefix + '/' + pageSlug; - try { - pageModule = await import(basePath + '.js'); - } catch (error) { - pageModuleError = pageModuleError || error; - - // Only retry at /index.js when the module genuinely could not - // be found. If it was found but failed to link or evaluate, retrying - // a path that does not exist replaces a precise error ("does not - // provide an export named 'createHash'") with a misleading 404. - const canRetryAsIndex = isModuleNotFoundError(error) && - pageSlug !== 'index' && !pageSlug.endsWith('/index'); - - if (!canRetryAsIndex) throw pageModuleError; - - try { - pageModule = await import(basePath + '/index.js'); - } catch (indexError) { - // For a real /index.tsx page, the first 404 was expected - // and this retry is the path that matters: if it reached a module - // and failed to link or evaluate, its error is the real one. Only - // when the retry 404s as well does the original describe more. - throw isModuleNotFoundError(indexError) ? pageModuleError : indexError; - } - } + pageModule = await loadPageModuleWithIndexFallback( + basePath, + pageSlug, + pageModuleError, + (moduleUrl) => import(moduleUrl), + ); } if (!pageModule) { diff --git a/src/modules/server/module-server.test.ts b/src/modules/server/module-server.test.ts index e127c4390f..a65c1a4422 100644 --- a/src/modules/server/module-server.test.ts +++ b/src/modules/server/module-server.test.ts @@ -1067,6 +1067,49 @@ describe({ name: "serveModule", sanitizeResources: false, sanitizeOps: false }, await Deno.remove(cacheDir, { recursive: true }); } }); + + it("serves a TypeScript source request as JavaScript", async () => { + const projectDir = await Deno.makeTempDir({ prefix: "vf-ts-content-type-" }); + + try { + await Deno.mkdir(`${projectDir}/lib`, { recursive: true }); + await Deno.writeTextFile( + `${projectDir}/lib/constants.ts`, + `export const SITE_NAME: string = "veryfront";\n`, + ); + + const response = await serve( + new Request("http://localhost:3000/_vf_modules/lib/constants.ts"), + projectDir, + ); + + assertEquals(response.status, 200); + assertEquals(response.headers.get("content-type"), "application/javascript; charset=utf-8"); + assertStringIncludes(await response.text(), "SITE_NAME"); + } finally { + await Deno.remove(projectDir, { recursive: true }); + } + }); + + it("serves a JSON module requested with a .js suffix as JSON", async () => { + const projectDir = await Deno.makeTempDir({ prefix: "vf-json-content-type-" }); + + try { + await Deno.mkdir(`${projectDir}/lib`, { recursive: true }); + await Deno.writeTextFile(`${projectDir}/lib/data.json`, `{"a":1}\n`); + + const response = await serve( + new Request("http://localhost:3000/_vf_modules/lib/data.json.js"), + projectDir, + ); + + assertEquals(response.status, 200); + assertEquals(response.headers.get("content-type"), "application/json; charset=utf-8"); + assertEquals(JSON.parse(await response.text()), { a: 1 }); + } finally { + await Deno.remove(projectDir, { recursive: true }); + } + }); }); describe("getDevModuleContentType", () => { @@ -1090,4 +1133,20 @@ describe("getDevModuleContentType", () => { it("serves extensionless paths as JavaScript", () => { assertEquals(getDevModuleContentType("pages/index"), "application/javascript; charset=utf-8"); }); + + // The import rewriter appends `.js` to any specifier whose extension it does + // not recognise, so `@/lib/data.json` reaches the server as `lib/data.json.js` + // while the body stays raw JSON. Typing that as JavaScript makes the browser + // throw a syntax error on the first `:` in the object. + it("serves a JSON module requested with a .js suffix as JSON", () => { + assertEquals(getDevModuleContentType("lib/data.json.js"), "application/json; charset=utf-8"); + }); + + it("serves a CSS module requested with a .js suffix as CSS", () => { + assertEquals(getDevModuleContentType("styles/globals.css.js"), "text/css; charset=utf-8"); + }); + + it("still serves plain .js as JavaScript", () => { + assertEquals(getDevModuleContentType("lib/legacy.js"), "application/javascript; charset=utf-8"); + }); }); diff --git a/src/modules/server/module-server.ts b/src/modules/server/module-server.ts index 074811762e..c75ef5d76c 100644 --- a/src/modules/server/module-server.ts +++ b/src/modules/server/module-server.ts @@ -121,8 +121,9 @@ export default {}; ].join("\n") + "\n", "_dnt.polyfills": `export default {};\n`, // Deno import-map alias stub for browser/HTTP-served framework modules. - // Must be a JS module (not JSON) because esbuild strips `with { type: "json" }` - // at es2020 target, and browsers reject JSON MIME type without the assertion. + // Must be a JS module (not JSON): a browser refuses a JSON module unless the + // importer carries `with { type: "json" }`, so serving JS keeps the stub + // independent of how far import attribute support has reached the browser. "_veryfront/_deno-config": `export default ${JSON.stringify({ version: VERSION })};\n`, // dnt rewrites #deno-config to relative deno.js in npm framework modules. "deno": `export default ${JSON.stringify({ version: VERSION })};\n`, @@ -1074,20 +1075,25 @@ const COMPILED_TO_JS_EXTENSIONS = /\.(?:tsx?|jsx|mdx|md)$/; */ export function getDevModuleContentType(modulePath: string): string { const normalizedPath = modulePath.toLowerCase(); + // The import rewriter appends `.js` to any specifier whose extension it does + // not recognise, so `@/lib/data.json` arrives here as `lib/data.json.js` + // while the source file, and therefore the body, is still raw JSON. Resolve + // the source extension the same way the module lookup does before deciding. + const sourcePath = normalizedPath.replace(/\.(?:mjs|js)$/, ""); - if (normalizedPath.endsWith(".map") || normalizedPath.endsWith(".json")) { + if (sourcePath.endsWith(".map") || sourcePath.endsWith(".json")) { return "application/json; charset=utf-8"; } - if (normalizedPath.endsWith(".css")) { + if (sourcePath.endsWith(".css")) { return "text/css; charset=utf-8"; } - // The request path carries the *source* extension, but the body we serve is - // always the compiled JavaScript. Typing the response from the source - // extension yields `application/typescript`, which browsers refuse to execute - // as a module under strict MIME checking. - if (COMPILED_TO_JS_EXTENSIONS.test(normalizedPath)) { + // The request path can carry a source extension, but the body served for one + // is the compiled JavaScript. Typing the response from the source extension + // yields `application/typescript`, which browsers refuse to execute as a + // module under strict MIME checking. + if (COMPILED_TO_JS_EXTENSIONS.test(sourcePath)) { return "application/javascript; charset=utf-8"; } diff --git a/src/rendering/orchestrator/module-loader/build-failure.ts b/src/rendering/orchestrator/module-loader/build-failure.ts new file mode 100644 index 0000000000..ec504279ab --- /dev/null +++ b/src/rendering/orchestrator/module-loader/build-failure.ts @@ -0,0 +1,30 @@ +/** + * Build-failure tagging for module loads. + * + * A page module can fail for two very different reasons, and callers need to + * tell them apart: + * + * - The source could not be compiled or resolved. That is a developer-facing + * build failure, and the message says how to fix it. + * - The module compiled, ran, and threw at module scope (a missing environment + * variable, a rejected top-level `await`). That is an ordinary application + * error, and a project's own error page should present it. + * + * Only the loader is in a position to know which happened, so it tags the + * error at the point of failure instead of leaving later layers to infer it. + */ + +const BUILD_FAILURE = Symbol.for("veryfront.module-loader.build-failure"); + +type TaggedError = Error & { [BUILD_FAILURE]?: true }; + +/** Tag `error` as a build failure and return it. */ +export function markBuildFailure(error: unknown): unknown { + if (error instanceof Error) (error as TaggedError)[BUILD_FAILURE] = true; + return error; +} + +/** True when `error` was raised while compiling or resolving project source. */ +export function isBuildFailure(error: unknown): boolean { + return error instanceof Error && (error as TaggedError)[BUILD_FAILURE] === true; +} diff --git a/src/rendering/orchestrator/module-loader/index.test.ts b/src/rendering/orchestrator/module-loader/index.test.ts index eaa7d7a2cf..6f784f2ab8 100644 --- a/src/rendering/orchestrator/module-loader/index.test.ts +++ b/src/rendering/orchestrator/module-loader/index.test.ts @@ -3,10 +3,11 @@ import { assert, assertEquals, assertNotStrictEquals, + assertRejects, assertStrictEquals, assertStringIncludes, } from "#veryfront/testing/assert.ts"; -import { describe, it } from "#veryfront/testing/bdd.ts"; +import { afterAll, describe, it } from "#veryfront/testing/bdd.ts"; import { getLocalAdapter } from "#veryfront/platform/adapters/registry.ts"; import { dirname, join } from "#veryfront/compat/path/index.ts"; import { runWithCacheDir } from "#veryfront/utils/cache-dir.ts"; @@ -17,6 +18,7 @@ import { transformModuleWithDeps, } from "./index.ts"; import { getModuleCacheKey } from "./module-cache-lookup.ts"; +import { isBuildFailure } from "./build-failure.ts"; async function withModuleLoaderFixture( files: Record, @@ -59,6 +61,13 @@ function assertTransformedImportPath(code: string, expectedPathPart: string): st } describe("module-loader/transformModuleWithDeps", () => { + // The `.ts` cycle case compiles real TypeScript, which starts esbuild's child + // process; stop it so the test does not leak the handle into a later suite. + afterAll(async () => { + const { stop } = await import("veryfront/extensions/bundler"); + await stop(); + }); + it("transforms @/ alias dependencies before rewriting the import to a file URL", async () => { await withModuleLoaderFixture( { @@ -95,6 +104,110 @@ describe("module-loader/transformModuleWithDeps", () => { ); }); + // A dynamic import is how a module graph legitimately breaks a cycle. Before + // dynamic specifiers were followed, this shape terminated because the cycle + // edge was invisible; following it eagerly recurses until the worker dies. + // The race turns a regression into a failure rather than a hung suite. + it("does not recurse forever when a dynamic import closes a cycle", async () => { + await withModuleLoaderFixture( + { + "app/page.json": [ + `import { a } from "../lib/a.json";`, + `export const pageValue = a;`, + ].join("\n"), + "lib/a.json": [ + `export const a = "cycle";`, + `export async function later() { return await import("../app/page.json"); }`, + ].join("\n"), + }, + async ({ projectDir, tmpDir, config }) => { + let timer = 0; + const transformed = await Promise.race([ + transformModuleWithDeps( + join(projectDir, "app/page.json"), + tmpDir, + config.adapter, + config, + ), + new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error("transform did not terminate")), 10_000); + }), + ]).finally(() => clearTimeout(timer)); + + assertStringIncludes(transformed, "/app/page.json"); + + // The cycle edge is left as the author wrote it, so the runtime + // resolves it if that branch is ever taken. + const depCode = await Deno.readTextFile( + assertTransformedImportPath( + await Deno.readTextFile(transformed), + "/lib/a.json", + ), + ); + assertStringIncludes(depCode, `import("../app/page.json")`); + }, + ); + }); + + // The `.ts` counterpart of the cycle case, which the `.json` shape above does + // not exercise: a `.ts` module is persisted as a *content-hashed* `.js` + // artifact (`app/page..js`), and the cycle edge — left un-transformed to + // break the recursion — is normalised by esbuild to a relative `../app/page.js` + // that does not match the hashed name. To make that edge resolvable, the cycle + // target persists a stable non-hashed alias (`app/page.js`) that re-exports + // its hashed artifact. This test pins both halves: the edge shape and the + // alias that backs it. (The alias is not yet runtime-verified end to end; if + // it does not resolve in a real runtime the branch stays broken, no worse than + // before.) + it("writes a resolvable alias when a dynamic import closes a .ts cycle", async () => { + await withModuleLoaderFixture( + { + "app/page.ts": [ + `import { a } from "../lib/a.ts";`, + `export const pageValue = a;`, + ].join("\n"), + "lib/a.ts": [ + `export const a = "cycle";`, + `export async function later() { return await import("../app/page.ts"); }`, + ].join("\n"), + }, + async ({ projectDir, tmpDir, config }) => { + let timer = 0; + const transformed = await Promise.race([ + transformModuleWithDeps( + join(projectDir, "app/page.ts"), + tmpDir, + config.adapter, + config, + ), + new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error("transform did not terminate")), 10_000); + }), + ]).finally(() => clearTimeout(timer)); + + // The static import to lib/a resolves to its content-hashed artifact. + const depArtifactPath = assertTransformedImportPath( + await Deno.readTextFile(transformed), + "/lib/a.", + ); + assert(/\/lib\/a\.[0-9a-f]{8}\.js$/.test(depArtifactPath), depArtifactPath); + + // The cycle edge survives as the relative `.js` specifier esbuild leaves. + const depCode = await Deno.readTextFile(depArtifactPath); + assertStringIncludes(depCode, `import("../app/page.js")`); + + // The alias the edge points at exists next to the hashed artifact and + // re-exports it, so `../app/page.js` resolves to the real module. + const aliasPath = join(tmpDir, "app/page.js"); + const aliasCode = await Deno.readTextFile(aliasPath); + assert( + /export \* from "\.\/page\.[0-9a-f]{8}\.js";/.test(aliasCode), + `alias should re-export the hashed artifact:\n${aliasCode}`, + ); + }, + ); + }); + it("resolves relative imports before rewriting them to file URLs", async () => { await withModuleLoaderFixture( { @@ -121,6 +234,61 @@ describe("module-loader/transformModuleWithDeps", () => { }); }); +describe("module-loader/loadModule build-failure tagging", () => { + // Compiling a real page module starts esbuild's child process; stop it so the + // test does not leak the handle rather than opting out of the sanitizer. + afterAll(async () => { + const { stop } = await import("veryfront/extensions/bundler"); + await stop(); + }); + + // A page whose module ran and threw is an application bug the project's own + // error page should present. A page that never compiled is a developer-facing + // build failure. Only the loader can tell them apart, so it tags the error. + it("tags a failure from the transform step", async () => { + await withModuleLoaderFixture( + { + "app/page.tsx": [ + `import logo from "@/assets/logo.svg";`, + `export default function Page() { return logo; }`, + ].join("\n"), + }, + async ({ projectDir, tmpDir, config }) => { + await runWithCacheDir(tmpDir, async () => { + const error = await assertRejects( + () => loadModule(join(projectDir, "app/page.tsx"), config), + Error, + ); + + assertEquals(isBuildFailure(error), true); + }); + }, + ); + }); + + it("does not tag a module that compiled and threw at module scope", async () => { + await withModuleLoaderFixture( + { + "app/page.ts": [ + `throw new Error("Missing API key");`, + `export const value = "unreachable";`, + ].join("\n"), + }, + async ({ projectDir, tmpDir, config }) => { + await runWithCacheDir(tmpDir, async () => { + const error = await assertRejects( + () => loadModule(join(projectDir, "app/page.ts"), config), + Error, + "Missing API key", + ); + + assertEquals(isBuildFailure(error), false); + }); + }, + ); + }); +}); + describe("module-loader/loadModule", () => { it("reuses the content-addressed module identity across repeated loads", async () => { await withModuleLoaderFixture( diff --git a/src/rendering/orchestrator/module-loader/index.ts b/src/rendering/orchestrator/module-loader/index.ts index d27424129a..67de4207e5 100644 --- a/src/rendering/orchestrator/module-loader/index.ts +++ b/src/rendering/orchestrator/module-loader/index.ts @@ -17,10 +17,14 @@ import { invalidateMdxEsmModule } from "#veryfront/transforms/mdx/esm-module-loa import { resolveModuleDependencies, rewriteResolvedDependencyImports, + type TransformedModuleDependency, } from "./dependency-resolver.ts"; import { persistTransformedModule } from "./module-persistence.ts"; import { transformModuleCodeWithCache } from "./module-transform-cache.ts"; import { getModuleCacheKey, resolveCachedModulePath } from "./module-cache-lookup.ts"; +import { markBuildFailure } from "./build-failure.ts"; + +export { isBuildFailure } from "./build-failure.ts"; const logger = rendererLogger.component("module-loader"); @@ -49,6 +53,12 @@ export async function transformModuleWithDeps( localAdapter: RuntimeAdapter, config: ModuleLoaderConfig, useLocalAdapter = false, + lineage: ReadonlySet = new Set(), + // Shared by reference across the whole transform tree (unlike `lineage`, which + // is copied per level): a descendant records a cycle target here, and the + // ancestor that eventually persists that target reads it to write a stable + // alias the left-as-authored cycle edge can resolve to. + cycleTargets: Set = new Set(), ): Promise { const { moduleCache, projectDir, projectId, contentSourceId, adapter, mode } = config; const cacheKey = getModuleCacheKey( @@ -83,25 +93,68 @@ export async function transformModuleWithDeps( projectDir, }); - const transformedDeps = await Promise.all( + // The module cache is only written once a transform completes, so it cannot + // break a cycle that is still in progress. Carry the chain instead. + const nextLineage = new Set(lineage).add(filePath); + + const transformedDeps = (await Promise.all( resolvedDeps.filter((d) => d.depFilePath).map(async (dep) => { + // `await import()` is how a module graph legitimately breaks an import + // cycle, so following one eagerly can lead straight back to a module + // further up this chain and recurse until the worker dies. Leave the + // specifier as authored so the recursion terminates. + // + // The cycle target is persisted as a content-hashed artifact whose hash + // is derived from transformed output we do not produce here (producing it + // is the recursion we are breaking), so the edge cannot be rewritten to + // that hashed path. Instead we record the target: when its ancestor + // persists it, a stable non-hashed alias is written next to the hashed + // artifact so the relative `.js` specifier esbuild leaves behind resolves. + // NOTE: this alias path is not yet runtime-verified end to end; if it does + // not resolve in a real runtime the cycle branch stays broken, which is no + // worse than before (and still a strict improvement over hanging). + if (nextLineage.has(dep.depFilePath!)) { + cycleTargets.add(dep.depFilePath!); + logger.debug("Skipping dependency already in the transform chain:", { + path: dep.path, + depFilePath: dep.depFilePath, + }); + return null; + } + logger.debug("Found dependency:", { path: dep.path, depFilePath: dep.depFilePath, isLocalLib: dep.isLocalLib, }); - const depTempPath = await transformModuleWithDeps( - dep.depFilePath!, - tmpDir, - localAdapter, - config, - dep.isLocalLib, - ); - - return { ...dep, depTempPath }; + try { + const depTempPath = await transformModuleWithDeps( + dep.depFilePath!, + tmpDir, + localAdapter, + config, + dep.isLocalLib, + nextLineage, + cycleTargets, + ); + + return { ...dep, depTempPath }; + } catch (error) { + // A static import has to resolve for the importer to run at all. A + // dynamic one may never be evaluated, so a module behind an untaken + // branch must not fail the page that merely mentions it. + if (!dep.isDynamic) throw error; + + logger.warn("Leaving an unresolvable dynamic dependency as authored:", { + path: dep.path, + depFilePath: dep.depFilePath, + reason: error instanceof Error ? error.message : String(error), + }); + return null; + } }), - ); + )).filter((dep): dep is TransformedModuleDependency => dep !== null); fileContent = rewriteResolvedDependencyImports(fileContent, transformedDeps); for (const dep of transformedDeps) { @@ -141,6 +194,7 @@ export async function transformModuleWithDeps( cacheKey, contentSourceId, reactVersion: config.reactVersion, + isCycleTarget: cycleTargets.has(filePath), }); } @@ -205,7 +259,15 @@ export async function loadModule( const tmpDir = await getModuleCacheDir(config); const localAdapter = await getLocalAdapter(); - const tempFilePath = await transformModuleWithDeps(filePath, tmpDir, localAdapter, config); + // Everything up to here compiles and resolves source, so a failure is a build + // failure. Everything after it is the module running. + let tempFilePath: string; + try { + tempFilePath = await transformModuleWithDeps(filePath, tmpDir, localAdapter, config); + } catch (error) { + throw markBuildFailure(error); + } + const moduleUrl = `file://${tempFilePath}`; try { @@ -263,7 +325,13 @@ export async function loadModule( // project-scoped — see invalidateMdxEsmModule). invalidateMdxEsmModule(tmpDir, filePath, config.projectDir, config.reactVersion); - const rebuiltPath = await transformModuleWithDeps(filePath, tmpDir, localAdapter, config); + let rebuiltPath: string; + try { + rebuiltPath = await transformModuleWithDeps(filePath, tmpDir, localAdapter, config); + } catch (rebuildError) { + throw markBuildFailure(rebuildError); + } + return await import(`file://${rebuiltPath}?t=${Date.now()}&rebuilt=1`); } diff --git a/src/rendering/orchestrator/module-loader/module-persistence.ts b/src/rendering/orchestrator/module-loader/module-persistence.ts index d72689abff..ac5e6d62d6 100644 --- a/src/rendering/orchestrator/module-loader/module-persistence.ts +++ b/src/rendering/orchestrator/module-loader/module-persistence.ts @@ -69,6 +69,63 @@ export interface PersistTransformedModuleInput { cacheKey: string; contentSourceId?: string; reactVersion?: string; + /** + * True when a dynamic import elsewhere closes a cycle back onto this module. + * Such an edge is left as authored (`import("../app/page.js")`), so it needs a + * stable, non-hashed alias next to the content-hashed artifact to resolve to. + */ + isCycleTarget?: boolean; +} + +/** + * Whether transformed output exposes a default export, so a cycle alias knows + * to re-export it. Covers esbuild's `export default …`, `… as default`, and + * `export { default } from …` forms. + */ +function hasDefaultExport(code: string): boolean { + return /\bexport\s+default\b/.test(code) || + /\bas\s+default\b/.test(code) || + /\bexport\s*\{[^}]*\bdefault\b[^}]*\}/.test(code); +} + +/** + * Write a stable, non-hashed alias next to a cycle target's hashed artifact. + * + * A dynamic import that closes an import cycle is left as the author wrote it + * (see the module loader), so esbuild normalises it to a relative `.js` path + * (`../app/page.js`) that does not match the content-hashed artifact + * (`../app/page..js`). The alias sits at that relative path and re-exports + * the real artifact, so the edge resolves if the branch runs. Best-effort: a + * failed alias just leaves the pre-existing (unresolved) cycle edge in place. + */ +async function writeCycleTargetAlias( + input: PersistTransformedModuleInput, + relativePath: string, + hashedFileName: string, +): Promise { + const aliasRelativePath = relativePath.replace(/\.(tsx?|jsx|mdx)$/, ".js"); + // Same extension in and out means nothing was renamed (already `.js`): the + // authored edge already points at the real artifact, so no alias is needed. + if (aliasRelativePath === relativePath) return; + + const aliasPath = join(input.tmpDir, aliasRelativePath); + const lines = [`export * from "./${hashedFileName}";`]; + if (hasDefaultExport(input.transformedCode)) { + lines.push(`export { default } from "./${hashedFileName}";`); + } + + try { + await input.localAdapter.fs.writeFile(aliasPath, lines.join("\n")); + logger.debug("Wrote cycle-target alias", { + alias: aliasRelativePath, + target: hashedFileName, + }); + } catch (error) { + logger.warn("Failed to write cycle-target alias", { + filePath: input.filePath.slice(-40), + error: error instanceof Error ? error.message : String(error), + }); + } } /** Write a transformed module artifact and register cache pointers. */ @@ -136,5 +193,11 @@ export async function persistTransformedModule( } input.moduleCache.set(input.cacheKey, tempFilePath); + + if (input.isCycleTarget) { + const hashedFileName = jsPath.slice(jsPath.lastIndexOf("/") + 1); + await writeCycleTargetAlias(input, relativePath, hashedFileName); + } + return tempFilePath; } diff --git a/src/rendering/orchestrator/pipeline.behavior.test.ts b/src/rendering/orchestrator/pipeline.behavior.test.ts index bcfca5b1d9..c551c5a9d4 100644 --- a/src/rendering/orchestrator/pipeline.behavior.test.ts +++ b/src/rendering/orchestrator/pipeline.behavior.test.ts @@ -2,6 +2,7 @@ import "#veryfront/schemas/_test-setup.ts"; import { assert, assertEquals, assertRejects } from "#veryfront/testing/assert.ts"; import { afterEach, describe, it } from "#veryfront/testing/bdd.ts"; import { RenderPipeline, type RenderPipelineConfig } from "./pipeline.ts"; +import { markBuildFailure } from "./module-loader/build-failure.ts"; import { cachePageCss, getPageCssCacheKey } from "./css-cache.ts"; import { cacheCSSAsync } from "#veryfront/html/styles-builder/index.ts"; import { RELEASE_ASSET_MANIFEST_ENV_FLAG } from "#veryfront/release-assets/constants.ts"; @@ -333,6 +334,54 @@ describe("RenderPipeline behavior", () => { } }); + describe("critical page module failures", () => { + // Downstream (the SSR handler) decides whether to show the project's own + // error page or the dev overlay, so the reason the module never loaded has + // to survive the trip. + type LoadModuleOverride = { loadModule: (path: string) => Promise }; + + function pipelineWithFailingPageModule(fail: () => never): RenderPipeline { + const pipeline = createPipeline("/project/pages/behavior-load-failure.tsx"); + (pipeline as unknown as LoadModuleOverride).loadModule = () => Promise.resolve(fail()); + return pipeline; + } + + function rejectLoad(pipeline: RenderPipeline): Promise { + const slug = "/behavior-load-failure"; + return assertRejects( + () => + pipeline.resolvePageData(slug, { + projectId: "proj-load-failure", + request: new Request(`http://localhost${slug}`), + url: new URL(`http://localhost${slug}`), + }), + Error, + "Critical page module(s) failed to load", + ); + } + + function buildFailureFlag(error: unknown): unknown { + const context = (error as { context?: { buildFailure?: unknown } }).context; + return context?.buildFailure; + } + + it("reports a build failure as one", async () => { + const error = await rejectLoad(pipelineWithFailingPageModule(() => { + throw markBuildFailure(new Error("Cannot import the static asset")); + })); + + assertEquals(buildFailureFlag(error), true); + }); + + it("does not report a module-scope runtime throw as a build failure", async () => { + const error = await rejectLoad(pipelineWithFailingPageModule(() => { + throw new Error("Missing API key"); + })); + + assertEquals(buildFailureFlag(error), false); + }); + }); + it("resolvePageData surfaces notFound from data hooks", async () => { const slug = "/behavior-not-found"; const projectId = "proj-not-found"; diff --git a/src/rendering/orchestrator/pipeline.ts b/src/rendering/orchestrator/pipeline.ts index 1dff5815d7..e4be0c8572 100644 --- a/src/rendering/orchestrator/pipeline.ts +++ b/src/rendering/orchestrator/pipeline.ts @@ -60,6 +60,7 @@ import { } from "#veryfront/html/styles-builder/tailwind-compiler.ts"; import { getReadyManifestForRender } from "#veryfront/release-assets/manifest-cache.ts"; import { createEsmCache, createModuleCache, loadModule } from "./module-loader/index.ts"; +import { isBuildFailure } from "./module-loader/build-failure.ts"; import type { ModuleLoaderConfig } from "./module-loader/index.ts"; import { getCSSImports, @@ -282,7 +283,7 @@ export class RenderPipeline { ); const loaded: LoadedModule[] = []; - const criticalFailures: Array<{ path: string; error: string }> = []; + const criticalFailures: Array<{ path: string; error: string; buildFailure: boolean }> = []; for (const result of results) { if (result.mod && !result.error) { @@ -295,7 +296,11 @@ export class RenderPipeline { const errorMessage = result.error.message; if (result.type === "page") { - criticalFailures.push({ path: result.path, error: errorMessage }); + criticalFailures.push({ + path: result.path, + error: errorMessage, + buildFailure: isBuildFailure(result.error), + }); renderPageLog.error("Critical page module failed to load", { path: result.path, error: errorMessage, @@ -317,6 +322,10 @@ export class RenderPipeline { detail: `Critical page module(s) failed to load:\n${failedDetails}`, context: { criticalFailures, + // A module that never compiled is a developer-facing build failure; + // one that compiled and threw at module scope is an application + // error the project's own error page should present. + buildFailure: criticalFailures.some((f) => f.buildFailure), loadedCount: loaded.length, totalModules: modules.length, }, diff --git a/src/routing/api/context-builder.test.ts b/src/routing/api/context-builder.test.ts index cc354c2606..89ccf67d9f 100644 --- a/src/routing/api/context-builder.test.ts +++ b/src/routing/api/context-builder.test.ts @@ -1,10 +1,10 @@ import "#veryfront/schemas/_test-setup.ts"; -import { assertEquals, assertExists } from "#veryfront/testing/assert.ts"; +import { assertEquals, assertExists, assertRejects } from "#veryfront/testing/assert.ts"; import { describe, it } from "#veryfront/testing/bdd.ts"; import { type APIContext, + createBodyReader, createContext, - createJsonHelper, normalizeParams, parseCookies, } from "./context-builder.ts"; @@ -389,26 +389,12 @@ describe("API Context Builder", () => { }); }); -describe("createContext: ctx.json arity", () => { +describe("createContext: ctx.json writes, ctx.body reads", () => { function ctxFor(request: Request): APIContext { return createContext(request, { params: {} } as RouteMatch, mockFs); } - it("parses the request body when called with no arguments", async () => { - // `await ctx.json()` used to stringify `undefined` into a Response, so the - // handler received an object that serialised back out as `{}`. - const ctx = ctxFor( - new Request("http://localhost/api/echo", { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ x: 1, nested: { y: "z" } }), - }), - ); - - assertEquals(await ctx.json(), { x: 1, nested: { y: "z" } }); - }); - - it("still builds a JSON response when called with data", async () => { + it("builds a JSON response from ctx.json(data)", async () => { const ctx = ctxFor(new Request("http://localhost/api/echo")); const response = ctx.json({ received: true }); @@ -425,57 +411,78 @@ describe("createContext: ctx.json arity", () => { assertEquals(await response.json(), { error: "nope" }); }); - // Regression: worker isolation built its own `ctx.json` as a response helper - // only, so `await ctx.json()` still returned a Response under - // WORKER_ISOLATION_API=1. Both contexts now build it from this one function, - // so a handler behaves the same whether or not isolation is enabled. - describe("createJsonHelper", () => { - it("reads the request body with no arguments", async () => { - const json = createJsonHelper( - new Request("http://localhost/api/echo", { - method: "POST", - body: JSON.stringify({ isolated: true }), - headers: { "Content-Type": "application/json" }, - }), - ); + it("reads the request body with ctx.body()", async () => { + const ctx = ctxFor( + new Request("http://localhost/api/echo", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ x: 1, nested: { y: "z" } }), + }), + ); - assertEquals(await json(), { isolated: true }); - }); + assertEquals(await ctx.body(), { x: 1, nested: { y: "z" } }); + }); - it("builds a response when given data", async () => { - const json = createJsonHelper(new Request("http://localhost/api/echo")); - const response = json({ ok: true }, { status: 201 }); + it("caches the parse so ctx.body() can be read more than once", async () => { + // A validation helper and the handler both receive only `ctx`, so both + // reach for the body. A single-use stream would make the second call throw. + const ctx = ctxFor( + new Request("http://localhost/api/echo", { + method: "POST", + body: JSON.stringify({ count: 7 }), + }), + ); - assertEquals(response instanceof Response, true); - assertEquals(response.status, 201); - assertEquals(response.headers.get("Content-Type"), "application/json"); - assertEquals(await response.json(), { ok: true }); - }); + assertEquals(await ctx.body(), { count: 7 }); + assertEquals(await ctx.body(), { count: 7 }); + }); - it("is the same helper the request context exposes", async () => { - const request = new Request("http://localhost/api/echo", { + it("does not consume the body away from a manual ctx.request.json()", async () => { + const ctx = ctxFor( + new Request("http://localhost/api/echo", { method: "POST", body: JSON.stringify({ shared: true }), - }); + }), + ); - const viaHelper = await createJsonHelper(request.clone())(); - const viaContext = await ctxFor(request).json(); + await ctx.body(); + // The raw request stream is untouched by ctx.body(), so this still works. + assertEquals(await ctx.request.json(), { shared: true }); + }); - assertEquals(viaContext, viaHelper); - }); + it("reads via ctx.body() even after ctx.request was consumed raw first", async () => { + // The reverse order: a handler reads the raw stream, *then* reaches for + // ctx.body(). The clone is taken at construction time, so it does not throw + // `Body already consumed` no matter which one runs first. + const ctx = ctxFor( + new Request("http://localhost/api/echo", { + method: "POST", + body: JSON.stringify({ shared: true }), + }), + ); + + assertEquals(await ctx.request.json(), { shared: true }); + assertEquals(await ctx.body(), { shared: true }); }); - it("rejects when the body is not valid JSON", async () => { + it("throws a 400 when the body is not valid JSON", async () => { const ctx = ctxFor( new Request("http://localhost/api/echo", { method: "POST", body: "not json" }), ); - let threw = false; - try { - await ctx.json(); - } catch (_) { - threw = true; - } - assertEquals(threw, true); + const error = await assertRejects(() => ctx.body()); + assertEquals((error as { status?: number }).status, 400); + }); + + it("createBodyReader reads the body under worker isolation too", async () => { + // Worker isolation builds its own context, so it uses this same reader. + const read = createBodyReader( + new Request("http://localhost/api/echo", { + method: "POST", + body: JSON.stringify({ isolated: true }), + }), + ); + + assertEquals(await read(), { isolated: true }); }); }); diff --git a/src/routing/api/context-builder.ts b/src/routing/api/context-builder.ts index bc717f4e85..290301be76 100644 --- a/src/routing/api/context-builder.ts +++ b/src/routing/api/context-builder.ts @@ -1,6 +1,7 @@ import type { RouteMatch } from "./api-route-matcher.ts"; import type { FileSystemAdapter } from "#veryfront/platform/adapters/base.ts"; import { parseCookies } from "#veryfront/utils/cookie-utils.ts"; +import { INVALID_ARGUMENT } from "#veryfront/errors"; import { flattenRouteParams } from "../flatten-route-params.ts"; export { parseCookies }; @@ -15,15 +16,19 @@ export interface APIContext { headers: Headers; url: URL; /** - * Read the request body as JSON, or build a JSON response. + * Build a JSON `Response`. `ctx.json(data, init?)` mirrors `Response.json`. * - * `ctx.json()` with no arguments parses and returns the request body. - * `ctx.json(data, init?)` returns a JSON `Response`. + * To read the request body, use `ctx.body()` or the raw `ctx.request.json()`. */ - json: { - (): Promise; - (data: unknown, init?: ResponseInit): Response; - }; + json: (data: unknown, init?: ResponseInit) => Response; + /** + * Read and parse the request body as JSON. + * + * The result is cached, so calling it more than once (or alongside a manual + * `ctx.request.json()`) does not throw `Body already consumed`. A body that + * is not valid JSON becomes a 400, not an unhandled 500. + */ + body: () => Promise; text: (data: string, init?: ResponseInit) => Response; fs: FileSystemAdapter; } @@ -43,27 +48,45 @@ function createResponse( } /** - * Build the `ctx.json` helper for a request. - * - * Overloaded on arity. `ctx.json(data)` builds a response; `ctx.json()` reads - * the request body, which is what handlers reach for, and what the zero-arg - * form was previously misread as. It used to stringify `undefined` into a - * Response, so `await ctx.json()` yielded a Response object that serialised - * back out as `{}` and silently dropped every posted payload. + * Build the `ctx.json` response helper. Writes only; mirrors `Response.json`. * * Exported because the isolation Worker builds its own context and has to * behave identically. Handlers must not care which one ran them. */ -export function createJsonHelper(request: Request): APIContext["json"] { - function json(): Promise; - function json(data: unknown, init?: ResponseInit): Response; - function json(...args: [] | [unknown, ResponseInit?]): Response | Promise { - if (args.length === 0) return request.json(); - const [data, init] = args; - return createResponse(JSON.stringify(data), "application/json", init); - } +export function createJsonHelper(_request: Request): APIContext["json"] { + return (data: unknown, init?: ResponseInit): Response => + createResponse(JSON.stringify(data), "application/json", init); +} + +/** + * Build the `ctx.body` request-body reader. + * + * The parse is memoised on the first call, so a validation helper and a handler + * that both read the body do not fight over a single-use stream. The clone is + * taken up front, while the context is built, so `ctx.request` is left intact + * for a handler that still wants the raw stream — and, crucially, so a handler + * that reads `ctx.request` raw *before* calling `ctx.body()` cannot make the + * clone throw `Body already consumed` (`request.clone()` throws synchronously + * once the original stream is disturbed). A malformed body is turned into a + * catalogued 400 rather than escaping as a 500. + */ +export function createBodyReader(request: Request): APIContext["body"] { + // Clone eagerly: at construction time the original body is guaranteed + // untouched, so the read is order-independent with any raw `ctx.request` + // access a handler performs later. + const source = request.clone(); + let parsed: Promise | undefined; + + const read = (): Promise => { + if (!parsed) { + parsed = source.json().catch(() => { + throw INVALID_ARGUMENT.create({ detail: "Request body is not valid JSON" }); + }); + } + return parsed; + }; - return json; + return (): Promise => read() as Promise; } export function createContext( @@ -73,6 +96,7 @@ export function createContext( ): APIContext { const url = new URL(request.url); const json = createJsonHelper(request); + const body = createBodyReader(request); const text = (data: string, init?: ResponseInit): Response => createResponse(data, "text/plain", init); @@ -86,6 +110,7 @@ export function createContext( headers: request.headers, url, json, + body, text, fs, }; diff --git a/src/routing/api/handler.test.ts b/src/routing/api/handler.test.ts index 5d0eea7e44..4565bcd601 100644 --- a/src/routing/api/handler.test.ts +++ b/src/routing/api/handler.test.ts @@ -528,6 +528,40 @@ describe("APIRouteHandler", () => { assertEquals(await empty?.text(), "Handler not found"); }); + // The load error names files, specifiers and build internals. It is a + // development aid, and the only thing keeping it out of a deployed response + // body is this flag. + it("withholds the load error from a response when the project is not local", async () => { + const { handler, localCtx } = await handlerWithTwoRoutes((modulePath) => { + if (modulePath.includes("broken")) { + throw new Error("Unexpected token in /srv/releases/17/pages/api/broken.ts"); + } + return Promise.resolve({}); + }); + + const hosted = await handler.handle( + new Request("http://localhost/api/broken"), + { ...localCtx, isLocalProject: false }, + ); + + assertEquals(hosted?.status, 500); + assertEquals(await hosted?.text(), "Handler not found"); + }); + + it("withholds the load error from a response when there is no context", async () => { + const { handler } = await handlerWithTwoRoutes((modulePath) => { + if (modulePath.includes("broken")) { + throw new Error("Unexpected token in /srv/releases/17/pages/api/broken.ts"); + } + return Promise.resolve({}); + }); + + const anonymous = await handler.handle(new Request("http://localhost/api/broken")); + + assertEquals(anonymous?.status, 500); + assertEquals(await anonymous?.text(), "Handler not found"); + }); + it("classifies the allow-list block against the current attempt only", async () => { const { handler } = await handlerWithTwoRoutes((modulePath) => { if (modulePath.includes("broken")) { diff --git a/src/routing/api/module-loader/loader.test.ts b/src/routing/api/module-loader/loader.test.ts index 5341d640ee..815e426001 100644 --- a/src/routing/api/module-loader/loader.test.ts +++ b/src/routing/api/module-loader/loader.test.ts @@ -177,6 +177,76 @@ describe("loadHandlerModule", { sanitizeResources: false, sanitizeOps: false }, assertEquals(typeof route?.GET, "function"); }); + // Every template ships `paths: { "@/*": ["./*"] }` and no import map entry, so + // the alias a real project uses is resolved by esbuild itself, not by the + // import map plugin. That lane has to work. + it("loads an @/ alias resolved through the project's tsconfig paths", async () => { + const projectDir = await makeTempDir(); + await fs.mkdir(join(projectDir, "lib"), { recursive: true }); + await fs.mkdir(join(projectDir, "pages", "api"), { recursive: true }); + + await fs.writeTextFile( + join(projectDir, "lib", "greeting.ts"), + `export const greeting = "tsconfig";`, + ); + await fs.writeTextFile( + join(projectDir, "tsconfig.json"), + JSON.stringify({ compilerOptions: { paths: { "@/*": ["./*"] } } }), + ); + + const modulePath = join(projectDir, "pages", "api", "aliased.ts"); + await fs.writeTextFile( + modulePath, + [ + `import { greeting } from "@/lib/greeting.ts";`, + `export function GET() { return new Response(greeting); }`, + ].join("\n"), + ); + + const route = await loadHandlerModule({ + projectDir, + modulePath, + adapter, + config: undefined, + }); + + assertEquals(typeof route?.GET, "function"); + }); + + // esbuild applies tsconfig `paths` before any plugin's onResolve runs, so a + // boundary check that lives in a resolver plugin never sees the result. An + // alias that climbs out of the project must not load. + it("rejects an @/ alias that escapes the project root", async () => { + const rootDir = await makeTempDir(); + const projectDir = join(rootDir, "project"); + const outsideDir = join(rootDir, "outside"); + await fs.mkdir(join(projectDir, "pages", "api"), { recursive: true }); + await fs.mkdir(outsideDir, { recursive: true }); + + await fs.writeTextFile( + join(outsideDir, "secret.ts"), + `export const secret = "TOP-SECRET-OUTSIDE-PROJECT";`, + ); + await fs.writeTextFile( + join(projectDir, "tsconfig.json"), + JSON.stringify({ compilerOptions: { paths: { "@/*": ["./*"] } } }), + ); + + const modulePath = join(projectDir, "pages", "api", "leak.ts"); + await fs.writeTextFile( + modulePath, + [ + `import { secret } from "@/../outside/secret.ts";`, + `export function GET() { return new Response(secret); }`, + ].join("\n"), + ); + + await assertRejects( + () => loadHandlerModule({ projectDir, modulePath, adapter, config: undefined }), + Error, + ); + }); + // Bundling reads the route through the adapter; a direct import does not. A // module that threw while evaluating must surface its own error rather than // be evaluated a second time under bundling semantics. diff --git a/src/routing/api/module-loader/loader.ts b/src/routing/api/module-loader/loader.ts index 9cffba52a6..93e1a75710 100644 --- a/src/routing/api/module-loader/loader.ts +++ b/src/routing/api/module-loader/loader.ts @@ -8,7 +8,7 @@ import { loadSecurityConfig } from "./security-config.ts"; import type { APIRoute, LoadModuleOptions } from "./types.ts"; import { createError, toError } from "#veryfront/errors"; import { getEsbuildLoader } from "#veryfront/utils/path-utils.ts"; -import { createFileSystem } from "#veryfront/platform/compat/fs.ts"; +import { createFileSystem, realPath } from "#veryfront/platform/compat/fs.ts"; import type { FileSystem } from "#veryfront/platform/compat/fs.ts"; import * as pathHelper from "#veryfront/compat/path"; import { FILE_EXTENSIONS, getLoaderForFile, validateModulePath } from "./loader-helpers.ts"; @@ -309,6 +309,62 @@ function createAdapterResolvePlugin( }; } +/** + * Refuse to load any file the bundle resolved outside the project. + * + * The resolver plugins above validate the specifiers they claim, but esbuild + * applies the project's own tsconfig `paths` itself, before any `onResolve` + * callback runs. A templated project maps `@/*` to `./*`, so `@/../secrets.ts` + * is resolved by esbuild straight to a path above the project root and loaded + * in the default namespace, where none of those guards ever see it. + * + * This runs at load time instead, which is the one point every resolution + * strategy converges on. Returning `undefined` defers to normal loading, so the + * plugin only ever subtracts. Package code is exempt: `node_modules` is + * resolved by esbuild's own node resolution rather than by a project alias, and + * is legitimately hoisted above the project root in a monorepo. + * + * `roots` carries both the project path as configured and its symlink-resolved + * form, because esbuild reports the real path of a file it loaded. A project + * reached through a symlink (`/var` -> `/private/var` on macOS, and any deploy + * layout that symlinks a release directory) would otherwise fail every import. + */ +/** + * The project path as configured, plus its symlink-resolved form when they + * differ. `realPath` throws if the directory is missing, in which case the + * configured path is all there is to compare against. + */ +async function resolveProjectRoots(projectDir: string): Promise { + const configured = pathHelper.resolve(projectDir); + + try { + const real = await realPath(configured); + return real === configured ? [configured] : [configured, real]; + } catch { + return [configured]; + } +} + +function createProjectBoundaryPlugin(roots: string[]): Plugin { + return { + name: "vf-project-boundary", + setup(build) { + build.onLoad({ filter: /.*/ }, (args) => { + if (roots.some((root) => isWithinDirectory(root, args.path))) return undefined; + if (args.path.split(/[\\/]/).includes("node_modules")) return undefined; + + logger.error(`[API] Resolved import escapes project: ${args.path}`); + return { + errors: [{ + text: `Import escapes the project directory: ${args.path}. ` + + `API routes may only import files inside the project.`, + }], + }; + }); + }, + }; +} + function loadAndTranspileModule( modulePath: string, projectDir: string, @@ -409,6 +465,7 @@ function loadAndTranspileModule( createImportMapPlugin(projectDir, adapter, config), createAdapterResolvePlugin(adapter, projectDir), createHTTPPlugin({ allowedHosts, projectDir }), + createProjectBoundaryPlugin(await resolveProjectRoots(projectDir)), ], }); diff --git a/src/security/sandbox/worker-script.ts b/src/security/sandbox/worker-script.ts index 0622aaec17..29c6aba6a5 100644 --- a/src/security/sandbox/worker-script.ts +++ b/src/security/sandbox/worker-script.ts @@ -32,8 +32,9 @@ import type { import { installWorkerEgressGuard, type WorkerEgressGuardOptions } from "./worker-egress-guard.ts"; import { isAbsolute, relative, resolve as resolvePath, sep as PATH_SEP } from "node:path"; import { runWithExactSourceIntegrationPolicy } from "#veryfront/integrations/source-policy-context.ts"; +import { isDataControlResult, toDataControlResult } from "#veryfront/data/helpers.ts"; import { parseSourceIntegrationPolicyManifest } from "#veryfront/integrations/source-policy.ts"; -import { createJsonHelper } from "#veryfront/routing/api/context-builder.ts"; +import { createBodyReader, createJsonHelper } from "#veryfront/routing/api/context-builder.ts"; // Module-level singletons to avoid per-call allocation churn const encoder = new TextEncoder(); @@ -365,6 +366,28 @@ function deserializeDataContext( }; } +/** + * Run the project's `getServerData` and fold a thrown control result back into + * a normal result. + * + * `throw notFound()` and `throw redirect(...)` must behave like the returned + * form here as well as in-process. The normalisation has to happen inside the + * worker: the brand is a symbol, `structuredClone` drops symbols, and the + * worker error path would otherwise serialize the plain object with `String()` + * and hand the host "[object Object]" as a 500. + */ +async function runServerData( + getServerData: (ctx: unknown) => unknown | Promise, + context: unknown, +): Promise { + try { + return (await getServerData(context)) as SerializedDataResult; + } catch (error) { + if (isDataControlResult(error)) return toDataControlResult(error); + throw error; + } +} + async function handleFetchData(req: FetchDataRequest): Promise { return await runWithWorkerSourceIntegrationPolicy( req.sourceIntegrationPolicy, @@ -379,7 +402,7 @@ async function handleFetchData(req: FetchDataRequest): Promise new Response(data, { ...init, diff --git a/src/server/dev-server/middleware.test.ts b/src/server/dev-server/middleware.test.ts index 1634122b77..d46cded271 100644 --- a/src/server/dev-server/middleware.test.ts +++ b/src/server/dev-server/middleware.test.ts @@ -121,6 +121,51 @@ describe("dev-server/middleware: actionable rejection", () => { assertStringIncludes(error.message, "other"); }); + it("describes a default export array with a non-function entry", async () => { + // Every wrong shape with a default export used to collapse to the useless + // "Found export(s): default." because the message read the namespace keys, + // not the resolved default. + const adapter = createVirtualAdapter( + "export default [async (c, next) => await next(), 'audit'];", + ); + + const error = await assertRejects( + () => loadMiddlewareFile("/app", adapter, { throwOnError: true }), + TypeError, + ); + + assertInstanceOf(error, TypeError); + assertStringIncludes(error.message, "non-function at index 1"); + assertStringIncludes(error.message, "(string)"); + }); + + it("describes an empty default export array", async () => { + const adapter = createVirtualAdapter("export default [];"); + + const error = await assertRejects( + () => loadMiddlewareFile("/app", adapter, { throwOnError: true }), + TypeError, + ); + + assertInstanceOf(error, TypeError); + assertStringIncludes(error.message, "empty default export array"); + }); + + it("describes a default export object that is not middleware", async () => { + const adapter = createVirtualAdapter( + "export default { handler: async (c, next) => await next() };", + ); + + const error = await assertRejects( + () => loadMiddlewareFile("/app", adapter, { throwOnError: true }), + TypeError, + ); + + assertInstanceOf(error, TypeError); + assertStringIncludes(error.message, "default export of type object"); + assertStringIncludes(error.message, "handler"); + }); + it("still accepts a valid default export", async () => { const adapter = createVirtualAdapter( "export default async function (c, next) { return await next(); }", diff --git a/src/server/dev-server/middleware.ts b/src/server/dev-server/middleware.ts index 3769d7f67d..86e5762b52 100644 --- a/src/server/dev-server/middleware.ts +++ b/src/server/dev-server/middleware.ts @@ -183,6 +183,27 @@ async function loadMiddlewareFromVirtualFS( } } +/** Describe the shape of the default export that was rejected. */ +function describeDefaultExport(exported: unknown): string { + if (Array.isArray(exported)) { + if (exported.length === 0) return `Found an empty default export array.`; + const badIndex = exported.findIndex((value) => typeof value !== "function"); + if (badIndex >= 0) { + return `Found a default export array with a non-function at index ${badIndex} ` + + `(${typeof exported[badIndex]}).`; + } + } + + if (exported && typeof exported === "object") { + const keys = Object.keys(exported as Record); + return keys.length > 0 + ? `Found a default export of type object with keys: ${keys.join(", ")}.` + : `Found a default export of type object with no keys.`; + } + + return `Found a default export of type ${typeof exported}.`; +} + /** * Explain what a root middleware file must export. When the module looks like it * was written for Next.js, say so, because that is overwhelmingly why this @@ -192,6 +213,7 @@ async function loadMiddlewareFromVirtualFS( function invalidMiddlewareExport( middlewareModule: unknown, sourceFile: string, + exported: unknown, ): TypeError { const named = middlewareModule && typeof middlewareModule === "object" ? Object.keys(middlewareModule as Record) @@ -200,10 +222,15 @@ function invalidMiddlewareExport( const looksLikeNext = named.includes("middleware") && typeof (middlewareModule as { middleware?: unknown }).middleware === "function"; + const hasDefault = middlewareModule && typeof middlewareModule === "object" && + "default" in middlewareModule; + const detail = looksLikeNext ? `Found a named "middleware" export, which is the Next.js convention. ` + `Veryfront expects a default export, and its middleware receives ` + `(c, next), where c is a context carrying c.req, not the Request itself.` + : hasDefault + ? describeDefaultExport(exported) : named.length > 0 ? `Found export(s): ${named.join(", ")}.` : `The module has no usable export.`; @@ -234,7 +261,7 @@ function normalizeMiddlewareExport( if ( strict && (exported.length === 0 || exported.some((value) => typeof value !== "function")) ) { - throw invalidMiddlewareExport(middlewareModule, sourceFile); + throw invalidMiddlewareExport(middlewareModule, sourceFile, exported); } return exported.filter((middleware): middleware is MiddlewareFunction => typeof middleware === "function" @@ -246,7 +273,7 @@ function normalizeMiddlewareExport( } if (strict) { - throw invalidMiddlewareExport(middlewareModule, sourceFile); + throw invalidMiddlewareExport(middlewareModule, sourceFile, exported); } return []; diff --git a/src/server/handlers/request/ssr/ssr.handler.test.ts b/src/server/handlers/request/ssr/ssr.handler.test.ts index 79b0ce4e6a..6c34edd4d5 100644 --- a/src/server/handlers/request/ssr/ssr.handler.test.ts +++ b/src/server/handlers/request/ssr/ssr.handler.test.ts @@ -689,7 +689,7 @@ describe("handle - build errors bypass the custom error page", () => { // A compile or import failure is a developer-facing bug, never something a // project's 500.tsx should present to a visitor. Masking one behind a // friendly page in dev hides the message that says how to fix it. - function buildFailureService() { + function moduleLoadFailureService(buildFailure: boolean) { return createMockSSRService({ renderPage: () => Promise.resolve({ @@ -701,13 +701,20 @@ describe("handle - build errors bypass the custom error page", () => { showDevOverlay: true, error: RENDER_ERROR.create({ detail: "Critical page module(s) failed to load", - context: { criticalFailures: [{ path: "pages/test/y.tsx", error: "bad import" }] }, + context: { + criticalFailures: [{ path: "pages/test/y.tsx", error: "bad import", buildFailure }], + buildFailure, + }, }), slug: "page", }), }); } + function buildFailureService() { + return moduleLoadFailureService(true); + } + function ctxRecordingStats(): { ctx: ReturnType; statted: string[] } { const statted: string[] = []; const adapter = createMockAdapter(); @@ -719,7 +726,7 @@ describe("handle - build errors bypass the custom error page", () => { return { ctx: makeCtx({ adapter }), statted }; } - it("does not look for a custom error page when the module failed to load", async () => { + it("does not look for a custom error page when the module never compiled", async () => { const { ctx, statted } = ctxRecordingStats(); const handler = new SSRHandler(buildFailureService()); @@ -729,6 +736,18 @@ describe("handle - build errors bypass the custom error page", () => { assertEquals(result.response!.status, 500); }); + it("still uses the custom error page when the module ran and threw", async () => { + // A page module that compiled and threw at module scope (a missing + // environment variable, say) also fails to load, but it is an application + // error, not a build failure, so pages/500.tsx must still present it. + const { ctx, statted } = ctxRecordingStats(); + const handler = new SSRHandler(moduleLoadFailureService(false)); + + await handler.handle(new Request("http://localhost/page"), ctx); + + assertEquals(statted.some((path) => path.endsWith("/pages")), true); + }); + it("still uses the custom error page for an ordinary thrown Error", async () => { const { ctx, statted } = ctxRecordingStats(); const handler = new SSRHandler(createMockSSRService({ diff --git a/src/server/handlers/request/ssr/ssr.handler.ts b/src/server/handlers/request/ssr/ssr.handler.ts index 3e079ba4c8..4dffa5aeaa 100644 --- a/src/server/handlers/request/ssr/ssr.handler.ts +++ b/src/server/handlers/request/ssr/ssr.handler.ts @@ -64,16 +64,18 @@ export function isProductionMode(ctx: HandlerContext, _url?: URL): boolean { * opposed to errors thrown by the running application. * * Module-load failures arrive wrapped in a RUNTIME-category `render-error`, - * which loses the original category, so they are identified by their - * `criticalFailures` context instead: a page module that could not be *loaded* - * always failed to compile or resolve. + * which loses the original category, so they carry a `buildFailure` flag that + * the module loader sets at the point of failure. Failing to load is not + * evidence on its own: a module that compiled fine and threw at module scope + * also fails to load, and that is an application error the project's own error + * page should present. */ function isBuildError(error: unknown): boolean { if (!(error instanceof VeryfrontError)) return false; if (error.category === "BUILD" || error.category === "MODULE") return true; - const context = error.context as { criticalFailures?: unknown } | undefined; - return Array.isArray(context?.criticalFailures) && context.criticalFailures.length > 0; + const context = error.context as { buildFailure?: unknown } | undefined; + return context?.buildFailure === true; } export class SSRHandler extends BaseHandler { diff --git a/src/transforms/esm/import-attributes.test.ts b/src/transforms/esm/import-attributes.test.ts new file mode 100644 index 0000000000..56bb7d3b7e --- /dev/null +++ b/src/transforms/esm/import-attributes.test.ts @@ -0,0 +1,134 @@ +import "#veryfront/schemas/_test-setup.ts"; +import { assertEquals } from "#veryfront/testing/assert.ts"; +import { describe, it } from "#veryfront/testing/bdd.ts"; +import { stripJsonImportAttributes, upgradeImportAssertions } from "./import-attributes.ts"; + +describe("upgradeImportAssertions", () => { + it("rewrites a static assertion clause", async () => { + assertEquals( + await upgradeImportAssertions(`import m from "./a.json" assert { type: "json" };`), + `import m from "./a.json" with { type: "json" };`, + ); + }); + + it("rewrites an assertion clause on a re-export", async () => { + assertEquals( + await upgradeImportAssertions(`export { a } from "./a.json" assert { type: "json" };`), + `export { a } from "./a.json" with { type: "json" };`, + ); + }); + + it("rewrites an assertion clause on a bare side-effect import", async () => { + assertEquals( + await upgradeImportAssertions(`import "./a.json" assert { type: "json" };`), + `import "./a.json" with { type: "json" };`, + ); + }); + + it("rewrites the assertion key of a dynamic import", async () => { + assertEquals( + await upgradeImportAssertions( + `const load = () => import("./a.json", { assert: { type: "json" } });`, + ), + `const load = () => import("./a.json", { with: { type: "json" } });`, + ); + }); + + it("rewrites minified output that carries no whitespace", async () => { + assertEquals( + await upgradeImportAssertions(`import m from"./a.json"assert{type:"json"};`), + `import m from"./a.json"with{type:"json"};`, + ); + }); + + it("rewrites every assertion in the module", async () => { + assertEquals( + await upgradeImportAssertions( + `import a from "./a.json" assert { type: "json" };\n` + + `import b from "./b.json" assert { type: "json" };\n`, + ), + `import a from "./a.json" with { type: "json" };\n` + + `import b from "./b.json" with { type: "json" };\n`, + ); + }); + + it("leaves an attribute clause that already uses the current spelling", async () => { + const code = `import m from "./a.json" with { type: "json" };`; + assertEquals(await upgradeImportAssertions(code), code); + }); + + it("leaves module source embedded in a string literal alone", async () => { + const code = 'export const TPL = `import d from "./a.json" assert { type: "json" };`;'; + assertEquals(await upgradeImportAssertions(code), code); + }); + + it("leaves an assert call that follows an import alone", async () => { + const code = `import { assert } from "./assert.js";\nassert (true);\n`; + assertEquals(await upgradeImportAssertions(code), code); + }); + + it("keeps positions correct when the module also imports over HTTP", async () => { + const code = `import "https://esm.sh/react@19.1.1";\n` + + `import m from "./a.json" assert { type: "json" };\n`; + + assertEquals( + await upgradeImportAssertions(code), + `import "https://esm.sh/react@19.1.1";\n` + + `import m from "./a.json" with { type: "json" };\n`, + ); + }); +}); + +describe("stripJsonImportAttributes", () => { + const stripAll = (code: string) => stripJsonImportAttributes(code, () => true); + + it("removes the clause from a static import", async () => { + assertEquals( + await stripAll(`import m from "./a.mjs" with { type: "json" };`), + `import m from "./a.mjs";`, + ); + }); + + it("removes the options argument from a dynamic import", async () => { + assertEquals( + await stripAll(`const load = () => import("./a.mjs", { with: { type: "json" } });`), + `const load = () => import("./a.mjs");`, + ); + }); + + it("only touches specifiers the predicate accepts", async () => { + const code = `import a from "./a.mjs" with { type: "json" };\n` + + `import b from "./b.json" with { type: "json" };\n`; + + assertEquals( + await stripJsonImportAttributes(code, (specifier) => specifier.endsWith(".mjs")), + `import a from "./a.mjs";\n` + + `import b from "./b.json" with { type: "json" };\n`, + ); + }); + + it("keeps an attribute that is not a json type", async () => { + const code = `import m from "./a.mjs" with { type: "css" };`; + assertEquals(await stripAll(code), code); + }); + + it("keeps a clause that declares more than the json type", async () => { + const code = `import m from "./a.mjs" with { type: "json", integrity: "sha384-abc" };`; + assertEquals(await stripAll(code), code); + }); + + it("leaves module source embedded in a string literal alone", async () => { + const code = 'export const TPL = `import d from "./a.mjs" with { type: "json" };`;'; + assertEquals(await stripAll(code), code); + }); + + it("keeps positions correct when the module also imports over HTTP", async () => { + const code = `import "https://esm.sh/react@19.1.1";\n` + + `import m from "./a.mjs" with { type: "json" };\n`; + + assertEquals( + await stripAll(code), + `import "https://esm.sh/react@19.1.1";\nimport m from "./a.mjs";\n`, + ); + }); +}); diff --git a/src/transforms/esm/import-attributes.ts b/src/transforms/esm/import-attributes.ts new file mode 100644 index 0000000000..bbf0487f55 --- /dev/null +++ b/src/transforms/esm/import-attributes.ts @@ -0,0 +1,134 @@ +/** + * Import attribute rewrites anchored to lexer-reported positions. + * + * Every edit here is bounded by a range es-module-lexer reported for a real + * import statement, so module source embedded in a string literal (the + * `#deno-config` stub, the generated RSC bundles) is never touched. A plain + * regex over the module text cannot make that distinction. + * + * @module transforms/esm/import-attributes + */ + +import { type ImportSpecifier, parseMaskedImports } from "./lexer.ts"; + +const ASSERT_KEYWORD = "assert"; + +/** + * The legacy `assert` keyword following a static import specifier. + * + * es-module-lexer 2 no longer models assertions, so it ends the statement at + * the specifier and the clause sits immediately after it. Matching is confined + * to the specifier's own line, which is what the withdrawn grammar required. + */ +const STATIC_ASSERT_KEYWORD = /[^\S\r\n]*assert(?=[\s{])/y; + +/** The legacy `assert` key opening the options argument of a dynamic import. */ +const DYNAMIC_ASSERT_KEY = /^\s*,\s*\{\s*assert(?=\s*:)/; + +/** A `with` clause on a static import, capturing the attribute object. */ +const STATIC_WITH_CLAUSE = /^\s*with\s*(\{[^{}]*\})\s*$/; + +/** The options argument of a dynamic import, capturing the attribute object. */ +const DYNAMIC_WITH_ARGUMENT = /^\s*,\s*\{\s*with\s*:\s*(\{[^{}]*\})\s*,?\s*\}\s*$/; + +/** An attribute object declaring a JSON module type and nothing else. */ +const JSON_TYPE_ATTRIBUTE = /^\{\s*(["']?)type\1\s*:\s*(["'])json\2\s*,?\s*\}$/; + +/** + * The range holding everything a specifier declares after its own text: the + * `with` clause of a static import, or the options argument of a dynamic one. + * + * The range is empty when the import declares no attributes. + */ +function attributeRange(imp: ImportSpecifier): { start: number; end: number } { + // Static: `e` indexes the closing quote and `se` ends the statement before + // any semicolon, so the clause and its keyword lie between them. + if (imp.d === -1) return { start: imp.e + 1, end: imp.se }; + + // Dynamic: `e` is already past the closing quote and `se` is past the + // closing paren, so the comma and the options argument lie between them. + return { start: imp.e, end: imp.se - 1 }; +} + +/** Whether the declared attributes are exactly a JSON module type. */ +function declaresJsonTypeOnly(clause: string, isDynamic: boolean): boolean { + const attributes = (isDynamic ? DYNAMIC_WITH_ARGUMENT : STATIC_WITH_CLAUSE).exec(clause)?.[1]; + return attributes !== undefined && JSON_TYPE_ATTRIBUTE.test(attributes); +} + +/** Position of the legacy `assert` keyword this import uses, or `null`. */ +function findAssertKeyword(masked: string, imp: ImportSpecifier): number | null { + if (imp.d === -1) { + STATIC_ASSERT_KEYWORD.lastIndex = imp.se; + const match = STATIC_ASSERT_KEYWORD.exec(masked); + return match === null ? null : STATIC_ASSERT_KEYWORD.lastIndex - ASSERT_KEYWORD.length; + } + + const { start, end } = attributeRange(imp); + if (start >= end) return null; + + const match = DYNAMIC_ASSERT_KEY.exec(masked.slice(start, end)); + return match === null ? null : start + match[0].length - ASSERT_KEYWORD.length; +} + +/** + * Rewrite the withdrawn `assert` spelling of an import attribute clause to + * `with`, for both the static and the dynamic form. + * + * esbuild treats `import-assertions` as a feature separate from + * `import-attributes` and drops the clause when the configured target does not + * claim it, which turns a working JSON import into "Attempted to load JSON + * module without specifying \"type\": \"json\"" at load time. Preserving the + * clause verbatim is no better: Node 22 and Deno 2 removed the keyword, so the + * only output that runs anywhere is `with`. + */ +export async function upgradeImportAssertions(code: string): Promise { + if (!code.includes(ASSERT_KEYWORD)) return code; + + const { masked, imports, unmask } = await parseMaskedImports(code); + let result = masked; + + for (let i = imports.length - 1; i >= 0; i--) { + const imp = imports[i]; + if (!imp?.n) continue; + + const keyword = findAssertKeyword(masked, imp); + if (keyword === null) continue; + + result = result.slice(0, keyword) + "with" + result.slice(keyword + ASSERT_KEYWORD.length); + } + + return unmask(result); +} + +/** + * Drop `with { type: "json" }` from every import whose specifier the caller + * accepts, leaving all other attributes in place. + * + * Only a lone JSON type attribute is removed. An import that declares anything + * else is describing something this rewrite knows nothing about, so it is left + * exactly as written. + */ +export async function stripJsonImportAttributes( + code: string, + shouldStrip: (specifier: string) => boolean, +): Promise { + if (!code.includes("with")) return code; + + const { masked, imports, unmask } = await parseMaskedImports(code); + let result = masked; + + for (let i = imports.length - 1; i >= 0; i--) { + const imp = imports[i]; + if (!imp?.n) continue; + if (!shouldStrip(unmask(imp.n))) continue; + + const { start, end } = attributeRange(imp); + if (start >= end) continue; + if (!declaresJsonTypeOnly(masked.slice(start, end), imp.d > -1)) continue; + + result = result.slice(0, start) + result.slice(end); + } + + return unmask(result); +} diff --git a/src/transforms/esm/import-parser.test.ts b/src/transforms/esm/import-parser.test.ts index f76616f99a..7b1a72c4f5 100644 --- a/src/transforms/esm/import-parser.test.ts +++ b/src/transforms/esm/import-parser.test.ts @@ -15,6 +15,10 @@ import { rewriteBodyImports } from "../mdx/compiler/import-rewriter.ts"; * for the target it was given. That rewrite is the whole point: at the "server" * target it turns `./Child.tsx` into an absolute `file://` URL, and a stub that * skipped it could not see what the parser does with the result. + * + * `compileMarkdown` mirrors the real extension too: Markdown becomes a fixed + * template whose only import is the bare JSX runtime, so a `.md` file can never + * contribute a dependency and must not be compiled to find that out. */ function withStubContentProcessor(): { calls: string[]; restore: () => void } { const calls: string[] = []; @@ -37,6 +41,17 @@ function withStubContentProcessor(): { calls: string[]; restore: () => void } { frontmatter: undefined, }); }, + compileMarkdown: (opts: Record) => { + calls.push(String(opts.filePath ?? "")); + + return Promise.resolve({ + compiledCode: [ + `import { jsx as _jsx } from "react/jsx-runtime";`, + `export default function MDContent() { return _jsx("div", {}); }`, + ].join("\n"), + frontmatter: undefined, + }); + }, }); return { calls, restore: () => unregister("ContentProcessor") }; } @@ -178,6 +193,71 @@ describe("transforms/esm/import-parser", () => { } }); + // Regression: an extensionless specifier is the common shape in real MDX, and + // the rewritten absolute URL carries no extension either. Resolving it with a + // bare existence check reported a file that exists as a missing dependency. + it("resolves an extensionless sibling an .mdx file imports", async () => { + const stub = withStubContentProcessor(); + try { + await withProject( + { + "components/snippet.mdx": `import Card from "./Card";\n\n\n`, + "components/Card.tsx": `export default () => null;`, + }, + async (projectDir) => { + const adapter = await getLocalAdapter(); + const filePath = join(projectDir, "components/snippet.mdx"); + const result = await parseLocalImports( + await Deno.readTextFile(filePath), + filePath, + projectDir, + adapter, + ); + + assertEquals(result.missing.length, 0, "an existing file must not be reported missing"); + assertEquals( + result.imports.some((imp) => imp.absolutePath.endsWith("components/Card.tsx")), + true, + "the extension ladder must find the sibling", + ); + }, + ); + } finally { + stub.restore(); + } + }); + + it("resolves a directory-index sibling an .mdx file imports", async () => { + const stub = withStubContentProcessor(); + try { + await withProject( + { + "components/snippet.mdx": `import { Ui } from "./ui";\n\n\n`, + "components/ui/index.tsx": `export const Ui = () => null;`, + }, + async (projectDir) => { + const adapter = await getLocalAdapter(); + const filePath = join(projectDir, "components/snippet.mdx"); + const result = await parseLocalImports( + await Deno.readTextFile(filePath), + filePath, + projectDir, + adapter, + ); + + assertEquals(result.missing.length, 0, "an existing file must not be reported missing"); + assertEquals( + result.imports.some((imp) => imp.absolutePath.endsWith("components/ui/index.tsx")), + true, + "the index ladder must find the directory entry point", + ); + }, + ); + } finally { + stub.restore(); + } + }); + it("tracks a stylesheet an .mdx file imports relatively", async () => { const stub = withStubContentProcessor(); try { @@ -227,7 +307,22 @@ describe("transforms/esm/import-parser", () => { assertEquals(result.imports.length, 0); assertEquals(result.missing.length, 1, "a dropped import must be reported, not silent"); - assertEquals(result.missing[0]?.reason.includes("Missing.tsx"), true); + + // The report reaches users verbatim in the "Component has missing + // dependencies" build error, so it names what the author wrote, not + // where the server happened to put the project. + const missing = result.missing[0]; + assertEquals(missing?.specifier, "./Missing.tsx"); + assertEquals( + `${missing?.specifier} ${missing?.reason}`.includes(projectDir), + false, + "a server path must not reach the user-facing report", + ); + assertEquals( + `${missing?.specifier} ${missing?.reason}`.includes("file://"), + false, + "an internal file URL must not reach the user-facing report", + ); }, ); } finally { @@ -259,6 +354,75 @@ describe("transforms/esm/import-parser", () => { } }); + // Dependency parsing runs on every render, including cache hits. Markdown + // compiles to a fixed template whose only import is the bare JSX runtime, so + // the answer is always "no dependencies" and the compile is pure cost. + it("answers for a .md file without invoking the compiler", async () => { + const stub = withStubContentProcessor(); + try { + await withProject( + { "content/post.md": `# Heading\n\nProse with a [link](https://example.com).\n` }, + async (projectDir) => { + const adapter = await getLocalAdapter(); + const filePath = join(projectDir, "content/post.md"); + const result = await parseLocalImports( + await Deno.readTextFile(filePath), + filePath, + projectDir, + adapter, + ); + + assertEquals(result.imports.length, 0); + assertEquals(result.cssImports.length, 0); + assertEquals(result.missing.length, 0); + assertEquals(stub.calls.length, 0, "Markdown must not be compiled to parse its imports"); + }, + ); + } finally { + stub.restore(); + } + }); + + // Dependency parsing runs on every render, so an uncached compile per render + // per MDX file is paid on every cache hit, recursively. + it("compiles unchanged .mdx content once across repeated parses", async () => { + const stub = withStubContentProcessor(); + try { + await withProject( + { + "components/snippet.mdx": `import Card from "./Card.tsx";\n\n\n`, + "components/Card.tsx": `export default () => null;`, + }, + async (projectDir) => { + const adapter = await getLocalAdapter(); + const filePath = join(projectDir, "components/snippet.mdx"); + const code = await Deno.readTextFile(filePath); + + const first = await parseLocalImports(code, filePath, projectDir, adapter); + const second = await parseLocalImports(code, filePath, projectDir, adapter); + + assertEquals(stub.calls.length, 1, "a repeat parse must reuse the compiled output"); + assertEquals(first.imports.length, 1); + assertEquals(second.imports.length, 1); + assertEquals(second.imports[0]?.absolutePath, first.imports[0]?.absolutePath); + + // Edited content must never be answered from the previous compile. + const edited = `import Other from "./Other.tsx";\n\n\n`; + await Deno.writeTextFile(join(projectDir, "components/Other.tsx"), `export default 1;`); + const third = await parseLocalImports(edited, filePath, projectDir, adapter); + + assertEquals(stub.calls.length, 2, "changed content must be compiled again"); + assertEquals( + third.imports.some((imp) => imp.absolutePath.endsWith("components/Other.tsx")), + true, + ); + }, + ); + } finally { + stub.restore(); + } + }); + it("short-circuits .css and .json without invoking the compiler", async () => { await withProject({}, async (projectDir) => { const adapter = await getLocalAdapter(); diff --git a/src/transforms/esm/import-parser.ts b/src/transforms/esm/import-parser.ts index 6cce77732f..f8e1f9caca 100644 --- a/src/transforms/esm/import-parser.ts +++ b/src/transforms/esm/import-parser.ts @@ -1,6 +1,8 @@ -import { compileContent } from "../mdx/compiler/index.ts"; +import { compileContent } from "#veryfront/transforms/mdx/compiler/index.ts"; import { getEsbuild } from "#veryfront/platform/compat/esbuild.ts"; -import { join } from "#veryfront/compat/path"; +import { dirname, join, relative } from "#veryfront/compat/path"; +import { computeHash } from "#veryfront/utils/hash-utils.ts"; +import { LRUCache } from "#veryfront/utils/lru-wrapper.ts"; import { createFileSystem } from "#veryfront/platform/compat/fs.ts"; import { isFrameworkSourcePath, @@ -39,32 +41,63 @@ interface ParseLocalImportsResult { const EXTENSIONS = [".tsx", ".ts", ".jsx", ".js", ".mdx"]; const HAS_EXTENSION_RE = /\.(tsx?|jsx?|mjs|cjs|mdx|css)$/; +/** + * Compiled MDX, keyed by project, file and content hash. + * + * Dependency parsing runs on every render, including every memory, Redis and + * MDX-ESM cache hit, and recurses through the dependency tree. Without this the + * full remark/rehype compile of every MDX file is paid again on each of them, + * for a result that cannot change while the content does not. + */ +const COMPILED_MDX_CACHE_MAX_ENTRIES = 200; +const compiledMdxCache = new LRUCache({ + maxEntries: COMPILED_MDX_CACHE_MAX_ENTRIES, +}); + +async function compileMdxForParsing( + code: string, + filePath: string, + projectDir: string, +): Promise { + const cacheKey = `${projectDir}::${filePath}::${await computeHash(code)}`; + const cached = compiledMdxCache.get(cacheKey); + if (cached !== undefined) return cached; + + const compiled = await compileContent( + "development", + projectDir, + code, + undefined, + filePath, + "server", + ); + + compiledMdxCache.set(cacheKey, compiled.compiledCode); + return compiled.compiledCode; +} + export async function parseLocalImports( code: string, filePath: string, projectDir: string, adapter?: RuntimeAdapter, ): Promise { - if (filePath.endsWith(".css") || filePath.endsWith(".json")) { + // Markdown compiles to a fixed template whose only import is the bare JSX + // runtime, which this parser discards, so the answer for a `.md` file is + // always "no dependencies". Compiling one to learn that is pure cost on a + // path that runs per render. + if (filePath.endsWith(".css") || filePath.endsWith(".json") || /\.md$/i.test(filePath)) { return { imports: [], cssImports: [], crossProjectImports: [], missing: [] }; } - // MDX/Markdown is not JSX, so handing the raw source to esbuild under the - // `jsx` loader fails with ":1:1: ERROR: Syntax error" — which surfaced - // to users as "Component has missing dependencies" for a file that exists. - // Compile content to JSX first, exactly as the transform pipeline's parse - // stage does, then read the imports out of that. + // MDX is not JSX, so handing the raw source to esbuild under the `jsx` loader + // fails with ":1:1: ERROR: Syntax error", which surfaced to users as + // "Component has missing dependencies" for a file that exists. Compile + // content to JSX first, exactly as the transform pipeline's parse stage does, + // then read the imports out of that. let parseSource = code; - if (/\.mdx?$/i.test(filePath)) { - const compiled = await compileContent( - "development", - projectDir, - code, - undefined, - filePath, - "server", - ); - parseSource = compiled.compiledCode; + if (/\.mdx$/i.test(filePath)) { + parseSource = await compileMdxForParsing(code, filePath, projectDir); } const esbuild = await getEsbuild(); @@ -96,19 +129,24 @@ export async function parseLocalImports( // below and are dropped without even being reported as missing, so an MDX // file's sibling components are never recursively transformed. if (specifier.startsWith("file://")) { - const absolutePath = fileUrlToPath(specifier); + const targetPath = fileUrlToPath(specifier); + // A rewritten specifier carries a server path the author never wrote, and + // this record is read back verbatim in the "Component has missing + // dependencies" build error. Report what the author wrote instead. + const authoredSpecifier = toAuthoredSpecifier(targetPath, specifier, filePath); + const resolved = targetPath ? await resolveExistingFilePath(targetPath, adapter) : null; - if (absolutePath && await checkFileExists(absolutePath, adapter)) { - const entry = { specifier, absolutePath }; - if (absolutePath.endsWith(".css")) cssImports.push(entry); + if (resolved) { + const entry = { specifier: authoredSpecifier, absolutePath: resolved }; + if (resolved.endsWith(".css")) cssImports.push(entry); else localImports.push(entry); continue; } missingImports.push({ - specifier, + specifier: authoredSpecifier, fromFile: filePath, - reason: `File not found: ${absolutePath ?? specifier}`, + reason: `File not found: tried extensions ${EXTENSIONS.join(", ")}`, }); continue; } @@ -168,6 +206,22 @@ export async function parseLocalImports( return { imports: localImports, cssImports, crossProjectImports, missing: missingImports }; } +/** + * The specifier as the author most likely wrote it, reconstructed from the + * absolute path a compile step rewrote it to. Falls back to the file name when + * the URL cannot be read, so no server path escapes into a user-facing report. + */ +function toAuthoredSpecifier( + targetPath: string | null, + specifier: string, + fromFile: string, +): string { + if (!targetPath) return `./${specifier.slice(specifier.lastIndexOf("/") + 1)}`; + + const relativePath = relative(dirname(fromFile), targetPath); + return relativePath.startsWith(".") ? relativePath : `./${relativePath}`; +} + /** Filesystem path behind a `file://` specifier, or null when it is not one. */ function fileUrlToPath(specifier: string): string | null { try { @@ -205,8 +259,19 @@ async function resolveLocalImportPath( } const fromDir = fromFile.substring(0, fromFile.lastIndexOf("/")); - const basePath = resolveRelative(fromDir, importSpecifier); + return await resolveExistingFilePath(resolveRelative(fromDir, importSpecifier), adapter); +} +/** + * Path of the file a local import points at: the adapter's own resolution + * first, then the extension and directory-index probes. Every local import + * shape resolves through here, so an extensionless or directory specifier + * behaves the same however it reached this module. + */ +async function resolveExistingFilePath( + basePath: string, + adapter?: RuntimeAdapter, +): Promise { if (adapter?.fs.resolveFile) { try { const normalizedPath = basePath.replace(/^\/+/, ""); @@ -218,7 +283,7 @@ async function resolveLocalImportPath( } } - if (HAS_EXTENSION_RE.test(importSpecifier)) { + if (HAS_EXTENSION_RE.test(basePath)) { return (await checkFileExists(basePath, adapter)) ? basePath : null; } diff --git a/src/transforms/esm/lexer.ts b/src/transforms/esm/lexer.ts index 47def690eb..e561c9d095 100644 --- a/src/transforms/esm/lexer.ts +++ b/src/transforms/esm/lexer.ts @@ -114,6 +114,40 @@ export async function parseImports(code: string): Promise string; +} + +/** + * Parse imports and hand back the masked source the positions belong to. + * + * Masking changes offsets, so `imp.s`, `imp.a` and friends are meaningless + * against the original text. Callers that splice by position must edit + * `masked` and run the result through `unmask`; callers that only need + * specifier names should use {@link parseImports} instead. + */ +export async function parseMaskedImports(code: string): Promise { + await initLexer(); + + const { masked, urlMap } = maskHttpUrls(code); + + let imports: readonly ImportSpecifier[]; + try { + imports = getLexer().parse(masked); + } catch (error) { + logParseError(error, masked); + throw error; + } + + return { masked, imports, unmask: (text) => unmaskHttpUrls(text, urlMap) }; +} + /** * Replace import specifiers (the path string) in the code. * Safe for simple re-mappings like aliases or rewriting URLs. diff --git a/src/transforms/esm/specifier-resolver.test.ts b/src/transforms/esm/specifier-resolver.test.ts index b9e7a216c8..230cbcf243 100644 --- a/src/transforms/esm/specifier-resolver.test.ts +++ b/src/transforms/esm/specifier-resolver.test.ts @@ -248,7 +248,7 @@ describe("transforms/esm/specifier-resolver", () => { }); it("aborts when a dynamic npm: specifier fails to resolve", async () => { - const code = `export const load = () => import("npm:redis");`; + const code = `export const load = () => import("npm:some-package");`; await assertRejects( () => buildReplacements(code, "https://esm.sh/parent@1/index.js", defaultOptions, async () => { @@ -259,6 +259,29 @@ describe("transforms/esm/specifier-resolver", () => { ); }); + it("leaves a server-only package external instead of routing it to esm.sh", async () => { + // `redis` and its explicit npm: form only run server-side. They must be + // left in place for the runtime to resolve (node_modules / npm:), never + // fetched from esm.sh — so the cache function is never called and nothing + // is degraded or aborted. + for (const specifier of ["redis", "npm:redis", "npm:redis@5.11.0"]) { + const code = `export const load = () => import(${JSON.stringify(specifier)});`; + let cacheCalls = 0; + const result = await buildReplacements( + code, + "https://esm.sh/parent@1/index.js", + defaultOptions, + async () => { + cacheCalls++; + return null; + }, + ); + assertEquals(cacheCalls, 0, `${specifier} must not hit esm.sh`); + assertEquals(result.replacements.size, 0, `${specifier} must be left in place`); + assertEquals(result.degraded, []); + } + }); + it("aborts when a dynamic bare specifier fails to resolve", async () => { const code = `export const load = () => import("some-package");`; await assertRejects( diff --git a/src/transforms/esm/specifier-resolver.ts b/src/transforms/esm/specifier-resolver.ts index 512944fb80..6229a619bf 100644 --- a/src/transforms/esm/specifier-resolver.ts +++ b/src/transforms/esm/specifier-resolver.ts @@ -10,6 +10,8 @@ import { basename } from "#veryfront/compat/path/index.ts"; import { resolveImport } from "#veryfront/modules/import-map/resolver.ts"; import { rendererLogger } from "#veryfront/utils"; +import { parseBarePackageSpecifier } from "../shared/package-specifier.ts"; +import { isServerOnlyPackage } from "../shared/server-only-packages.ts"; import { type ImportSpecifier, parseImports, replaceSpecifiers } from "./lexer.ts"; const logger = rendererLogger.component("specifier-resolver"); @@ -47,6 +49,17 @@ async function resolveSpecifier( ): Promise { if (isExternalScheme(specifier)) return null; + // Server-only packages (`redis`, `pg`, …), including their explicit `npm:` + // form, must never be routed through esm.sh. esm.sh either 500s building them + // or emits a browser bundle with Node built-ins stubbed that can never + // connect. The framework's adapters only `import()` them behind a lazy, + // configured code path, so leaving the specifier external lets the runtime + // resolve the real package (node_modules on Node, npm: on Deno) if and when + // the backend is actually used — and costs nothing when it is not. + const serverOnlyCandidate = specifier.startsWith("npm:") ? specifier.slice(4) : specifier; + const serverOnlyParsed = parseBarePackageSpecifier(serverOnlyCandidate); + if (serverOnlyParsed && isServerOnlyPackage(serverOnlyParsed.packageName)) return null; + if (isInternalBare(specifier)) { const mapped = resolveImport(specifier, options.importMap); if (mapped === specifier) return null; diff --git a/src/transforms/esm/transform-utils.ts b/src/transforms/esm/transform-utils.ts index 48a6b538e8..74f551a9cc 100644 --- a/src/transforms/esm/transform-utils.ts +++ b/src/transforms/esm/transform-utils.ts @@ -17,8 +17,18 @@ export function computeShortContentHash(content: string): Promise { * that consumes this output requires the attribute to load a JSON module, so * dropping it turns a working import into a load-time error: * `Attempted to load JSON module without specifying "type": "json"`. + * + * `import-assertions` is the separate feature key esbuild uses for the + * withdrawn `assert { type: "json" }` spelling of the same clause. It is + * enabled for the same reason, so the clause reaches the output instead of + * vanishing. The output must not keep that spelling though, because Node 22 + * and Deno 2 removed the keyword, so every consumer of these options runs the + * result through `upgradeImportAssertions` to rewrite it to `with`. */ -export const ESBUILD_SUPPORTED_FEATURES = { "import-attributes": true } as const; +export const ESBUILD_SUPPORTED_FEATURES = { + "import-attributes": true, + "import-assertions": true, +} as const; const EXTENSION_LOADERS: Record = { ".tsx": "tsx", diff --git a/src/transforms/import-rewriter/project-paths.ts b/src/transforms/import-rewriter/project-paths.ts new file mode 100644 index 0000000000..43228d6462 --- /dev/null +++ b/src/transforms/import-rewriter/project-paths.ts @@ -0,0 +1,53 @@ +/** + * Project-relative path helpers shared by the rewrite strategies. + * + * Several strategies need the path of the file being transformed expressed + * relative to the project root: the alias and relative strategies to build + * module URLs, the asset strategy to name the importer in its message. + */ + +/** Normalize separators and drop any trailing slash. */ +function normalizeDir(dir: string): string { + return dir.replace(/\\/g, "/").replace(/\/+$/, ""); +} + +/** + * Path of `filePath` relative to `projectDir`, or null when the file is not + * inside the project. + * + * The check is on a path boundary, so `/projectile/src/Header.tsx` is not + * treated as living inside `/project`. + */ +export function relativeToProjectDir(filePath: string, projectDir: string): string | null { + const normalizedFilePath = filePath.replace(/\\/g, "/"); + const normalizedProjectDir = normalizeDir(projectDir); + + if (normalizedFilePath === normalizedProjectDir) return ""; + if (!normalizedFilePath.startsWith(`${normalizedProjectDir}/`)) return null; + + return normalizedFilePath.slice(normalizedProjectDir.length + 1); +} + +/** + * Best-effort project-relative path, for callers that need a path to build a + * module URL from and have no useful fallback. + * + * A file outside the project can still be a copy of a project file staged + * somewhere else (a temp bundle directory, for example), so the project + * directory name is looked up in the path before giving up and returning the + * input unchanged. + */ +export function getProjectRelativePath(filePath: string, projectDir: string): string { + const direct = relativeToProjectDir(filePath, projectDir); + if (direct !== null) return direct; + + if (!filePath.startsWith("/")) return filePath; + + const pathParts = filePath.split("/"); + const lastProjectPart = normalizeDir(projectDir).split("/").at(-1); + const projectIndex = lastProjectPart ? pathParts.indexOf(lastProjectPart) : -1; + + if (projectIndex >= 0) return pathParts.slice(projectIndex + 1).join("/"); + + return filePath; +} diff --git a/src/transforms/import-rewriter/strategies/alias-strategy.test.ts b/src/transforms/import-rewriter/strategies/alias-strategy.test.ts index 5ec305e137..c0082fc7d0 100644 --- a/src/transforms/import-rewriter/strategies/alias-strategy.test.ts +++ b/src/transforms/import-rewriter/strategies/alias-strategy.test.ts @@ -209,7 +209,10 @@ describe("AliasStrategy", () => { assertEquals(result.specifier, "../../components/Badge.js"); }); - it("should not double-append .js to an explicit .js extension", () => { + // Guard, not a regression: `jsx?` in the extension guard has always + // matched a bare `.js`, so this has never produced `.js.js`. The test + // holds that behaviour in place. + it("should leave an explicit .js extension alone", () => { const result = aliasStrategy.rewrite( makeInfo("@/lib/legacy.js"), makeCtx({ filePath: "/project/pages/index.tsx" }), diff --git a/src/transforms/import-rewriter/strategies/alias-strategy.ts b/src/transforms/import-rewriter/strategies/alias-strategy.ts index 32839ab3c0..75b1e03ff6 100644 --- a/src/transforms/import-rewriter/strategies/alias-strategy.ts +++ b/src/transforms/import-rewriter/strategies/alias-strategy.ts @@ -5,6 +5,7 @@ import type { RewriteResult, } from "../types.ts"; import { normalizeExtension } from "../url-builder.ts"; +import { getProjectRelativePath } from "../project-paths.ts"; export class AliasStrategy implements ImportRewriteStrategy { readonly name = "alias"; @@ -21,7 +22,7 @@ export class AliasStrategy implements ImportRewriteStrategy { if (ctx.target === "ssr") { let normalizedPath = normalizeExtension(path); // Add .js if no extension present - if (!/\.(tsx?|jsx?|mjs|cjs|mdx|css|js)$/.test(normalizedPath)) { + if (!/\.(tsx?|jsx?|mjs|cjs|mdx|css)$/.test(normalizedPath)) { normalizedPath = `${normalizedPath}.js`; } return { specifier: `/_vf_modules/${normalizedPath}` }; @@ -33,7 +34,7 @@ export class AliasStrategy implements ImportRewriteStrategy { // but module path is "_vf_modules/components/elements/Textarea.js"). if (ctx.moduleServerUrl) { let normalizedPath = normalizeExtension(path); - if (!/\.(tsx?|jsx?|mjs|cjs|mdx|css|js)$/.test(normalizedPath)) { + if (!/\.(tsx?|jsx?|mjs|cjs|mdx|css)$/.test(normalizedPath)) { normalizedPath = `${normalizedPath}.js`; } return { specifier: `${ctx.moduleServerUrl}/${normalizedPath}` }; @@ -41,40 +42,19 @@ export class AliasStrategy implements ImportRewriteStrategy { // Fallback: Use relative paths when no module server is configured. // This is used for local development without a module server. - const relativeFilePath = this.getRelativeFilePath(ctx.filePath, ctx.projectDir); + const relativeFilePath = getProjectRelativePath(ctx.filePath, ctx.projectDir); const fileDir = relativeFilePath.substring(0, relativeFilePath.lastIndexOf("/")); const depth = fileDir.split("/").filter(Boolean).length; const prefix = depth === 0 ? "./" : "../".repeat(depth); let relativePath = normalizeExtension(`${prefix}${path}`); - if (!/\.(tsx?|jsx?|mjs|cjs|mdx|css|js)$/.test(relativePath)) { + if (!/\.(tsx?|jsx?|mjs|cjs|mdx|css)$/.test(relativePath)) { relativePath = `${relativePath}.js`; } return { specifier: relativePath }; } - - private getRelativeFilePath(filePath: string, projectDir: string): string { - const normalizedProjectDir = projectDir.replace(/\\/g, "/").replace(/\/$/, ""); - - if (filePath.startsWith(normalizedProjectDir)) { - return filePath.substring(normalizedProjectDir.length + 1); - } - - if (!filePath.startsWith("/")) return filePath; - - const pathParts = filePath.split("/"); - const projectParts = normalizedProjectDir.split("/"); - const lastProjectPart = projectParts.at(-1); - const projectIndex = lastProjectPart ? pathParts.indexOf(lastProjectPart) : -1; - - if (projectIndex >= 0) { - return pathParts.slice(projectIndex + 1).join("/"); - } - - return filePath; - } } export const aliasStrategy = new AliasStrategy(); diff --git a/src/transforms/import-rewriter/strategies/asset-strategy.test.ts b/src/transforms/import-rewriter/strategies/asset-strategy.test.ts index 69f3678f0e..b61edd2c4a 100644 --- a/src/transforms/import-rewriter/strategies/asset-strategy.test.ts +++ b/src/transforms/import-rewriter/strategies/asset-strategy.test.ts @@ -59,6 +59,47 @@ describe("AssetStrategy", () => { assertEquals(assetStrategy.matches("@/assets/logo.svg?raw", makeCtx()), true); }); + it("matches the non-code file types a project is most likely to import", () => { + for ( + const specifier of [ + "@/wasm/mod.wasm", + "@/native/addon.node", + "@/media/clip.mov", + "@/content/notes.txt", + "@/config/settings.yaml", + "@/config/settings.yml", + "@/data/rows.csv", + ] + ) { + assertEquals(assetStrategy.matches(specifier, makeCtx()), true, specifier); + } + }); + + it("leaves JSON alone, which is importable with an import attribute", () => { + // `import manifest from "./manifest.json" with { type: "json" }` is + // supported and the compile stage preserves the attribute. matches() + // sees only the specifier, so it cannot tell the two apart. + assertEquals(assetStrategy.matches("@/data/config.json", makeCtx()), false); + assertEquals(assetStrategy.matches("./manifest.json", makeCtx()), false); + }); + + it("only claims specifiers the alias and relative strategies would resolve", () => { + // Any other strategy owning the specifier knows where the file lives. + // "Move it to public/" is not actionable for a file inside a dependency + // or on another host, and the URL strategy already handles remote assets. + for ( + const specifier of [ + "leaflet/dist/images/marker-icon.png", + "https://cdn.example.com/icons/logo.svg", + "http://cdn.example.com/icons/logo.svg", + "veryfront/assets/logo.svg", + "otherproject@1.0.0/@/assets/logo.svg", + ] + ) { + assertEquals(assetStrategy.matches(specifier, makeCtx()), false, specifier); + } + }); + it("does not match code modules", () => { for ( const specifier of ["@/lib/constants", "./Button.tsx", "react", "@/lib/svg-utils.ts"] @@ -86,15 +127,47 @@ describe("AssetStrategy", () => { assertStringIncludes(message, "docs/guides/project-structure.md"); }); - it("drops query strings from the suggested public filename", () => { + it("keeps the directories under the alias root so two logos stay distinct", () => { + const message = messageFromRewrite("@/assets/icons/logo.svg", makeCtx()); + assertStringIncludes(message, "public/icons/logo.svg"); + assertStringIncludes(message, ''); + }); + + it("keeps the directories a relative specifier walks into", () => { + const message = messageFromRewrite("../images/photo.jpeg", makeCtx()); + assertStringIncludes(message, "public/images/photo.jpeg"); + }); + + it("drops a query suffix from the suggested destination", () => { + // Otherwise the advice reads "move the file to public/logo.svg?raw", + // which names a file nobody can create. const message = messageFromRewrite("@/assets/logo.svg?raw", makeCtx()); - assertStringIncludes(message, "@/assets/logo.svg?raw"); assertStringIncludes(message, "public/logo.svg"); - assertStringIncludes(message, ''); - assertEquals(message.includes("public/logo.svg?raw"), false); + assertEquals(message.includes("logo.svg?raw and"), false); assertEquals(message.includes('src="/logo.svg?raw"'), false); }); + it("suggests a stylesheet rule for a font, not an image tag", () => { + const message = messageFromRewrite("@/fonts/Inter.woff2", makeCtx()); + assertStringIncludes(message, "public/Inter.woff2"); + assertStringIncludes(message, "@font-face"); + assertStringIncludes(message, 'url("/Inter.woff2")'); + assertEquals(message.includes(" { + const message = messageFromRewrite("@/media/clip.mp4", makeCtx()); + assertStringIncludes(message, '