Skip to content

fix(cli): --variables-file reads the file instead of failing with io.readFile is not a function - #6591

Closed
cathrynlavery wants to merge 3 commits into
heygen-com:mainfrom
cathrynlavery:fix/variables-file-default-reader
Closed

cathrynlavery wants to merge 3 commits into
heygen-com:mainfrom
cathrynlavery:fix/variables-file-default-reader

Conversation

@cathrynlavery

Copy link
Copy Markdown
Contributor

What

In 0.8.209, --variables-file fails on render, lambda render, cloudrun and cloud render:

✗  Could not read the variables JSON
   vars.json: io.readFile is not a function

With this change, readJsonFlagText uses its default reader whenever a caller passes undefined for readFile or readStdin.

Why

resolveVariablesArg calls parseVariablesArg(inline, filePath) without readers, and parseVariablesArg forwards { readFile, readStdin }, both undefined, to readJsonFlagText. There, { ...defaultIO, ...partialIo } copies the undefined values over the defaults. Every existing test injects a reader, so none of them reached the default.

To reproduce on 0.8.209, in any project:

echo '{"title":"x"}' > vars.json
npx hyperframes render --variables-file vars.json --output out.mp4

Related work

Regression from #6518.

How

readJsonFlagText picks each reader with ?? instead of spreading partialIo, so an explicit undefined keeps the default. The fix sits in the shared reader, so any other caller that forwards optional readers gets it too.

Test plan

  • Unit tests added/updated: parseVariablesArg(undefined, <path>) with no reader reads a real file. On main it fails with cause: "io.readFile is not a function".
  • Manual testing performed: A one-second project with data-var-text="title", rendered with --variables-file vars.json --strict-variables. 0.8.209 exits 1 with the error above. This branch renders, and the frame shows the title from the file.
  • Typecheck passes in packages/cli, and the variables, jsonFlag and media-treatment tests pass (56). In the full CLI suite, the failures on this Mac also fail on main (ffmpeg track decode, whisper/sherpa runtime, Chromium WebGL sweep), apart from two file-watcher timing tests that pass when run alone.
  • Documentation updated (if applicable): Not needed.
  • Comments follow CONTRIBUTING.md "Comments": No comments added. check-comment-citations and comment-ratchet pass.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed at 3c6118866a87f0414067967dfa222b8ac5f6d392.

The explicit nullish defaults fix undefined injected file readers. Please rebase onto main before I approve the resulting tree.

vitest run jsonFlag.test.ts --configLoader runner: 4 passed. No full integration run is claimed.

Need: yes · Size: ok · Reuse: existing helper and dispatch paths · Simpler: keep the focused fix · Structure: fine
Risk: medium - compatibility with current main.

Verdict: COMMENT
Reasoning: The branch currently conflicts with main; I will review the resolved delta after a rebase.

Reasoning: The explicit nullish defaults fix undefined injected file readers. Please rebase onto main before I approve the resulting tree.

@cathrynlavery

Copy link
Copy Markdown
Contributor Author

#6831 merged the same fix, so I'm closing this one. Thanks for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants