Skip to content
Merged
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
4 changes: 4 additions & 0 deletions cli/commands/build/command-help.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ export const buildHelp: CommandHelp = {
description: "Select build preset (e.g. embedded)",
},
],
notes: [
"--preset embedded emits a single bundle, so of the build flags it honours only -o/--output and build.outDir. Global flags such as --json, --verbose and --quiet are unaffected.",
"It rejects --dry-run, --split/--no-split, --compress/--no-compress, --prefetch, --ssg/--no-ssg, --include and --exclude rather than ignoring them.",
],
Comment thread
coderabbitai[bot] marked this conversation as resolved.
examples: [
"veryfront build",
"veryfront build --output dist",
Expand Down
32 changes: 32 additions & 0 deletions cli/commands/build/command.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -264,3 +264,35 @@ describe("commands/build/command", () => {
});
});
});

describe("cli/build resolveBuildOutputDir clearsOutputDir", () => {
// Raised in review on #3781. The guard exists because the production build
// removes its output directory first. The embedded preset only mkdir's and
// writes, so applying the guard there rejected `-o .` — a plausible call for
// a preset meant to be embedded in a host project — over a hazard that does
// not exist on that path.
it("rejects an output directory containing the project when the caller clears it", () => {
assertThrows(
() => resolveBuildOutputDir("/tmp/proj", "/tmp/proj", { build: {} }),
Error,
);
});

it("allows the same directory when the caller only writes into it", () => {
assertEquals(
resolveBuildOutputDir("/tmp/proj", "/tmp/proj", { build: {} }, {
clearsOutputDir: false,
}),
"/tmp/proj",
);
});

it("still honours build.outDir when the guard is opted out", () => {
assertEquals(
resolveBuildOutputDir("/tmp/proj", undefined, { build: { outDir: "custom" } }, {
clearsOutputDir: false,
}),
"/tmp/proj/custom",
);
});
});
17 changes: 13 additions & 4 deletions cli/commands/build/command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,15 @@ export function resolveBuildOutputDir(
projectDir: string,
explicitOutputDir: string | undefined,
config: Pick<VeryfrontConfig, "build">,
options: { clearsOutputDir?: boolean } = {},
): string {
const outputDir = explicitOutputDir ?? resolveConfiguredOutputDir(projectDir, config);
assertOutputDirExcludesProject(projectDir, outputDir, explicitOutputDir !== undefined);
// The guard below exists because the caller wipes the directory first. A
// caller that only writes into it is not dangerous, and rejecting it would
// block legitimate invocations — see the embedded preset.
if (options.clearsOutputDir !== false) {
assertOutputDirExcludesProject(projectDir, outputDir, explicitOutputDir !== undefined);
}
return outputDir;
}

