Add typed JSON output to Hydrogen maintenance commands - #4103
gonzaloriestra wants to merge 1 commit into
Conversation
90028e0 to
a8c77b5
Compare
a8c77b5 to
9cc3998
Compare
9cc3998 to
6122bca
Compare
6122bca to
5ff252b
Compare
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
fredericoo
left a comment
There was a problem hiding this comment.
nice way to wrap up the stack - shortcut and upgrade follow the same execute/present split as the rest of the series, fatal errors go through the shared path, and the human output is unchanged. The changeset and README section cover the whole series well.
a couple of small comments but overall LGTM
| await displayUpgradeSummary({ | ||
| appPath: result.directory, | ||
| currentVersion: result.currentVersion, | ||
| selectedRelease: selectedRelease!, |
There was a problem hiding this comment.
non-blocking: presentUpgradeResult is exported and accepts status: 'upgraded' without a selectedRelease, so the only thing stopping displayUpgradeSummary crashing on selectedRelease.version is this !. It works today because runUpgrade always passes them together, but it's easy to hold wrong.
Let's make executeUpgrade return a discriminated union so the types enforce the pairing, yeah? Something like:
type UpgradeExecution =
| {result: UpgradeResult & {status: 'unchanged'}}
| {result: UpgradeResult & {status: 'upgraded'}; selectedRelease: Release};and have presentUpgradeResult take the whole UpgradeExecution. That also lets us swap the inline import('../../lib/maintenance/types.js').UpgradeResult annotations for a normal type import, since the module is already imported at the top.
| expect(outputMock.info()).toMatch( | ||
| / success.+ latest Hydrogen version/is, | ||
| ); | ||
| const {stdout} = await captureJsonOutput(() => runUpgrade({appPath})); |
There was a problem hiding this comment.
non-blocking: love that the unchanged path is covered end to end. The upgraded path is only covered with a hand-built result through the presenter though (in maintenance-json.test.ts), so nothing checks what executeUpgrade actually puts in packages, removedPackages and instructionsFile. Those are the new bits of logic in this PR.
Would be worth adding a JSON assertion to one of the existing real upgrade flows (e.g. the one around line 2418) so a regression in how the receipt is built gets caught.
WHY are these changes introduced?
Shortcut setup and upgrades complete the finite Hydrogen command migration.
WHAT is this pull request doing?
Add typed receipts to
shortcutandupgrade, keeping upgrade execution separate from final terminal output. Report installed versions, package changes and the instructions file, and use the shared fatal JSON path for unsupported shells. Document the complete JSON interface and add one CLI changeset for the stack. Only streaming commands remain in the shared exception list.Without
--json, the existing terminal presentation remains. Example JSON result:{"alias":"h2","shells":["zsh"]}HOW to test your changes?
pnpm --filter @shopify/cli-hydrogen test src/commands/hydrogen/maintenance-json.test.ts src/commands/hydrogen/upgrade.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 9 of 9. Previous: #4102. One changeset is included in #4103.