Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion cli/commands/build/command.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import "#veryfront/schemas/_test-setup.ts";
import { assertEquals, assertExists, assertRejects } from "#veryfront/testing/assert.ts";
import { describe, it } from "#veryfront/testing/bdd.ts";
import { buildCommand, formatBuildOutputPath, runWithBundlerShutdown } from "./command.ts";
import {
buildCommand,
formatBuildOutputPath,
releaseBuildExtensions,
runWithBundlerShutdown,
} from "./command.ts";
import type { BuildOptions } from "./types.ts";

describe("commands/build/command", () => {
Expand Down Expand Up @@ -55,6 +60,33 @@ describe("commands/build/command", () => {
});
});

describe("releaseBuildExtensions", () => {
it("tears down the composed extensions", async () => {
let torndown = 0;
await releaseBuildExtensions({
teardownAll: () => {
torndown++;
return Promise.resolve();
},
});
assertEquals(torndown, 1);
});

it("does nothing when no extensions were composed", async () => {
// The build can fail before composition, so the release path runs with
// nothing to release and must not throw.
await releaseBuildExtensions(undefined);
});

it("does not let a teardown failure change the build outcome", async () => {
// The build has already produced its result. runWithBundlerShutdown sets
// the same precedent by preserving the build error over a shutdown one.
await releaseBuildExtensions({
teardownAll: () => Promise.reject(new Error("teardown exploded")),
});
});
});