Expand All @@ -102,9 +108,12 @@ function resolveConfiguredOutputDir(
/**
* Refuse an output directory that is the project directory or an ancestor of it.
*
* The build clears its output directory before writing, so an `outDir` of `.`
* or `..` would recursively delete the project's own source — or the workspace
* above it. That was unreachable while `build.outDir` was ignored; now that the
* The production build clears its output directory before writing
* (`build-setup.ts` removes it recursively), so an `outDir` of `.` or `..`
* would recursively delete the project's own source — or the workspace above
* it. This is why the check is opt-out via `clearsOutputDir`: a caller that
* only writes into the directory, like the embedded preset, carries no such
* risk and must not be blocked. That was unreachable while `build.outDir` was ignored; now that the
* config value is honored, a stale compatibility-era config could reach it.
* Failing loudly is the only safe answer: silently substituting `dist` would
* reintroduce the ignored-configuration bug this change exists to fix.
Expand Down
155 changes: 155 additions & 0 deletions cli/commands/build/embedded-preset-flags.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
import "#veryfront/schemas/_test-setup.ts";
import { assertEquals, assertRejects } from "#veryfront/testing/assert.ts";
import { describe, it } from "#veryfront/testing/bdd.ts";
import { join } from "#veryfront/compat/path/index.ts";
import { withCwd } from "#veryfront/testing/cwd.ts";
import { parseCliArgs } from "#cli/shared/args";
import { assertEmbeddedPresetFlags, handleBuildCommand } from "./handler.ts";

async function exists(path: string): Promise<boolean> {
try {
await Deno.stat(path);
return true;
} catch (error) {
if (error instanceof Deno.errors.NotFound) return false;
throw error;
}
}

async function makeProject(prefix: string): Promise<string> {
const projectDir = await Deno.makeTempDir({ prefix });
await Deno.mkdir(join(projectDir, "app"), { recursive: true });
await Deno.writeTextFile(join(projectDir, "app/page.mdx"), "# Home\n");
return projectDir;
}

describe("commands/build/handler embedded preset flags", () => {
it("does not write a bundle for a --dry-run embedded build", async () => {
const projectDir = await makeProject("vf-embedded-dry-run-");
try {
await withCwd(projectDir, async () => {
await assertRejects(
() =>
handleBuildCommand(
parseCliArgs(["build", "--preset", "embedded", "--dry-run"]),
),
Error,
"--dry-run",
);
});

assertEquals(
await exists(join(projectDir, "dist/embedded/manifest.json")),
false,
"a dry run must not write dist/embedded/manifest.json",
);
} finally {
await Deno.remove(projectDir, { recursive: true });
}
});

it("writes an embedded bundle to build.outDir from veryfront.config.js", async () => {
const projectDir = await makeProject("vf-embedded-outdir-");
try {
await Deno.writeTextFile(
join(projectDir, "veryfront.config.js"),
'export default { build: { outDir: "custom-out" } };\n',
);

await withCwd(projectDir, async () => {
await handleBuildCommand(parseCliArgs(["build", "--preset", "embedded"]));
});

assertEquals(
await exists(join(projectDir, "custom-out/embedded/manifest.json")),
true,
"build.outDir must decide where the embedded preset writes",
);
assertEquals(
await exists(join(projectDir, "dist")),
false,
"the embedded preset must not fall back to dist/ when build.outDir is set",
);
} finally {
await Deno.remove(projectDir, { recursive: true });
}
});

describe("flag validation", () => {
const rejected: Array<{ argv: string[]; flag: string }> = [
{ argv: ["build", "--preset", "embedded", "--dry-run"], flag: "--dry-run" },
{ argv: ["build", "--preset", "embedded", "--split"], flag: "--split" },
{ argv: ["build", "--preset", "embedded", "--no-split"], flag: "--no-split" },
{ argv: ["build", "--preset", "embedded", "--compress"], flag: "--compress" },
{ argv: ["build", "--preset", "embedded", "--no-compress"], flag: "--no-compress" },
{ argv: ["build", "--preset", "embedded", "--prefetch"], flag: "--prefetch" },
{ argv: ["build", "--preset", "embedded", "--prefetch=false"], flag: "--prefetch" },
{ argv: ["build", "--preset", "embedded", "--ssg"], flag: "--ssg" },
{ argv: ["build", "--preset", "embedded", "--no-ssg"], flag: "--no-ssg" },
{ argv: ["build", "--preset", "embedded", "--include", "/docs"], flag: "--include" },
{ argv: ["build", "--preset", "embedded", "--exclude", "/api"], flag: "--exclude" },
];

for (const { argv, flag } of rejected) {
it(`rejects ${argv.slice(3).join(" ")} and names ${flag}`, () => {
const args = parseCliArgs(argv);
let thrown: unknown;
try {
assertEmbeddedPresetFlags(args, "embedded");
} catch (error) {
thrown = error;
}

assertEquals(thrown instanceof Error, true, `${flag} must be rejected`);
const message = (thrown as Error).message;
assertEquals(
message.startsWith("Invalid "),
true,
`usage errors must start with "Invalid " so the router exits 2: ${message}`,
);
assertEquals(
message.includes(flag),
true,
`the error must name ${flag}: ${message}`,
);
});
}

const accepted: string[][] = [
["build", "--preset", "embedded"],
["build", "--preset", "embedded", "-o", "out"],
["build", "--preset", "embedded", "--output", "out"],
["build", "--preset", "embedded", "--json"],
["build", "--preset", "embedded", "--verbose"],
["build", "--preset", "embedded", "--quiet"],
];

for (const argv of accepted) {
it(`accepts ${argv.slice(1).join(" ")}`, () => {
assertEmbeddedPresetFlags(parseCliArgs(argv), "embedded");
});
}

it("leaves the default preset alone", () => {
assertEmbeddedPresetFlags(
parseCliArgs(["build", "--dry-run", "--no-split", "--include", "/docs"]),
undefined,
);
});

it("names every rejected flag the user typed", () => {
let thrown: unknown;
try {
assertEmbeddedPresetFlags(
parseCliArgs(["build", "--preset", "embedded", "--dry-run", "--no-ssg"]),
"embedded",
);
} catch (error) {
thrown = error;
}
const message = (thrown as Error).message;
assertEquals(message.includes("--dry-run"), true, message);
assertEquals(message.includes("--no-ssg"), true, message);
});
});
});
87 changes: 83 additions & 4 deletions cli/commands/build/handler.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import { defineSchema, lazySchema } from "veryfront/schemas";
import type { InferSchema } from "veryfront/extensions/schema";
import { dim } from "#cli/ui";
import { join } from "veryfront/platform/path";
import { cliLogger, isVerbose, logSuccess } from "#cli/utils";
import { cwd } from "veryfront/platform";
import { cwd, runtime } from "veryfront/platform";
import { CommonArgs, createArgParser, parseArgsOrThrow } from "#cli/shared/args";
import { ensureCliBundlerContracts } from "#cli/shared/default-contracts";
import { showHeader } from "#cli/utils";
Expand Down Expand Up @@ -55,12 +54,76 @@ export const parseBuildArgs = createArgParser(BuildArgsSchema, {
dryRun: CommonArgs.dryRun,
}, { rejectUnknown: true });

/**
* Flags the embedded preset cannot honour, in the spelling the user types.
*
* The embedded preset emits one esbuild bundle: there is nothing to split,
* no compression pass, no prefetch manifest and no prerender step, so
* `--split`, `--compress`, `--prefetch`, `--ssg`, `--include` and `--exclude`
* describe stages it does not have. `--dry-run` is different in kind — the
* preset simply never implemented it, and a flag whose whole contract is
* "changes nothing" writing to disk is the worst outcome of the three.
*
* Rejecting is the honest answer for all of them. Accepting a flag and
* dropping it, which is what this path used to do, tells the user the build
* ran the way they asked when it did not.
*/
const UNSUPPORTED_EMBEDDED_FLAGS = [
"dry-run",
"split",
"no-split",
"compress",
"no-compress",
"prefetch",
"ssg",
"no-ssg",
"include",
"exclude",
] as const;

/**
* Refuse an embedded build that was given a flag the preset cannot honour.
*
* Reads `__explicit` from the *raw* args rather than the parsed options on
* purpose. The schema defaults `split`, `compress` and `prefetch` to `true`
* and `dryRun`, `noSplit`, `noCompress` and `noSsg` to `false`, so the parsed
* object cannot tell a typed flag from a default — keying off it would reject
* every embedded build, including a bare one.
*
* The message starts with `Invalid ` so the router maps it to exit code 2 as a
* usage error, matching `parseArgsOrThrow`.
*
* @param args raw parsed argv, carrying `__explicit`
* @param preset the lowercased `--preset` value, if any
* @internal
*/
export function assertEmbeddedPresetFlags(
args: ParsedArgs,
preset: string | undefined,
): void {
if (preset !== "embedded") return;

const explicit = args.__explicit ?? {};
const unsupported = UNSUPPORTED_EMBEDDED_FLAGS
.filter((flag) => explicit[flag] === true)
.map((flag) => `--${flag}`);
if (unsupported.length === 0) return;

throw new Error(
`Invalid build arguments: the embedded preset does not support ${unsupported.join(", ")}`,
);
}

export async function handleBuildCommand(args: ParsedArgs): Promise<void> {
showHeader();
const opts = parseArgsOrThrow(parseBuildArgs, "build", args);
const preset = opts.preset?.toLowerCase();
// Before any bundler setup, so a rejected build touches nothing and returns
// immediately.
assertEmbeddedPresetFlags(args, preset);

await ensureCliBundlerContracts();
const projectDir = cwd();
const preset = opts.preset?.toLowerCase();

if (preset === "embedded") {
await ensureBuiltinContentProcessor();
Expand All @@ -87,8 +150,23 @@ export async function handleBuildCommand(args: ParsedArgs): Promise<void> {

async function handleEmbeddedBuild(projectDir: string, outputDir?: string): Promise<void> {
const { buildEmbeddedPreset } = await import("veryfront/build");
const { getConfig } = await import("veryfront/config");
const { resolveBuildOutputDir } = await import("./command.ts");

const finalOutput = outputDir ?? join(projectDir, "dist");
// The config was never loaded on this path, so `build.outDir` was ignored
// and the preset always wrote `dist`. Resolving through the same helper the
// default path uses also brings its guard against an output directory that
// contains the project.
const adapter = await runtime.get();
const config = await getConfig(projectDir, adapter);
// `clearsOutputDir: false` because `buildEmbeddedPreset` only mkdir's and
// writes into the target; unlike the production build it never removes it.
// Without this, `--preset embedded -o .` — a plausible call for a preset
// whose whole purpose is embedding into a host project — hard-fails on a
// deletion hazard that does not exist on this path.
const finalOutput = resolveBuildOutputDir(projectDir, outputDir, config, {
clearsOutputDir: false,
});

cliLogger.info("Building embedded preset...");
if (isVerbose()) {
Expand All @@ -100,6 +178,7 @@ async function handleEmbeddedBuild(projectDir: string, outputDir?: string): Prom
projectDir,
outDir: finalOutput,
runtime: "deno",
config,
});

logSuccess("Built embedded preset");
Expand Down