diff --git a/packages/cli/src/commands/add.test.ts b/packages/cli/src/commands/add.test.ts index 2f3d7ce5e1..a4f35c44b6 100644 --- a/packages/cli/src/commands/add.test.ts +++ b/packages/cli/src/commands/add.test.ts @@ -156,6 +156,8 @@ const ITEM_BY_NAME: Record = { "my-example": EXAMPLE_ITEM, }; +const DEP_BLOCK_HTML = `
`; + function mockFetch(): void { vi.stubGlobal( "fetch", @@ -171,6 +173,9 @@ function mockFetch(): void { } // File fetch — match `///` and serve synthetic content. const f = /\/(examples|blocks|components)\/([^/]+)\/(.+)$/.exec(url); + if (f?.[3] === "dep-block.html") { + return new Response(DEP_BLOCK_HTML, { status: 200 }); + } if (f) { return new Response(`/* ${f[3]} */\n`, { status: 200 }); } @@ -357,6 +362,20 @@ describe("runAdd (integration, mocked registry)", () => { } }); + it("refuses a wrong-typed --vars before writing any item in the plan", async () => { + const dir = tmp(); + try { + writeRegistryConfig(dir); + await expect( + runAdd({ name: "dep-block", projectDir: dir, skipClipboard: true, vars: '{"maths":1}' }), + ).rejects.toThrow(/maths: expected boolean, got number/); + expect(existsSync(join(dir, "compositions/components/base-component"))).toBe(false); + expect(existsSync(join(dir, "hyperframes.lock.json"))).toBe(false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it("installs transitive registryDependencies before the requested item", async () => { const dir = tmp(); try { diff --git a/packages/cli/src/commands/add.ts b/packages/cli/src/commands/add.ts index c37ea85152..6ba47c5ce9 100644 --- a/packages/cli/src/commands/add.ts +++ b/packages/cli/src/commands/add.ts @@ -15,8 +15,15 @@ import { existsSync } from "node:fs"; import { resolve, relative } from "node:path"; import { ITEM_TYPE_DIRS, type RegistryItem } from "@hyperframes/core"; import { c } from "../ui/colors.js"; -import { DEFAULT_REGISTRY_URL, installItem, resolveItemsByTag } from "../registry/index.js"; +import { + DEFAULT_REGISTRY_URL, + prepareItem, + publishItem, + resolveItemsByTag, + type PreparedItem, +} from "../registry/index.js"; import { resolveItemWithDependencies } from "../registry/resolver.js"; +import { InvalidVariableValuesError } from "../registry/variableDefaults.js"; import { gateRegistryItemsCompatibility, RegistryCompatibilityError, @@ -194,8 +201,8 @@ function assertCompatibleOrThrow(items: RegistryItem[], cliVersion?: string): st } // Install a topologically-ordered plan (dependencies first, requested item -// last). The installer validates every target before any write; a failure on -// any item surfaces as an install-failed AddError. Returns all written paths. +// last). Every item is fetched and checked before any is written, so a refusal +// or failed fetch anywhere in the plan writes nothing. Returns all written paths. async function installAll( installPlan: RegistryItem[], destDir: string, @@ -208,35 +215,39 @@ async function installAll( preserved: string[]; variablesApplied: string[]; variablesUnknown: string[]; - variablesInvalid: { id: string; reason: string }[]; }> { const written: string[] = []; const preserved: string[] = []; let variablesApplied: string[] = []; let variablesUnknown: string[] = []; - let variablesInvalid: { id: string; reason: string }[] = []; try { + const prepared: PreparedItem[] = []; for (const planItem of installPlan) { - const result = await installItem(planItem, { - destDir, - baseUrl, - force, - // Only the item the user named. A dependency dragged in behind it never - // declared these variables and must not be rewritten by them. - variableValues: planItem.name === requestedName ? variableValues : null, - }); + prepared.push( + await prepareItem(planItem, { + destDir, + baseUrl, + force, + // Only the item the user named. A dependency dragged in behind it never + // declared these variables and must not be rewritten by them. + variableValues: planItem.name === requestedName ? variableValues : null, + }), + ); + } + for (const [i, planItem] of installPlan.entries()) { + const result = publishItem(prepared[i]!); written.push(...result.written); preserved.push(...result.preserved); if (planItem.name === requestedName) { variablesApplied = result.variablesApplied; variablesUnknown = result.variablesUnknown; - variablesInvalid = result.variablesInvalid; } } } catch (err) { + if (err instanceof InvalidVariableValuesError) throw new AddError(err.message, "invalid-vars"); throw new AddError(describeInstallFailure(err, baseUrl), "install-failed"); } - return { written, preserved, variablesApplied, variablesUnknown, variablesInvalid }; + return { written, preserved, variablesApplied, variablesUnknown }; } /** @@ -318,15 +329,14 @@ export async function runAdd(opts: RunAddArgs): Promise { // 5. Install — dependencies first, requested item last. const variableValues = parseVariableValues(opts.vars); - const { written, preserved, variablesApplied, variablesUnknown, variablesInvalid } = - await installAll( - installPlan, - projectDir, - config.registry, - opts.force ?? false, - item.name, - variableValues, - ); + const { written, preserved, variablesApplied, variablesUnknown } = await installAll( + installPlan, + projectDir, + config.registry, + opts.force ?? false, + item.name, + variableValues, + ); // Report what landed, not what was asked for: a failed install throws above, // and the bulk `add ` path re-enters here per item, so this one place @@ -358,9 +368,6 @@ export async function runAdd(opts: RunAddArgs): Promise { const snippet = buildSnippet(item, snippetTargetRel, variableValues); const clipboardCopied = !opts.skipClipboard && snippet ? copyToClipboard(snippet) : false; - for (const { id, reason } of variablesInvalid) { - warnings.push(`--vars ${id} ignored: ${reason}`); - } if (variablesUnknown.length > 0) { warnings.push(`--vars ignored (not declared by ${item.name}): ${variablesUnknown.join(", ")}`); } @@ -539,8 +546,9 @@ export default defineCommand({ if (!json) console.log(` ${c.warn("Warning:")} ${warning}`); } if (!json) console.log(` ${c.success("✓")} ${result.name}`); - } catch { - if (!json) console.log(` ${c.error("✗")} ${item.name} (skipped)`); + } catch (err) { + const reason = err instanceof Error ? err.message : String(err); + if (!json) console.log(` ${c.error("✗")} ${item.name} (skipped: ${reason})`); } } diff --git a/packages/cli/src/registry/index.ts b/packages/cli/src/registry/index.ts index b81f5f12a5..7e4ab696c4 100644 --- a/packages/cli/src/registry/index.ts +++ b/packages/cli/src/registry/index.ts @@ -5,4 +5,4 @@ export { DEFAULT_REGISTRY_URL } from "./remote.js"; export { listRegistryItems, loadAllItems, resolveItemsByTag } from "./resolver.js"; -export { installItem } from "./installer.js"; +export { installItem, prepareItem, publishItem, type PreparedItem } from "./installer.js"; diff --git a/packages/cli/src/registry/installer.test.ts b/packages/cli/src/registry/installer.test.ts index 3b380985ae..984170068f 100644 --- a/packages/cli/src/registry/installer.test.ts +++ b/packages/cli/src/registry/installer.test.ts @@ -21,7 +21,7 @@ vi.mock("./remote.js", () => ({ fetchItemFile: vi.fn(async () => Buffer.from(remote.contents)), })); -const { hasLocalEdits, installItem } = await import("./installer.js"); +const { hasLocalEdits, installItem, prepareItem, publishItem } = await import("./installer.js"); function project(): string { return mkdtempSync(join(tmpdir(), "hf-installer-")); @@ -148,6 +148,21 @@ describe("installItem", () => { expect(readFileSync(join(dir, target), "utf-8")).toBe("MY OWN COLOURS\n"); }); + it("keeps an edit saved while the rest of the plan was still downloading", async () => { + const dir = project(); + await installItem(item, { destDir: dir }); + remote.contents = "REGISTRY VERSION 2\n"; + const prepared = await prepareItem(item, { destDir: dir }); + remote.contents = "REGISTRY VERSION\n"; + writeFileSync(join(dir, target), "MY OWN COLOURS\n", "utf-8"); + + const result = publishItem(prepared); + + expect(result.written).toEqual([]); + expect(result.preserved).toHaveLength(1); + expect(readFileSync(join(dir, target), "utf-8")).toBe("MY OWN COLOURS\n"); + }); + it("keeps a file that was there before any install", async () => { const dir = project(); mkdirSync(join(dir, "components"), { recursive: true }); @@ -236,3 +251,42 @@ describe("installing several items, as a dependency plan does", () => { expect(readFileSync(join(dir, target), "utf-8")).toBe("EDITED DEPENDENCY\n"); }); }); + +describe("installing with --vars the item cannot take", () => { + const declaration = `
`; + const block = { + name: "demo-block", + title: "Demo block", + description: "Block", + dimensions: { width: 100, height: 100 }, + duration: 1, + type: "hyperframes:block", + files: [ + { + path: "demo-block.html", + target: "compositions/demo-block.html", + type: "hyperframes:composition", + }, + ], + } as unknown as RegistryItem; + + it.each([ + ["block", block, "compositions/demo-block.html"], + ["component", item, target], + ])( + "refuses a wrong-typed value for a %s and writes nothing", + async (_kind, installable, file) => { + const dir = project(); + remote.contents = declaration; + try { + await expect( + installItem(installable, { destDir: dir, variableValues: { maths: 1 } }), + ).rejects.toThrow(/maths: expected boolean, got number/); + expect(existsSync(join(dir, file))).toBe(false); + } finally { + remote.contents = "REGISTRY VERSION\n"; + rmSync(dir, { recursive: true, force: true }); + } + }, + ); +}); diff --git a/packages/cli/src/registry/installer.ts b/packages/cli/src/registry/installer.ts index 3a58aee8bd..b1f5304d90 100644 --- a/packages/cli/src/registry/installer.ts +++ b/packages/cli/src/registry/installer.ts @@ -14,7 +14,11 @@ import { existsSync, readFileSync } from "node:fs"; import { resolve, relative, isAbsolute } from "node:path"; import type { FileTarget, RegistryItem } from "@hyperframes/core"; import { fetchItemFile, DEFAULT_REGISTRY_URL } from "./remote.js"; -import { applyVariableDefaults, type ApplyResult } from "./variableDefaults.js"; +import { + applyVariableDefaults, + InvalidVariableValuesError, + type ApplyResult, +} from "./variableDefaults.js"; export interface InstallOptions { /** Project root where files land. Every target resolves relative to this. */ @@ -40,7 +44,6 @@ export interface InstallResult { variablesApplied: string[]; /** Ids the item does not declare, and ids it declares but cannot accept. */ variablesUnknown: string[]; - variablesInvalid: { id: string; reason: string }[]; } /** @@ -100,6 +103,16 @@ export function hasLocalEdits( return installed !== digest(onDisk); } +/** An install must leave this file alone: the project changed it, and `--force` was not given. */ +function keptByProject( + record: InstallRecord, + destPath: string, + target: string, + force: boolean | undefined, +): boolean { + return !force && existsSync(destPath) && hasLocalEdits(record, target, readFileSync(destPath)); +} + /** * Reject target paths that would escape `destDir`. Mirrors the pattern check * in `packages/core/schemas/registry-item.json#files.items.target`, but runs at @@ -144,16 +157,37 @@ function addRegistryItemMarker(source: string, item: RegistryItem): string { return `\n${source}`; } -interface FileOutcome { +export interface FileOutcome { destPath: string; target: string; preserved: boolean; hash: string | null; vars: ApplyResult | null; + /** What to write; null for a preserved file. */ + bytes: Buffer | null; } -/** Fetch, write and post-process one file. Extracted so installItem stays readable. */ -async function installOneFile( +/** + * Check `--vars` against the file that declares them. A block's values ride on + * its mount, so only a component's own declaration is rewritten. + */ +function bakeVariables( + item: RegistryItem, + file: FileTarget, + bytes: Buffer, + values: Record | null | undefined, +): { bytes: Buffer; vars: ApplyResult | null } { + const component = isInstalledComponentSnippet(item, file); + if (!values || !(component || isInstalledRegistryBlockComposition(item, file))) { + return { bytes, vars: null }; + } + const vars = applyVariableDefaults(bytes.toString("utf8"), values); + const rewrite = component && vars.applied.length > 0; + return { bytes: rewrite ? Buffer.from(vars.html) : bytes, vars }; +} + +/** Fetch and post-process one file; publishItem writes it once the whole plan is ready. */ +async function prepareOneFile( item: RegistryItem, file: FileTarget, destDir: string, @@ -167,32 +201,26 @@ async function installOneFile( // Decided before fetching rather than after: a file we are going to keep // should never be overwritten and then put back, because a crash in // between would lose it for real. - if ( - !options.force && - existsSync(destPath) && - hasLocalEdits(record, file.target, readFileSync(destPath)) - ) { - return { destPath, target: file.target, preserved: true, hash: null, vars: null }; + if (keptByProject(record, destPath, file.target, options.force)) { + return { destPath, target: file.target, preserved: true, hash: null, vars: null, bytes: null }; } let bytes = await fetchItemFile(item, file, baseUrl, budget); if (isInstalledRegistryBlockComposition(item, file)) { bytes = Buffer.from(addRegistryItemMarker(bytes.toString("utf8"), item)); } - let vars: ApplyResult | null = null; - if (options.variableValues && isInstalledComponentSnippet(item, file)) { - vars = applyVariableDefaults(bytes.toString("utf8"), options.variableValues); - if (vars.applied.length > 0) bytes = Buffer.from(vars.html); - } - publishRegistryFile(destDir, file.target, bytes); - // Hash what actually landed, marker and baked defaults included, or the - // next install reads its own output as the project's edit. + const baked = bakeVariables(item, file, bytes, options.variableValues); + bytes = baked.bytes; + const vars = baked.vars; + // Hash what will land, marker and baked defaults included, or the next + // install reads its own output as the project's edit. return { destPath, target: file.target, preserved: false, hash: digest(bytes), vars, + bytes, }; } @@ -204,6 +232,21 @@ export async function installItem( item: RegistryItem, options: InstallOptions, ): Promise { + return publishItem(await prepareItem(item, options)); +} + +/** An item fetched and checked, with nothing written yet. */ +export interface PreparedItem { + root: string; + outcomes: FileOutcome[]; + force: boolean; +} + +/** Fetch and check every file of an item without writing, so a caller can refuse a whole plan. */ +export async function prepareItem( + item: RegistryItem, + options: InstallOptions, +): Promise { if (!validRegistryItem(item, item.name, item.type)) throw new Error("Invalid registry item"); const baseUrl = options.baseUrl ?? DEFAULT_REGISTRY_URL; const destDir = resolve(options.destDir); @@ -219,8 +262,26 @@ export async function installItem( const budget = { remainingBytes: 512 * 1024 * 1024 }; const outcomes = await installFileBatches(item.files, (file) => - installOneFile(item, file, root, baseUrl, record, options, budget), + prepareOneFile(item, file, root, baseUrl, record, options, budget), ); + const invalid = outcomes.flatMap((o) => o.vars?.invalid ?? []); + if (invalid.length > 0) throw new InvalidVariableValuesError(invalid); + return { root, outcomes, force: options.force ?? false }; +} + +/** Write a prepared item and record what landed. */ +export function publishItem({ root, outcomes: prepared, force }: PreparedItem): InstallResult { + // Read now, not at prepare time: another item published in between has recorded its own files. + const record = readInstallRecord(root); + // Asked again at write time: a save made while the rest of the plan downloaded is still an edit. + const outcomes = prepared.map((o) => + o.bytes && keptByProject(record, o.destPath, o.target, force) + ? { ...o, preserved: true, hash: null, vars: null, bytes: null } + : o, + ); + for (const outcome of outcomes) { + if (outcome.bytes) publishRegistryFile(root, outcome.target, outcome.bytes); + } const written = outcomes.filter((o) => !o.preserved).map((o) => o.destPath); const preserved = outcomes.filter((o) => o.preserved).map((o) => o.destPath); @@ -245,7 +306,6 @@ export async function installItem( vars[0]!.unknown, ) : [], - variablesInvalid: vars.flatMap((v) => v.invalid), }; } @@ -267,11 +327,11 @@ function validatePhysicalTargets(root: string, files: FileTarget[]): void { async function installFileBatches( files: FileTarget[], - install: (file: FileTarget) => Promise, + prepare: (file: FileTarget) => Promise, ): Promise { const outcomes: FileOutcome[] = []; for (let at = 0; at < files.length; at += 4) { - const batch = await Promise.allSettled(files.slice(at, at + 4).map(install)); + const batch = await Promise.allSettled(files.slice(at, at + 4).map(prepare)); for (const result of batch) { if (result.status === "rejected") throw result.reason; outcomes.push(result.value); diff --git a/packages/cli/src/registry/variableDefaults.test.ts b/packages/cli/src/registry/variableDefaults.test.ts index 04b07b1553..9307cfd4cc 100644 --- a/packages/cli/src/registry/variableDefaults.test.ts +++ b/packages/cli/src/registry/variableDefaults.test.ts @@ -106,3 +106,30 @@ describe("applyVariableDefaults", () => { expect(applyVariableDefaults(COMPONENT, {}).html).toBe(COMPONENT); }); }); + +/** One variable of each type the CLI previously wrote without checking. */ +const TYPED = `
`; + +describe("applyVariableDefaults type checks", () => { + it("refuses a value whose type does not match the declaration", () => { + expect(applyVariableDefaults(TYPED, { maths: 1, accent: 5, title: {} }).invalid).toEqual([ + { id: "maths", reason: "expected boolean, got number" }, + { id: "accent", reason: "expected color, got number" }, + { id: "title", reason: "expected string, got object" }, + ]); + }); + + it("keeps a boolean a boolean, so the composition's own check still sees true", () => { + const r = applyVariableDefaults(TYPED, { maths: true, accent: "#ffffff" }); + expect(r.invalid).toEqual([]); + expect(defaultOf(r.html, "maths")).toBe(true); + expect(defaultOf(r.html, "accent")).toBe("#ffffff"); + }); +}); diff --git a/packages/cli/src/registry/variableDefaults.ts b/packages/cli/src/registry/variableDefaults.ts index 6b1b631afd..fd12b508a3 100644 --- a/packages/cli/src/registry/variableDefaults.ts +++ b/packages/cli/src/registry/variableDefaults.ts @@ -16,7 +16,11 @@ * silently discarded for every component in the catalog. */ -import { isCompositionVariable, type CompositionVariable } from "@hyperframes/core/variables"; +import { + isCompositionVariable, + validateVariables, + type CompositionVariable, +} from "@hyperframes/core/variables"; export interface ApplyResult { /** The source with defaults rewritten. Unchanged when nothing applied. */ @@ -29,6 +33,14 @@ export interface ApplyResult { invalid: { id: string; reason: string }[]; } +/** `--vars` values the item's declaration cannot take; the install writes nothing. */ +export class InvalidVariableValuesError extends Error { + constructor(readonly invalid: { id: string; reason: string }[]) { + super(`--vars refused: ${invalid.map(({ id, reason }) => `${id}: ${reason}`).join("; ")}`); + this.name = "InvalidVariableValuesError"; + } +} + const ATTR = "data-composition-variables"; /** Locate the attribute's quoted value, tolerating either delimiter. */ @@ -52,23 +64,6 @@ function encode(json: string, quote: string): string { return quote === "'" ? json.replace(/'/g, "'") : json.replace(/"/g, """); } -function optionValues(decl: CompositionVariable): string[] | null { - return decl.type === "enum" ? decl.options.map((option) => option.value) : null; -} - -/** - * Reject a value the declaration cannot represent, rather than writing it. - * - * A bad enum falls back to the default at runtime and warns, so writing one - * here would produce a file that renders as if the value had been ignored -- - * which is the exact failure this function exists to remove. - */ -function rejectEnum(decl: CompositionVariable, value: unknown): string | null { - const options = optionValues(decl); - if (!options) return null; - return options.includes(String(value)) ? null : `not one of ${options.join(", ")}`; -} - function rejectNumber(decl: CompositionVariable, value: unknown): string | null { if (decl.type !== "number") return null; const n = typeof value === "number" ? value : Number(value); @@ -78,8 +73,15 @@ function rejectNumber(decl: CompositionVariable, value: unknown): string | null return null; } +// Refuse what the runtime would ignore. Numbers keep their own check (string input, min/max); +// every other type goes through the runtime's validator. function reject(decl: CompositionVariable, value: unknown): string | null { - return rejectEnum(decl, value) ?? rejectNumber(decl, value); + if (decl.type === "number") return rejectNumber(decl, value); + const [issue] = validateVariables({ [decl.id]: value }, [decl]); + if (!issue) return null; + if (issue.kind === "enum-out-of-range") return `not one of ${issue.allowed.join(", ")}`; + if (issue.kind === "type-mismatch") return `expected ${issue.expected}, got ${issue.actual}`; + return null; } export function applyVariableDefaults( @@ -116,7 +118,7 @@ export function applyVariableDefaults( const applied: string[] = []; const invalid: { id: string; reason: string }[] = []; const byId = new Map(declared.map((decl) => [decl.id, decl])); - const updated = new Map(); + const updated = new Map(); for (const [id, value] of Object.entries(values)) { const decl = byId.get(id); @@ -129,7 +131,7 @@ export function applyVariableDefaults( // The declaration's own type decides how the value is stored. A number // written as the string "76" would trip the composition's guard and fall // back, which looks exactly like the value being ignored. - updated.set(id, decl.type === "number" ? Number(value) : String(value)); + updated.set(id, decl.type === "number" ? Number(value) : value); applied.push(id); }