describe("formatBuildOutputPath", () => {
it("reports the default output relative to the project", () => {
assertEquals(
Expand Down
45 changes: 44 additions & 1 deletion cli/commands/build/command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { displayBuildSuccess } from "./stats-display.ts";
import type { BuildOptions } from "./types.ts";
import { isJsonMode, streamJsonLine } from "../../shared/json-output.ts";
import { ensureBuiltinContentProcessor } from "../../shared/ensure-content-processor.ts";
import { setupBuildCliExtensions } from "../../shared/build-extensions.ts";

/** @internal */
export async function runWithBundlerShutdown<T>(
Expand Down Expand Up @@ -37,6 +38,33 @@ export async function runWithBundlerShutdown<T>(
return result;
}

/**
* Release the extensions composed for this build.
*
* Extensions can hold timers and other resources, and `teardownAll()` also
* clears the process-global contract registry that `orchestrateExtensions`
* populated. `veryfront eval` and `veryfront serve` already do this; the build
* did not, so a command that composes extensions left them running.
*
* A teardown failure never changes the build's outcome. The build has already
* produced its result by this point, and `runWithBundlerShutdown` sets the
* same precedent by preserving the build error over a shutdown one.
*
* @internal
*/
export async function releaseBuildExtensions(
loader: { teardownAll: () => Promise<void> } | undefined,
): Promise<void> {
if (!loader) return;
try {
await loader.teardownAll();
} catch {
if (!isJsonMode()) {
cliLogger.warn("Extension teardown failed after the build");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use the required public warning style.

When JSON mode is disabled, Line 63 emits user-facing copy that does not address the reader. Use direct copy such as Veryfront cannot release your build extensions after this build.

As per coding guidelines, use direct, concise, present-tense, active-voice public copy and address the reader as you.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/commands/build/command.ts` at line 63, Update the warning message in the
build command’s extension teardown failure path to use concise, present-tense,
active-voice copy that addresses the reader as “you,” while preserving the
existing JSON-mode behavior.

Source: Coding guidelines

}
}
}

export function formatBuildOutputPath(projectDir: string, outputDir: string): string {
return relative(projectDir, resolve(projectDir, outputDir)).replace(/\\/g, "/");
}
Expand All @@ -48,6 +76,14 @@ export function buildCommand(options: BuildOptions): Promise<void> {
const outputDir = options.outputDir ?? join(options.projectDir, "dist");
const startTime = Date.now();
const dryRun = options.dryRun ?? false;
let extensions: Awaited<ReturnType<typeof setupBuildCliExtensions>> | undefined;
// exit() does not run `finally`, so the JSON error path below releases
// explicitly before exiting. Clearing the handle keeps that idempotent.
const releaseExtensions = async (): Promise<void> => {
const loader = extensions;
extensions = undefined;
await releaseBuildExtensions(loader);
};

try {
if (isJsonMode()) {
Expand All @@ -58,7 +94,11 @@ export function buildCommand(options: BuildOptions): Promise<void> {

const stats = await runWithBundlerShutdown(async () => {
const adapter = await runtime.get();
await getConfig(options.projectDir, adapter);
const config = await getConfig(options.projectDir, adapter);
// Compose the project's extensions before anything that resolves a
// contract. Only server bootstrap used to do this, so the build ran
// with whatever one-off shims had been added and failed on the rest.
extensions = await setupBuildCliExtensions(options.projectDir, config);
await ensureBuiltinContentProcessor();

if (isJsonMode()) {
Expand Down Expand Up @@ -121,11 +161,14 @@ export function buildCommand(options: BuildOptions): Promise<void> {
success: false,
error: error instanceof Error ? error.message : String(error),
});
await releaseExtensions();
const { exit } = await import("veryfront/platform");
exit(1);
return;
}
handleBuildError(error);
} finally {
await releaseExtensions();
}
},
{ "cli.projectDir": options.projectDir },
Expand Down
91 changes: 91 additions & 0 deletions cli/shared/build-extensions.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import "#veryfront/schemas/_test-setup.ts";
import { assertEquals, assertExists } from "#veryfront/testing/assert.ts";
import { describe, it } from "#veryfront/testing/bdd.ts";
import { setupBuildCliExtensions } from "./build-extensions.ts";

/** Minimal stand-in for the loader; the build path only needs it to resolve. */
const loaderStub = {} as Awaited<ReturnType<typeof setupBuildCliExtensions>>;

/**
* Deferred builtins do not declare their contracts until they load, so identity
* is what a caller can assert on before orchestration runs.
*/
function extensionNames(
builtins: readonly { extension: { name: string } }[],
): Set<string> {
return new Set(builtins.map((builtin) => builtin.extension.name));
}

describe("cli/shared/build-extensions", () => {
it("composes the project's configured extensions", async () => {
let seen: { projectDir?: string; config?: unknown } = {};
// A full Extension entry: ExtensionConfigEntry only admits an Extension or
// an explicit { name, enabled: false } disable.
const config = {
extensions: [{ name: "ext-css-lightning", version: "1.0.0", capabilities: [] }],
};

await setupBuildCliExtensions("/projects/app", config, (options) => {
seen = { projectDir: options.projectDir, config: options.config };
return Promise.resolve(loaderStub);
});

assertEquals(seen.projectDir, "/projects/app");
Comment on lines +28 to +33

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove absolute fixture paths from the tests.

These stubs only forward projectDir. Use a relative fixture identifier and update the expected value.

Proposed fix
-    await setupBuildCliExtensions("/projects/app", config, (options) => {
+    await setupBuildCliExtensions("test-project", config, (options) => {
       seen = { projectDir: options.projectDir, config: options.config };
       return Promise.resolve(loaderStub);
     });

-    assertEquals(seen.projectDir, "/projects/app");
+    assertEquals(seen.projectDir, "test-project");

-    await setupBuildCliExtensions("/projects/app", {}, (options) => {
+    await setupBuildCliExtensions("test-project", {}, (options) => {
-    await setupBuildCliExtensions("/projects/app", {}, (options) => {
+    await setupBuildCliExtensions("test-project", {}, (options) => {
-    await setupBuildCliExtensions("/projects/app", {}, (options) => {
+    await setupBuildCliExtensions("test-project", {}, (options) => {
-      "/projects/app",
+      "test-project",

As per coding guidelines, never place local absolute paths in tests.

Also applies to: 41-41, 54-54, 71-71, 83-85

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/shared/build-extensions.test.ts` around lines 28 - 33, Update the
setupBuildCliExtensions test cases and their expected assertions to use a
relative fixture identifier instead of the absolute “/projects/app” path. Apply
the same change to the additional occurrences noted in the comment, while
preserving the existing projectDir forwarding behavior.

Source: Coding guidelines

// The build must honor what the project declares, not a hardcoded default.
assertEquals(seen.config, config);
});

it("offers the CSSProcessor provider among the builtins", async () => {
let builtins: readonly { extension: { name: string } }[] = [];

await setupBuildCliExtensions("/projects/app", {}, (options) => {
builtins = options.builtinExtensions ?? [];
return Promise.resolve(loaderStub);
});

// Without this, `veryfront build` reaches the release-asset CSS compile with
// no CSSProcessor registered and fails with "Missing extension for contract".
assertEquals(extensionNames(builtins).has("ext-css-tailwind"), true);
});

it("offers the bundler and content providers the build also needs", async () => {
let builtins: readonly { extension: { name: string } }[] = [];

await setupBuildCliExtensions("/projects/app", {}, (options) => {
builtins = options.builtinExtensions ?? [];
return Promise.resolve(loaderStub);
});

const names = extensionNames(builtins);
assertEquals(names.has("ext-bundler-esbuild"), true);
assertEquals(names.has("ext-content-mdx"), true);
});

it("hands orchestration a logger it can actually log through", async () => {
// Not "names the build": cliLogger.component() deliberately returns the
// same logger, because CLI output carries no structured component tag. So
// there is no attribution to assert on, only that orchestration receives
// something usable. `typeof x === "object"` alone would also accept null.
let logger: Record<string, unknown> | undefined;

await setupBuildCliExtensions("/projects/app", {}, (options) => {
logger = options.logger as unknown as Record<string, unknown>;
return Promise.resolve(loaderStub);
});

assertExists(logger);
for (const method of ["debug", "info", "warn", "error"] as const) {
assertEquals(typeof logger[method], "function", `logger.${method} must be callable`);
}
});

it("returns the composed loader to the caller", async () => {
const result = await setupBuildCliExtensions(
"/projects/app",
{},
() => Promise.resolve(loaderStub),
);

assertEquals(result, loaderStub);
});
});
51 changes: 51 additions & 0 deletions cli/shared/build-extensions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/**
* Extension composition for `veryfront build`.
*
* Extension orchestration used to happen only in server bootstrap, so commands
* that never start a server ran with an almost empty contract registry. The
* build path compensated with one-off shims (`ensureCliBundlerContracts`,
* `ensureBuiltinContentProcessor`) that covered the contracts someone had
* already been bitten by, and nothing else.
*
* CSS was the contract nobody had added a shim for. A scaffolded project whose
* stylesheet is `@import "tailwindcss";` reached the release-asset CSS compile
* with no CSSProcessor registered and failed with:
*
* Missing extension for contract "CSSProcessor".
* Install it with: deno add @veryfront/ext-css-tailwind
*
* The extension was installed the whole time. `veryfront dev` compiled the same
* stylesheet correctly because starting a server orchestrated it.
*
* Composing extensions here fixes that class of failure rather than one
* instance of it, and honors what the project configures: a project that
* declares `ext-css-lightning` gets its own processor instead of whichever one
* a shim happened to hardcode. `veryfront eval` already does this.
*
* @module cli/shared/build-extensions
*/

import { orchestrateExtensions } from "veryfront/extensions";
import { cliLogger } from "#cli/utils";
import { createBuiltinExtensions } from "../../src/extensions/builtin-extensions.ts";

type OrchestrateExtensions = typeof orchestrateExtensions;
type OrchestrateOptions = Parameters<OrchestrateExtensions>[0];

/**
* Compose the extensions a production build needs.
*
* `orchestrate` is a test seam and defaults to the real implementation.
*/
export async function setupBuildCliExtensions(
projectDir: string,
config: OrchestrateOptions["config"],
orchestrate: OrchestrateExtensions = orchestrateExtensions,
): Promise<Awaited<ReturnType<OrchestrateExtensions>>> {
return await orchestrate({
projectDir,
config,
logger: cliLogger.component("build-extensions"),
builtinExtensions: createBuiltinExtensions(),
});
}