Add typed JSON output to hydrogen deploy - #4098
gonzaloriestra wants to merge 1 commit into
Conversation
137c307 to
217879b
Compare
217879b to
f3d45cb
Compare
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
f3d45cb to
e3794de
Compare
e3794de to
589de46
Compare
fredericoo
left a comment
There was a problem hiding this comment.
nice and simple - splitting executeDeploy from the presenter in runDeploy reads well, human-mode output is unchanged, and h2_deploy_log.json keeps the same bytes and stays independent of --json (with a test for it). Errors go through the shared JSON error path from #4095, and the two helpers that used to warn-and-return now throw in JSON mode so we don't end up emitting null for what's really a failure.
a couple of small comments on the JSON-mode custom build hook, otherwise LGTM
| level: 'warning', | ||
| message: stderr.trim(), | ||
| }); | ||
| } catch (error) { |
There was a problem hiding this comment.
non-blocking: when the build fails we drop whatever it printed. exec's error message has stderr in it, but stdout is lost, and some build tools print the useful bit to stdout. Since the failed build is when you most need the logs, let's emit the captured output as diagnostics here too before rethrowing. IIRC the exec rejection error has stdout/stderr on it.
Some smaller tradeoffs vs oxygen-cli's own runner, which already pipes all build output to stderr, so stdout was never at risk. Fine if these are deliberate:
- output is buffered until the build finishes, so long builds are silent in JSON mode
maxBuffermeans a very chatty build (>64MB) fails only in JSON mode (unlikely)- oxygen-cli's Bugsnag
buildCommandmetadata isn't recorded on this path
| } | ||
| }); | ||
|
|
||
| it('writes a single deployment result and keeps diagnostics off stdout', async () => { |
There was a problem hiding this comment.
non-blocking: the JSON-mode buildFunction for --build-command is the main new behaviour in this PR, but nothing tests it. Let's add a test that runs with buildCommand + json: true and checks the build output ends up as diagnostic events rather than on stdout. The new isJsonOutput() throws in getOxygenDeploymentData and renderMissingStorefront could use a quick test each too (get-oxygen-deployment-data.test.ts already exists).
WHY are these changes introduced?
Deployment needs a typed stdout result while preserving the existing CI deployment-file contract.
WHAT is this pull request doing?
Separate deployment execution from its final presentation. Add
--jsonfor the completed deployment ornullon cancellation, capture custom build output as diagnostics, and flush the result before exiting. Keep--json-outputindependent and preserve the existing deployment file bytes.Without
--json, the existing terminal presentation remains. Example JSON result:{"url":"https://example.com"}HOW to test your changes?
pnpm --filter @shopify/cli-hydrogen test src/commands/hydrogen/deploy.test.tsBuild and typecheck were checked at each layer. The full stack passed 498 tests (6 skipped), lint, and schema discovery for all 22 finite commands. Command-line smoke checks covered a successful unlink result, fatal project errors, watch-mode rejection and schema environment flags. Live authenticated Shopify operations have not been exercised.
Stack
Draft 4 of 9. Previous: #4097. Next: #4099. One changeset is included in #4103.