Add typed JSON output to Hydrogen build tooling - #4096
gonzaloriestra wants to merge 1 commit into
Conversation
75cb4b0 to
db7eb48
Compare
db7eb48 to
7052847
Compare
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
7052847 to
43c52f2
Compare
43c52f2 to
d964345
Compare
fredericoo
left a comment
fredericoo
left a comment
There was a problem hiding this comment.
nice and tidy - pulling checkRoutes out as a pure function returning the domain result is a good shape, and the human output looks unchanged. A few small comments, mainly Vite's TTY progress output possibly getting into stdout in JSON mode, and codegen detecting --json differently from build/check.
| customLogger.error = (msg) => collectLog('error', msg); | ||
| } | ||
|
|
||
| if (isJsonOutput()) { |
There was a problem hiding this comment.
non-blocking: overriding the logger methods covers the logs that go through customLogger, but IIRC Vite's build reporter also writes progress straight to process.stdout (transforming (N) ..., rendering chunks (N)..., computing gzip size (N)... plus the matching clearLine escapes) when process.stdout.isTTY && !process.env.CI. It checks config.logLevel, not the custom logger, so that path is untouched here.
build --json | jq is fine because stdout isn't a TTY there, but anything that runs the command under a PTY (some agent harnesses, script, etc.) might end up with progress fragments before the JSON document, and parsing would fail.
Setting logLevel: 'warn' in commonConfig when isJsonOutput() should switch the reporter off, if I'm not mistaken. We'd lose the N modules transformed / chunk table diagnostics, which seems fine for JSON consumers. What do you reckon?
|
|
||
| if (!watch) { | ||
| const result = {generatedFiles}; | ||
| if (!watch && !writeJsonResult(codegenJsonOutputSchema, result)) { |
There was a problem hiding this comment.
non-blocking: build and check both pass flags.json to writeJsonResult, but runCodegen never gets it and falls back to the ambient isJsonOutput(). They should agree in practice, but let's pass it through the same way so all three commands in this PR decide JSON mode the same way, yeah? It also means the Codegen tests exercise the flag rather than only captureJsonOutput's env/argv setup.
| }); | ||
|
|
||
| it('encodes build output paths through the real writer', async () => { | ||
| const result = { |
There was a problem hiding this comment.
non-blocking: this test only covers writeJsonResult and the schema. It doesn't run Build or runBuild, so if the writeJsonResult call in Build.run or the Vite logger rerouting to diagnostics broke, it would still pass. The Vite build is a lot to mock, so not fussed about full coverage, but even a test that stubs runBuild and checks that Build.run writes result.result to stdout with --json would cover the wiring. process.exit would need stubbing too.
This branch has not been deployed
2 participants
WHY are these changes introduced?
Build, route checks and code generation currently expose terminal output that scripts cannot consume as a typed result.
WHAT is this pull request doing?
Add domain schemas and
--jsontobuild,checkandcodegen, with schema discovery and generated help. Keep build/codegen diagnostics separate from stdout, capture type-generation subprocess output, and reject--json --watchbefore execution.Without
--json, the existing terminal presentation remains. Example JSON result:{"missingRoutes":[],"reservedRoutes":[]}HOW to test your changes?
pnpm --filter @shopify/cli-hydrogen test src/commands/hydrogen/build-json.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 2 of 9. Previous: #4095. Next: #4097. One changeset is included in #4103.