Angular: Fix custom paths for stats.json on angular#34551
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReorder JSON-schema ChangesSchema Type Reordering
Build-storybook tests
Start-storybook tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
|
Hi @mrginglymus, could you please fix the TypeScript issues? |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/frameworks/angular/src/builders/start-storybook/index.spec.ts (1)
127-245:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a regression test for string-valued stats options.
The
start-storybookbuilder forwards bothstatsJsonandwebpackStatsJsontobuildDevStandalone, but the test suite never schedules the builder with these set to a string value. Since this PR reorders schema unions specifically to preserve string paths for stats options, add a test that verifies the string value is correctly passed through—similar to the coverage already present in thebuild-storybookspec (lines 181, 201).Test shape
+ it('should forward string statsJson values', async () => { + const run = await architect.scheduleBuilder('@storybook/angular:start-storybook', { + tsConfig: 'path/to/tsConfig.json', + port: 4400, + compodoc: false, + statsJson: './custom-stats.json', + }); + + const output = await run.result; + + await run.stop(); + + expect(output.success).toBeTruthy(); + expect(buildDevStandalone).toHaveBeenCalledWith( + expect.objectContaining({ + statsJson: './custom-stats.json', + }) + ); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/frameworks/angular/src/builders/start-storybook/index.spec.ts` around lines 127 - 245, Add a regression test that schedules the '@storybook/angular:start-storybook' builder with string-valued stats options and asserts they are forwarded to buildDevStandalone; specifically, create a new it block (mirroring existing tests like "should start storybook with tsConfig") that calls architect.scheduleBuilder with statsJson: 'path/to/stats.json' and webpackStatsJson: 'path/to/webpack-stats.json', awaits run.result, stops the run, and then expects buildDevStandalone toHaveBeenCalledWith an objectContaining where statsJson and webpackStatsJson equal those string paths (use the existing test patterns and mocks such as buildDevStandalone, mockRunScript, and expect.objectContaining to locate the assertion).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@code/frameworks/angular/src/builders/start-storybook/index.spec.ts`:
- Around line 127-245: Add a regression test that schedules the
'@storybook/angular:start-storybook' builder with string-valued stats options
and asserts they are forwarded to buildDevStandalone; specifically, create a new
it block (mirroring existing tests like "should start storybook with tsConfig")
that calls architect.scheduleBuilder with statsJson: 'path/to/stats.json' and
webpackStatsJson: 'path/to/webpack-stats.json', awaits run.result, stops the
run, and then expects buildDevStandalone toHaveBeenCalledWith an
objectContaining where statsJson and webpackStatsJson equal those string paths
(use the existing test patterns and mocks such as buildDevStandalone,
mockRunScript, and expect.objectContaining to locate the assertion).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c89c2b6f-74d4-4497-9292-02fabe923b47
📒 Files selected for processing (1)
code/frameworks/angular/src/builders/start-storybook/index.spec.ts
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
…tart-storybook Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes #
What I did
Running the angular builder with `--stats-json=` seems to be parsed as `false`. This means the chromatic will fail to run turbosnap, as it will not generate a stats file.
I'm not sure how long this has been the case, but I suspect it might be 'forever', and only recently a change to chromatic cli has made it stricter about the presence of the stats file.
It seems to stem from the angular schema being defined as `["boolean", "string"]`, and presumably angular attempting to parse them in order - and seeing as `` isn't `true` it must be `false`. Swapping the order fixes the problem.
Tests are now unskipped and passing after modernising the mocking approach.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Documentation
Checklist for Maintainers