mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-03 04:38:33 +00:00
fix(cli): default render fps to the composition's data-fps
* fix(cli): default render fps to the composition's data-fps
hyperframes render hard-coded fps to 30 when --fps was omitted, ignoring a
data-fps declared on the composition root — so a composition authored at
data-fps="24" silently rendered at 30fps unless the user knew to pass --fps 24.
The runtime already honors data-fps; the CLI now matches it.
Precedence: explicit --fps > composition root data-fps > 30. New pure
readCompositionFps() extracts the root data-fps via linkedom (mirrors the
runtime's root resolution: [data-composition-id][data-root=true], else the
outermost [data-composition-id]); render validates it through parseFps and
falls back to 30 on an absent/invalid value. Unit-tested.
* fix(cli): honor composition data-fps on cloud renders and --composition targets
The local render command read data-fps from project.dir/index.html even when
--composition rendered a different file, and the lambda/cloudrun render paths
ignored data-fps entirely (hardcoded ?? 30). Both are the same silently-wrong-
fps bug on other render entry points:
- render.ts resolves the entry file first, then reads data-fps from the file
actually being rendered (falling back to index.html).
- lambda render/render-batch and cloudrun render/render-batch default fps from
the composition's data-fps, accepted only when it is one of the cloud-allowed
values {24,30,60}, else the existing 30 default. Explicit --fps still wins.
* fix(cli): drop citty fps default so data-fps resolution actually runs
The fps arg had default: "30", so citty set args.fps="30" on omission and
resolveDefaultFpsArg short-circuited (explicitFps never null) — reverting the
command to always-30 and making the whole data-fps feature a no-op (caught in
review). Remove the arg default; the "30" fallback already lives at
parseFps(fpsArg ?? "30"). Adds a regression guard asserting the arg has no
default.
* test(cli): read citty args through a plain record in the fps-default guard
The regression guard accessed cmd.args.fps directly, but citty types args as
Resolvable<ArgsDef> so .fps failed typecheck in CI. Read it through a plain
record cast.
This commit is contained in:
@@ -31,6 +31,7 @@ import {
|
||||
validateVariablesAgainstProject,
|
||||
} from "../utils/variables.js";
|
||||
import { normalizeErrorMessage } from "../utils/errorMessage.js";
|
||||
import { readAllowedCompositionFpsFromDir } from "../utils/compositionFps.js";
|
||||
|
||||
export const examples: Example[] = [
|
||||
["Deploy the Cloud Run render stack", "hyperframes cloudrun deploy --project my-gcp-project"],
|
||||
@@ -499,7 +500,8 @@ async function runRender(args: Record<string, unknown>): Promise<void> {
|
||||
console.error("[cloudrun render] --width and --height are required.");
|
||||
process.exit(1);
|
||||
}
|
||||
const fps = parseIntFlag(args.fps) ?? 30;
|
||||
const fps =
|
||||
parseIntFlag(args.fps) ?? readAllowedCompositionFpsFromDir(projectDir, [24, 30, 60]) ?? 30;
|
||||
if (fps !== 24 && fps !== 30 && fps !== 60) {
|
||||
console.error(`[cloudrun render] --fps must be 24, 30, or 60; got ${fps}.`);
|
||||
process.exit(1);
|
||||
@@ -609,7 +611,8 @@ async function runRenderBatch(args: Record<string, unknown>): Promise<void> {
|
||||
console.error("[cloudrun render-batch] --width and --height are required.");
|
||||
process.exit(1);
|
||||
}
|
||||
const fps = parseIntFlag(args.fps) ?? 30;
|
||||
const fps =
|
||||
parseIntFlag(args.fps) ?? readAllowedCompositionFpsFromDir(projectDir, [24, 30, 60]) ?? 30;
|
||||
if (fps !== 24 && fps !== 30 && fps !== 60) {
|
||||
console.error(`[cloudrun render-batch] --fps must be 24, 30, or 60; got ${fps}.`);
|
||||
process.exit(1);
|
||||
|
||||
@@ -17,6 +17,7 @@ import {
|
||||
} from "@hyperframes/core";
|
||||
import type { Example } from "./_examples.js";
|
||||
import { c } from "../ui/colors.js";
|
||||
import { readAllowedCompositionFpsFromDir } from "../utils/compositionFps.js";
|
||||
|
||||
export const examples: Example[] = [
|
||||
["Deploy the Lambda render stack to AWS", "hyperframes lambda deploy"],
|
||||
@@ -297,7 +298,10 @@ export default defineCommand({
|
||||
console.error("[lambda render] --width and --height are required.");
|
||||
process.exit(1);
|
||||
}
|
||||
const fpsRaw = parseIntFlag(args.fps) ?? 30;
|
||||
const fpsRaw =
|
||||
parseIntFlag(args.fps) ??
|
||||
readAllowedCompositionFpsFromDir(projectDir, [24, 30, 60]) ??
|
||||
30;
|
||||
if (fpsRaw !== 24 && fpsRaw !== 30 && fpsRaw !== 60) {
|
||||
console.error(`[lambda render] --fps must be 24, 30, or 60; got ${fpsRaw}.`);
|
||||
process.exit(1);
|
||||
@@ -349,7 +353,10 @@ export default defineCommand({
|
||||
console.error("[lambda render-batch] --width and --height are required.");
|
||||
process.exit(1);
|
||||
}
|
||||
const fpsRaw = parseIntFlag(args.fps) ?? 30;
|
||||
const fpsRaw =
|
||||
parseIntFlag(args.fps) ??
|
||||
readAllowedCompositionFpsFromDir(projectDir, [24, 30, 60]) ??
|
||||
30;
|
||||
if (fpsRaw !== 24 && fpsRaw !== 30 && fpsRaw !== 60) {
|
||||
console.error(`[lambda render-batch] --fps must be 24, 30, or 60; got ${fpsRaw}.`);
|
||||
process.exit(1);
|
||||
|
||||
@@ -506,4 +506,21 @@ describe("checkRenderResolutionPreflight", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("render fps arg definition", () => {
|
||||
it("declares no citty default for --fps (so data-fps resolution can run)", async () => {
|
||||
// Regression guard: a `default: "30"` here makes citty set args.fps="30"
|
||||
// on omission, which short-circuits resolveDefaultFpsArg (explicitFps is
|
||||
// never null) and silently reverts the command to always-30 — the exact
|
||||
// no-op caught in review. The "30" fallback must live at the
|
||||
// parseFps(fpsArg ?? "30") call, not on the arg.
|
||||
const cmd = (await import("./render.js")).default;
|
||||
// citty types `args` as Resolvable (it could be a promise/factory); in
|
||||
// practice it's the literal object, so read it through a plain record.
|
||||
const args = cmd.args as unknown as Record<string, { default?: unknown } | undefined>;
|
||||
const fpsArg = args.fps;
|
||||
expect(fpsArg).toBeDefined();
|
||||
expect(fpsArg?.default).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
// Variables-helper tests live in `../utils/variables.test.ts`.
|
||||
|
||||
@@ -10,6 +10,7 @@ import {
|
||||
parseGifLoopArg,
|
||||
resolveBrowserTimeoutMsArg,
|
||||
resolveCompositionEntryArg,
|
||||
resolveDefaultFpsArg,
|
||||
} from "../utils/renderArgs.js";
|
||||
|
||||
export const examples: Example[] = [
|
||||
@@ -171,8 +172,12 @@ export default defineCommand({
|
||||
description:
|
||||
"Frame rate. Accepts integer (24, 25, 30, 50, 60, 120, 240) or " +
|
||||
"ffmpeg-style rational (30000/1001 for NTSC 29.97, 24000/1001 for " +
|
||||
"23.976, 60000/1001 for 59.94). Range 1-240.",
|
||||
default: "30",
|
||||
"23.976, 60000/1001 for 59.94). Range 1-240. " +
|
||||
"Defaults to the composition's root data-fps, else 30.",
|
||||
// No `default` here on purpose: citty would set args.fps="30" on
|
||||
// omission, which would make explicitFps always non-null and short-
|
||||
// circuit the data-fps resolution below (resolveDefaultFpsArg). The
|
||||
// "30" fallback lives at the parseFps(fpsArg ?? "30") call instead.
|
||||
},
|
||||
quality: {
|
||||
type: "string",
|
||||
@@ -382,15 +387,24 @@ export default defineCommand({
|
||||
// ── Resolve project ────────────────────────────────────────────────────
|
||||
const project = resolveProject(args.dir);
|
||||
|
||||
// ── Resolve composition entry file ─────────────────────────────────────
|
||||
// Needed early: fps default below must read the actual render target, not
|
||||
// always index.html.
|
||||
const entryFile = resolveCompositionEntryArg(args.composition, project.dir, statSync);
|
||||
|
||||
// ── Validate fps ───────────────────────────────────────────────────────
|
||||
// Accept either integer (`30`) or ffmpeg-style rational (`30000/1001`).
|
||||
// The whitelist-based validator was replaced with a sane numeric range so
|
||||
// legitimate framerates (NTSC trio, PAL, 120/240 slow-mo) work without
|
||||
// CLI gymnastics. The exact rational survives end-to-end into FFmpeg's
|
||||
// `-r` / `-framerate` flags via `fpsToFfmpegArg`.
|
||||
const fpsParse = parseFps(args.fps ?? "30");
|
||||
// Precedence: explicit --fps, else the composition's root data-fps, else 30.
|
||||
// Honoring data-fps matches the runtime — render used to silently force 30
|
||||
// even when the composition declared e.g. data-fps="24".
|
||||
const fpsArg = resolveDefaultFpsArg(args.fps, project.dir, project.indexPath, entryFile);
|
||||
const fpsParse = parseFps(fpsArg ?? "30");
|
||||
if (!fpsParse.ok) {
|
||||
errorBox("Invalid fps", formatFpsParseError(args.fps ?? "30", fpsParse.reason));
|
||||
errorBox("Invalid fps", formatFpsParseError(fpsArg ?? "30", fpsParse.reason));
|
||||
process.exit(1);
|
||||
}
|
||||
let fps: Fps = fpsParse.value;
|
||||
@@ -659,12 +673,11 @@ export default defineCommand({
|
||||
console.log(c.warn(" GIF output is capped at 30fps. Use --fps 15 for smaller files."));
|
||||
}
|
||||
|
||||
// ── Validate browser-timeout (seconds) and composition entry file ────
|
||||
// Both validators live in `utils/renderArgs.ts` so the parse/reject
|
||||
// ── Validate browser-timeout (seconds) ───────────────────────────────
|
||||
// This validator lives in `utils/renderArgs.ts` so the parse/reject
|
||||
// branches are unit-testable without `process.exit`. See issue #1199
|
||||
// for the original EISDIR / silent-timeout-0 footguns this guards.
|
||||
// for the original silent-timeout-0 footgun this guards.
|
||||
const pageNavigationTimeoutMs = resolveBrowserTimeoutMsArg(args["browser-timeout"]);
|
||||
const entryFile = resolveCompositionEntryArg(args.composition, project.dir, statSync);
|
||||
|
||||
// ── Preflight batch rows before browser/lint work ────────────────────
|
||||
let batchModule: typeof import("./batchRender.js") | undefined;
|
||||
|
||||
@@ -0,0 +1,107 @@
|
||||
import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import { readAllowedCompositionFpsFromDir, readCompositionFps } from "./compositionFps.js";
|
||||
|
||||
const wrap = (body: string) => `<!DOCTYPE html><html><body>${body}</body></html>`;
|
||||
|
||||
describe("readCompositionFps", () => {
|
||||
it("reads data-fps from the explicit data-root composition element", () => {
|
||||
const html = wrap('<div data-composition-id="root" data-root="true" data-fps="24">x</div>');
|
||||
expect(readCompositionFps(html)).toBe("24");
|
||||
});
|
||||
|
||||
it("reads data-fps from the outermost composition when no data-root is marked", () => {
|
||||
const html = wrap(
|
||||
'<div data-composition-id="root" data-fps="48"><div data-composition-id="child" data-fps="12">x</div></div>',
|
||||
);
|
||||
expect(readCompositionFps(html)).toBe("48");
|
||||
});
|
||||
|
||||
it("preserves a fractional rate verbatim for parseFps to validate", () => {
|
||||
const html = wrap(
|
||||
'<div data-composition-id="root" data-root="true" data-fps="30000/1001">x</div>',
|
||||
);
|
||||
expect(readCompositionFps(html)).toBe("30000/1001");
|
||||
});
|
||||
|
||||
it("returns null when the root has no data-fps", () => {
|
||||
expect(readCompositionFps(wrap('<div data-composition-id="root">x</div>'))).toBeNull();
|
||||
});
|
||||
|
||||
it("returns null when there is no composition root", () => {
|
||||
expect(readCompositionFps(wrap("<div>plain</div>"))).toBeNull();
|
||||
});
|
||||
|
||||
it("returns null for a blank data-fps", () => {
|
||||
expect(
|
||||
readCompositionFps(wrap('<div data-composition-id="root" data-fps=" ">x</div>')),
|
||||
).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe("readAllowedCompositionFpsFromDir", () => {
|
||||
const projectDirs: string[] = [];
|
||||
const allowedCloudFps = [24, 30, 60] as const;
|
||||
|
||||
afterEach(() => {
|
||||
for (const dir of projectDirs.splice(0)) {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
function makeProject(indexBody: string): string {
|
||||
const dir = mkdtempSync(join(tmpdir(), "hyperframes-composition-fps-"));
|
||||
projectDirs.push(dir);
|
||||
writeFileSync(join(dir, "index.html"), wrap(indexBody));
|
||||
return dir;
|
||||
}
|
||||
|
||||
it("uses cloud-allowed data-fps=60 as the default", () => {
|
||||
const dir = makeProject(
|
||||
'<div data-composition-id="root" data-root="true" data-fps="60">x</div>',
|
||||
);
|
||||
|
||||
expect(readAllowedCompositionFpsFromDir(dir, allowedCloudFps)).toBe(60);
|
||||
});
|
||||
|
||||
it("uses cloud-allowed data-fps=24 as the default", () => {
|
||||
const dir = makeProject(
|
||||
'<div data-composition-id="root" data-root="true" data-fps="24">x</div>',
|
||||
);
|
||||
|
||||
expect(readAllowedCompositionFpsFromDir(dir, allowedCloudFps)).toBe(24);
|
||||
});
|
||||
|
||||
it("returns null for data-fps=48 so cloud callers keep their ?? 30 fallback", () => {
|
||||
const dir = makeProject(
|
||||
'<div data-composition-id="root" data-root="true" data-fps="48">x</div>',
|
||||
);
|
||||
|
||||
const declared = readAllowedCompositionFpsFromDir(dir, allowedCloudFps);
|
||||
expect(declared).toBeNull();
|
||||
expect(declared ?? 30).toBe(30);
|
||||
});
|
||||
|
||||
it("returns null for fractional data-fps because cloud fps must be an integer", () => {
|
||||
const dir = makeProject(
|
||||
'<div data-composition-id="root" data-root="true" data-fps="30000/1001">x</div>',
|
||||
);
|
||||
|
||||
expect(readAllowedCompositionFpsFromDir(dir, allowedCloudFps)).toBeNull();
|
||||
});
|
||||
|
||||
it("returns null when index.html has no data-fps", () => {
|
||||
const dir = makeProject('<div data-composition-id="root" data-root="true">x</div>');
|
||||
|
||||
expect(readAllowedCompositionFpsFromDir(dir, allowedCloudFps)).toBeNull();
|
||||
});
|
||||
|
||||
it("returns null when index.html cannot be read", () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), "hyperframes-composition-fps-missing-"));
|
||||
projectDirs.push(dir);
|
||||
|
||||
expect(readAllowedCompositionFpsFromDir(dir, allowedCloudFps)).toBeNull();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,59 @@
|
||||
import { readFileSync } from "node:fs";
|
||||
import { join } from "node:path";
|
||||
import { parseHTML } from "linkedom";
|
||||
|
||||
/**
|
||||
* Read a composition's declared frame rate from its root element's `data-fps`
|
||||
* attribute — the same attribute the runtime honors (core/runtime/init.ts) — so
|
||||
* `hyperframes render` can default to it instead of a hard-coded 30 when `--fps`
|
||||
* is not passed. Returns the raw attribute string (for the caller to validate
|
||||
* via `parseFps`, which supports fractional rates like `30000/1001`), or `null`
|
||||
* when no root `data-fps` is present.
|
||||
*
|
||||
* Root resolution mirrors the runtime: prefer an explicit
|
||||
* `[data-composition-id][data-root="true"]`, else the outermost
|
||||
* `[data-composition-id]` (one with no `[data-composition-id]` ancestor).
|
||||
*/
|
||||
export function readCompositionFps(html: string): string | null {
|
||||
let doc: Document;
|
||||
try {
|
||||
doc = parseHTML(html).document as unknown as Document;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
|
||||
const explicitRoot = doc.querySelector('[data-composition-id][data-root="true"]');
|
||||
const root =
|
||||
explicitRoot ??
|
||||
Array.from(doc.querySelectorAll("[data-composition-id]")).find(
|
||||
(el) => !el.parentElement?.closest("[data-composition-id]"),
|
||||
) ??
|
||||
null;
|
||||
|
||||
const raw = root?.getAttribute("data-fps")?.trim();
|
||||
return raw ? raw : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Cloud render backends (Lambda, Cloud Run) accept only an integer fps
|
||||
* from a small fixed allowed set (currently {24, 30, 60}) — unlike local
|
||||
* `render`, they can't take an arbitrary/fractional data-fps. Reads
|
||||
* `<projectDir>/index.html` and returns its declared data-fps as a number
|
||||
* ONLY when it parses to an integer AND is a member of `allowed`;
|
||||
* otherwise `null` so the caller keeps its own existing default (30).
|
||||
*/
|
||||
export function readAllowedCompositionFpsFromDir(
|
||||
projectDir: string,
|
||||
allowed: readonly number[],
|
||||
): number | null {
|
||||
let html: string;
|
||||
try {
|
||||
html = readFileSync(join(projectDir, "index.html"), "utf8");
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
const raw = readCompositionFps(html);
|
||||
if (raw == null) return null;
|
||||
const n = Number(raw);
|
||||
return Number.isInteger(n) && allowed.includes(n) ? n : null;
|
||||
}
|
||||
@@ -1,11 +1,13 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import type { Stats } from "node:fs";
|
||||
import { resolve } from "node:path";
|
||||
import { mkdirSync, mkdtempSync, rmSync, writeFileSync, type Stats } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { dirname, join, resolve } from "node:path";
|
||||
import {
|
||||
MAX_PAGE_NAVIGATION_TIMEOUT_SECONDS,
|
||||
parseBrowserTimeoutMsArg,
|
||||
parseCompositionEntryArg,
|
||||
parseGifLoopArg,
|
||||
resolveDefaultFpsArg,
|
||||
type BrowserTimeoutParseResult,
|
||||
type CompositionEntryParseResult,
|
||||
} from "./renderArgs.js";
|
||||
@@ -173,6 +175,70 @@ describe("parseCompositionEntryArg", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("resolveDefaultFpsArg", () => {
|
||||
function writeComposition(path: string, fps: string): void {
|
||||
writeFileSync(
|
||||
path,
|
||||
`<!DOCTYPE html><html><body><div data-composition-id="root" data-root="true" data-fps="${fps}">x</div></body></html>`,
|
||||
);
|
||||
}
|
||||
|
||||
function makeProject(): {
|
||||
dir: string;
|
||||
indexPath: string;
|
||||
entryFile: string;
|
||||
cleanup: () => void;
|
||||
} {
|
||||
const dir = mkdtempSync(join(tmpdir(), "hyperframes-render-fps-"));
|
||||
const indexPath = join(dir, "index.html");
|
||||
const entryFile = "compositions/intro.html";
|
||||
const entryPath = join(dir, entryFile);
|
||||
writeComposition(indexPath, "24");
|
||||
mkdirSync(dirname(entryPath), { recursive: true });
|
||||
return {
|
||||
dir,
|
||||
indexPath,
|
||||
entryFile,
|
||||
cleanup: () => rmSync(dir, { recursive: true, force: true }),
|
||||
};
|
||||
}
|
||||
|
||||
it("reads data-fps from the --composition target instead of index.html", () => {
|
||||
const project = makeProject();
|
||||
try {
|
||||
writeComposition(join(project.dir, project.entryFile), "60");
|
||||
|
||||
expect(
|
||||
resolveDefaultFpsArg(undefined, project.dir, project.indexPath, project.entryFile),
|
||||
).toBe("60");
|
||||
} finally {
|
||||
project.cleanup();
|
||||
}
|
||||
});
|
||||
|
||||
it("falls back to index.html data-fps when --composition is not set", () => {
|
||||
const project = makeProject();
|
||||
try {
|
||||
expect(resolveDefaultFpsArg(undefined, project.dir, project.indexPath, undefined)).toBe("24");
|
||||
} finally {
|
||||
project.cleanup();
|
||||
}
|
||||
});
|
||||
|
||||
it("keeps an explicit --fps value ahead of any composition default", () => {
|
||||
const project = makeProject();
|
||||
try {
|
||||
writeComposition(join(project.dir, project.entryFile), "60");
|
||||
|
||||
expect(resolveDefaultFpsArg("120", project.dir, project.indexPath, project.entryFile)).toBe(
|
||||
"120",
|
||||
);
|
||||
} finally {
|
||||
project.cleanup();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("parseGifLoopArg", () => {
|
||||
it("accepts absent flag, bounds, and integers", () => {
|
||||
expect(parseGifLoopArg(undefined)).toEqual({ ok: true, value: undefined });
|
||||
|
||||
@@ -9,9 +9,11 @@
|
||||
* `timeout: 0` footguns at the rate of "one per missing branch".
|
||||
*/
|
||||
|
||||
import { readFileSync, type Stats } from "node:fs";
|
||||
import { resolve, sep } from "node:path";
|
||||
import { type Stats } from "node:fs";
|
||||
import { parseFps } from "@hyperframes/core";
|
||||
import { errorBox } from "../ui/format.js";
|
||||
import { readCompositionFps } from "./compositionFps.js";
|
||||
|
||||
// ── --browser-timeout ──────────────────────────────────────────────────
|
||||
|
||||
@@ -220,6 +222,32 @@ export function resolveCompositionEntryArg(
|
||||
return result.value;
|
||||
}
|
||||
|
||||
// ── default fps ────────────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
* Resolve the fps argument that local `render` should parse: explicit --fps,
|
||||
* else the actual composition entry file's root data-fps when valid, else
|
||||
* undefined so the caller can apply its final "30" default.
|
||||
*/
|
||||
export function resolveDefaultFpsArg(
|
||||
explicitFps: string | undefined,
|
||||
projectDir: string,
|
||||
indexPath: string,
|
||||
entryFile: string | undefined,
|
||||
): string | undefined {
|
||||
if (explicitFps != null) return explicitFps;
|
||||
try {
|
||||
const fpsSourcePath = entryFile ? resolve(projectDir, entryFile) : indexPath;
|
||||
const declared = readCompositionFps(readFileSync(fpsSourcePath, "utf8"));
|
||||
if (declared != null && parseFps(declared).ok) {
|
||||
return declared;
|
||||
}
|
||||
} catch {
|
||||
// Unreadable composition file — fall back to the default fps in render.ts.
|
||||
}
|
||||
return undefined;
|
||||
}
|
||||
|
||||
export type GifLoopParseResult =
|
||||
| { ok: true; value: number | undefined }
|
||||
| { ok: false; message: string };
|
||||
|
||||
Reference in New Issue
Block a user