Repository navigation
Angular: Install @analogjs/vite-plugin-angular in angular-to-angular-vite automigration - #35432
Conversation
…vite automigration
📝 WalkthroughWalkthroughThe Angular-to-Angular-Vite migration now adds ChangesAngular Vite dependency migration
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.test.ts (1)
364-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove mock setup into
beforeEach.This test configures
mockPromptConfirm,mockReadFile, andgetAllDependenciesinside the test body. Put these behaviors in a nestedbeforeEachfor the existing-plugin scenario so the test follows the repository’s setup convention.As per coding guidelines, “Implement mock behaviors in
beforeEachblocks in Vitest tests” and “Avoid inline mock implementations within test cases.”🤖 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/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.test.ts` around lines 364 - 370, Move the mockPromptConfirm, mockReadFile, and mockPackageManager.getAllDependencies setup for the existing-plugin scenario into a nested beforeEach, leaving the it test focused only on executing assertions and behavior. Preserve the current mocked values and scope the setup to that scenario.Source: Coding guidelines
🤖 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.
Inline comments:
In `@code/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.ts`:
- Around line 531-534: Update the duplicate check in the migration logic around
`packageManager.getAllDependencies()` to also inspect `optionalDependencies`
when determining whether `@analogjs/vite-plugin-angular` is already declared,
while preserving the existing dev-dependency behavior. Extend the
`getAllDependencies()` test helper contract as needed and add a regression test
covering the optional-dependency case.
---
Nitpick comments:
In
`@code/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.test.ts`:
- Around line 364-370: Move the mockPromptConfirm, mockReadFile, and
mockPackageManager.getAllDependencies setup for the existing-plugin scenario
into a nested beforeEach, leaving the it test focused only on executing
assertions and behavior. Preserve the current mocked values and scope the setup
to that scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1c04642-e260-4032-92ae-2323a53b52b8
📒 Files selected for processing (2)
code/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.test.tscode/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.ts
LogDetails |
|
Ran the following steps:
The dependency in the QA instructions installs, but Storybook fails with: Fixed following manual steps in #36008 but as a user I shouldn't have to do anything? |
Closes #
What I did
@analogjs/vite-plugin-angularis a required (non-optional) peer dependency of@storybook/angular-viteand is hard-imported by the framework preset at startup. Theangular-to-angular-viteautomigration swapped@storybook/angularfor@storybook/angular-vitebut never installed this peer. npm 7+ auto-installs missing peers, so the gap was invisible there, but Yarn and pnpm do not - users on those package managers ended up with a broken Storybook right after migrating.The automigration now adds
@analogjs/vite-plugin-angularas a devDependency alongside@storybook/angular-vite, mirroring the init generator. When the project already declares it, the existing version is left untouched.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
@storybook/angular), using yarn or pnpm (the bug does not reproduce on npm because npm auto-installs peers)npx storybook@0.0.0-pr-35432-sha-<sha> automigrate angular-to-angular-vite@analogjs/vite-plugin-angularwas added to the project's devDependenciesyarn storybook/pnpm storybookand verify the dev server starts (before this fix it fails with a module-not-found error for@analogjs/vite-plugin-angular)Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This pull request has been released as version
0.0.0-pr-35432-sha-8487b1ab. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-35432-sha-8487b1ab sandboxor in an existing project withnpx storybook@0.0.0-pr-35432-sha-8487b1ab upgrade.More information
0.0.0-pr-35432-sha-8487b1abvalentin/angular-vite-automigration-analogjs-peer-dep8487b1ab1783669734)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=35432Summary by CodeRabbit