fix(core): harden runtime resolution, inline constant, CI smoke test#458
fix(core): harden runtime resolution, inline constant, CI smoke test#458miguel-heygen merged 1 commit intomainfrom
Conversation
340ce52 to
6947d6f
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Verdict: approve
Squash is faithful to the two reviews I already went through — #455 (CI smoke) + #457 (core hardening + inline constant). I diffed the composite hunks against the originals:
- ci.yml
smoke-global-install+.github/workflows/**filter — same as #455. loadRuntimeSourcethree-strategy chain withgetInlinedRuntimebetweenbuildFromSourceandreadPrebuiltArtifact— same as #457.buildHyperframesRuntimeScript(): string | nullwithexistsSync(entryPath)guard — same.- Generated
src/generated/runtime-inline.tsvia the JSON-escaped IIFE constant, gitignored, wired intopackages/core/src/index.ts— same. - Build order flip
build:hyperframes-runtime && tsc— same. - All caller-side null-handling (
loadHyperframeRuntimeSource, core dev scripts, enginetest-fitTextFontSize-browser.ts,build-hyperframes-runtime-artifact.ts) — same.
New in the squash: uniform 0.4.15 → 0.4.16 bump across cli, core, engine, player, producer, shader-transitions, studio. Correct for a release cut that carries both #452's shipping fix and this hardening — the inlined-constant export is a new public API on @hyperframes/core, so a minor-revision bump (rather than patch) would also be defensible, but 0.4.16 is fine given the pre-1.0 posture.
Prior non-blockers still apply (see <https://github.com/heygen-com/hyperframes/pull/455#pullrequestreview-4165273519|#455 review> and <https://github.com/heygen-com/hyperframes/pull/457#pullrequestreview-4165276834|#457 review>): npx variant follow-up, fresh-clone typecheck footgun for src/generated/ (postinstall fix), trivial tarball bloat, and the housekeeping ask to promote Smoke: global install to a required check in branch protection once this lands.
Minor observation: the PR body mentions "oxfmt trailing blank line fix in chunkEncoder.test.ts" but that hunk isn't in the #458 diff — it was part of #452, already on main. Body line is stale; not a review concern.
CI at review time: the initial run was cancelled on all core jobs (Build/Typecheck/Test/Lint/Windows) — probably a force-push retrigger. A second run is in flight (regression shards, perf, Windows render in_progress, Smoke: global install cancelled-and-presumably-queued). My approval is on code merit; gate the merge on green CI as usual, with particular attention to Typecheck given the generated-file arithmetic.
Ship it once CI settles green.
Review by hyperframes
… test Guard buildHyperframesRuntimeScript() against missing entry.ts so it returns null instead of crashing with esbuild stderr output. Add getHyperframeRuntimeScript() that returns the pre-built IIFE as a baked-in string constant — no esbuild, no file I/O, no import.meta.url. Consolidate CLI runtime source resolution into a single module with a clear priority chain: esbuild from source (dev) → inlined constant (production) → pre-built artifact file (fallback). Add CI smoke test that npm-packs the CLI, installs globally, runs hyperframes preview, and asserts no stderr errors + runtime endpoint returns JS. Bump version to 0.4.16.
6947d6f to
e51a682
Compare
Summary
Hardens the runtime resolution pipeline end-to-end so the #452 bug class can't recur, and adds a CI smoke test to catch it if it does.
Core hardening
buildHyperframesRuntimeScript()in@hyperframes/coreused to callesbuild.buildSync()unconditionally onsrc/runtime/entry.ts. When any consumer bundles core (tsup, webpack, etc.),import.meta.urlresolves to the bundle — not the source tree — and esbuild crashes with✘ [ERROR]on stderr before throwing. The JS-levelcatchcan't suppress that.Fix:
existsSync(entryPath)guard before calling esbuild. Returnsnullwhenentry.tsis missing. No stderr, no crash, no consumer-side workaround needed.Inlined runtime constant
New export
getHyperframeRuntimeScript()— the build script bakes the IIFE intosrc/generated/runtime-inline.tsas a string constant. No esbuild, no file I/O, noimport.meta.urlpath arithmetic. This is the production-safe default.The split follows the design from the review:
loadHyperframeRuntimeSource()stays as the dev-only helper (esbuild fromentry.ts),getHyperframeRuntimeScript()is the production path (baked-in constant).CLI resolution chain
loadRuntimeSource()in the CLI now has a clear priority chain:entry.tsexistencegetHyperframeRuntimeScript(), production defaultdist/, fallback for edge casesCI smoke test
New
smoke-global-installjob that reproduces the exact user flow:npm pack→ install globally with--prefixhyperframes init --example blankhyperframes preview --port 3099GET /api/runtime.jsreturns non-empty JS✘ [ERROR]orFailed to load runtimeValidated: grep pattern catches the pre-#452 broken state (verified against 0.4.15-alpha.1) and passes on fixed code. CI green.
Other
.github/workflows/**added to change detection filterchunkEncoder.test.tsTest plan
bun run buildpassesbun run --cwd packages/core test— 514 tests passbun run --cwd packages/cli test— 161 tests passnpm pack --dry-runconfirmsdist/generated/runtime-inline.jsships