Repository navigation
Angular: Honor stylePreprocessorOptions.includePaths in angular-vite - #35483
valentinpalkovic merged 1 commit into
Conversation
The builder schema exposes SCSS search paths as `includePaths` (matching angular.json), but the framework preset read `loadPaths`, so the value was silently dropped end-to-end. Read `includePaths` (with `loadPaths` accepted as a dart-sass/Vite-spelling alias) and allow both spellings in the builder schemas. Also fix the docs: the "TypeScript paths" section claimed aliases work out of the box, but nothing maps tsconfig `paths` into Vite's resolver. Document the `vite-tsconfig-paths` workaround and how to configure SCSS include paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MoVAvMxLe3UVXfyh24btWK
📝 WalkthroughWalkthroughAngular Vite adds ChangesAngular Vite style path configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-vite/build-schema.json (1)
90-109: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd
stylePreprocessorOptions.sassto both Angular builder schemas.
builders.jsonpoints atbuild-schema.jsonandstart-schema.json, so these are the schemas Architect validates. Both still rejectsassunderstylePreprocessorOptionseven thoughsrc/preset.tsforwards it into SCSSpreprocessorOptions, which blocks valid Angular config likesilenceDeprecations.
code/frameworks/angular-vite/build-schema.json#L90-L109code/frameworks/angular-vite/start-schema.json#L103-L123🤖 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-vite/build-schema.json` around lines 90 - 109, Add a sass property to stylePreprocessorOptions in both code/frameworks/angular-vite/build-schema.json lines 90-109 and code/frameworks/angular-vite/start-schema.json lines 103-123, matching the forwarded SCSS preprocessorOptions shape so settings such as silenceDeprecations are accepted while preserving the existing schema validation.
🧹 Nitpick comments (2)
code/frameworks/angular-vite/src/preset.test.ts (1)
12-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock doesn't follow the
spy: true/beforeEachpattern required by guidelines.This module mock overrides
findConfigFilevia a manualimportOriginalfactory instead of{ spy: true }, and the override lives in the factory rather than abeforeEach. As per coding guidelines, "Usevi.mock()with thespy: trueoption for all package and file mocks" and "Implement mock behaviors inbeforeEachblocks."♻️ Suggested refactor to match the guideline pattern
-// The plugin's `config` hook looks up the preview file on disk before reading -// style options; stub just that lookup so the test stays hermetic. -vi.mock(import('storybook/internal/common'), async (importOriginal) => ({ - ...(await importOriginal()), - findConfigFile: () => undefined, -})); +// The plugin's `config` hook looks up the preview file on disk before reading +// style options; stub just that lookup so the test stays hermetic. +vi.mock(import('storybook/internal/common'), { spy: true });import { findConfigFile } from 'storybook/internal/common'; beforeEach(() => { vi.mocked(findConfigFile).mockReturnValue(undefined); });🤖 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-vite/src/preset.test.ts` around lines 12 - 15, Update the mock for storybook/internal/common to use the guideline-compliant spy-enabled vi.mock pattern, import findConfigFile, and move its undefined return configuration into a beforeEach block using vi.mocked(findConfigFile).mockReturnValue(undefined). Remove the manual importOriginal factory override.Source: Coding guidelines
docs/get-started/frameworks/angular-vite.mdx (1)
236-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIncomplete
viteFinalsnippet — inconsistent with other examples in this doc.Every other
viteFinalexample in this file is wrapped in the fullStorybookConfigobject withexport default config; this one is a bare method body, which isn't directly copy-pasteable.📝 Suggested fix
```ts title=".storybook/main.ts" -async viteFinal(config) { - const { mergeConfig } = await import('vite'); - return mergeConfig(config, { - css: { preprocessorOptions: { scss: { loadPaths: ['src/styles'] } } }, - }); -}, +import type { StorybookConfig } from '`@storybook/angular-vite`'; + +const config: StorybookConfig = { + framework: '`@storybook/angular-vite`', + async viteFinal(config) { + const { mergeConfig } = await import('vite'); + return mergeConfig(config, { + css: { preprocessorOptions: { scss: { loadPaths: ['src/styles'] } } }, + }); + }, +}; + +export default config;</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/get-started/frameworks/angular-vite.mdxaround lines 236 - 245, Expand
theviteFinalexample into a complete.storybook/main.tsconfiguration:
importStorybookConfig, define a typedconfigobject with the Angular Vite
framework and the existingviteFinalimplementation, then export it as the
default. Preserve the current SCSSloadPathsbehavior.</details> <!-- cr-comment:v1:fd9f3e8eb57aafae2b7b0d23 --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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-vite/build-schema.json:
- Around line 90-109: Add a sass property to stylePreprocessorOptions in both
code/frameworks/angular-vite/build-schema.json lines 90-109 and
code/frameworks/angular-vite/start-schema.json lines 103-123, matching the
forwarded SCSS preprocessorOptions shape so settings such as silenceDeprecations
are accepted while preserving the existing schema validation.
Nitpick comments:
In@code/frameworks/angular-vite/src/preset.test.ts:
- Around line 12-15: Update the mock for storybook/internal/common to use the
guideline-compliant spy-enabled vi.mock pattern, import findConfigFile, and move
its undefined return configuration into a beforeEach block using
vi.mocked(findConfigFile).mockReturnValue(undefined). Remove the manual
importOriginal factory override.In
@docs/get-started/frameworks/angular-vite.mdx:
- Around line 236-245: Expand the
viteFinalexample into a complete
.storybook/main.tsconfiguration: importStorybookConfig, define a typed
configobject with the Angular Vite framework and the existingviteFinal
implementation, then export it as the default. Preserve the current SCSS
loadPathsbehavior.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `16a87c2c-8fb0-41d3-9241-e0fb2164ca8c` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 4fd2dcc69a132fe1de290b70ec1abfe9702212c9 and 0abc583bbfd9d076123fc0d447ee351a0973828f. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `code/frameworks/angular-vite/build-schema.json` * `code/frameworks/angular-vite/src/preset.test.ts` * `code/frameworks/angular-vite/src/preset.ts` * `code/frameworks/angular-vite/start-schema.json` * `docs/get-started/frameworks/angular-vite.mdx` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
brandonroberts
changed the title
brandonroberts
added
ci:docs
valentinpalkovic
added
the
patch:yes
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 21.32 MB | 22.16 MB | 🚨 +834 KB 🚨 |
| Dependency size | 36.44 MB | 36.44 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 13 KB | 15 KB | 🚨 +2 KB 🚨 |
| Dependency size | 30.94 MB | 30.94 MB | 🚨 +657 B 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 533 | 533 | 0 |
| Self size | 640 KB | 663 KB | 🚨 +23 KB 🚨 |
| Dependency size | 62.28 MB | 62.31 MB | 🚨 +35 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 93 | 93 | 0 |
| Self size | 1.37 MB | 1.39 MB | 🚨 +22 KB 🚨 |
| Dependency size | 23.97 MB | 24.01 MB | 🚨 +38 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 122 | 122 | 0 |
| Self size | 29 KB | 30 KB | 🚨 +970 B 🚨 |
| Dependency size | 25.04 MB | 25.08 MB | 🚨 +38 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 83 | 83 | 0 |
| Self size | 32 KB | 36 KB | 🚨 +4 KB 🚨 |
| Dependency size | 21.75 MB | 21.78 MB | 🚨 +34 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 274 | 274 | 0 |
| Self size | 23 KB | 23 KB | 🚨 +789 B 🚨 |
| Dependency size | 48.06 MB | 48.09 MB | 🚨 +35 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/tanstack-react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 84 | 84 | 0 |
| Self size | 112 KB | 110 KB | 🎉 -2 KB 🎉 |
| Dependency size | 21.78 MB | 21.82 MB | 🚨 +38 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 205 | 205 | 0 |
| Self size | 827 KB | 827 KB | 🎉 -168 B 🎉 |
| Dependency size | 91.32 MB | 92.16 MB | 🚨 +834 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 198 | 198 | 0 |
| Self size | 32 KB | 32 KB | 🎉 -43 B 🎉 |
| Dependency size | 89.80 MB | 90.63 MB | 🚨 +834 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 74 | 74 | 0 |
| Self size | 1.09 MB | 1.09 MB | 🎉 -654 B 🎉 |
| Dependency size | 57.77 MB | 58.60 MB | 🚨 +834 KB 🚨 |
| Bundle Size Analyzer | node | node |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 59 | 59 | 0 |
| Self size | 1.49 MB | 1.52 MB | 🚨 +35 KB 🚨 |
| Dependency size | 12.45 MB | 12.45 MB | 🎉 -97 B 🎉 |
| Bundle Size Analyzer | Link | Link |
…/include-paths-alias Angular: Honor stylePreprocessorOptions.includePaths in angular-vite (cherry picked from commit 77b861c)
ndelangen
added
patch:done
3 participants
The builder schema exposes SCSS search paths as
includePaths(matching angular.json), but the framework preset readloadPaths, so the value was silently dropped end-to-end. ReadincludePaths(withloadPathsaccepted as a dart-sass/Vite-spelling alias) and allow both spellings in the builder schemas.Also fix the docs: the "TypeScript paths" section claimed aliases work out of the box, but nothing maps tsconfig
pathsinto Vite's resolver. Document thevite-tsconfig-pathsworkaround and how to configure SCSS include paths.Closes #
What I did
@storybook/angular-vite's preset readstylePreprocessorOptions.loadPaths, but the builder schema (andangular.json) expose the field asincludePaths, so user-configured SCSS include paths were silently dropped. The preset now readsincludePaths, keepingloadPathsas a dart-sass/Vite-spelling alias, and both spellings are allowed in thestart-storybook/build-storybookschemas.angularOptionsPlugin(resolution, alias, precedence,sasspassthrough, empty config).tsconfigpathsinto Vite's resolver, so documented thevite-tsconfig-paths/resolve.aliasworkaround — and added a "SCSS include paths" section coveringincludePaths/loadPathson the builder target and the CLI-onlyviteFinalpath.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Using the
angular-vitesandbox (yarn task --task sandbox --start-from auto --template angular-vite/default-ts):src/styles/_theme.scsscontaining$primary: hotpink;.angular.jsonto include that folder, using the Angular spelling:@use 'theme';(relying on the include path, not a relative path).ng run <project>:storybookand confirm the story renders with the styles applied — before this fix the build fails to resolvethemebecauseincludePathswas ignored."loadPaths"instead of"includePaths"and confirm it behaves identically (alias).Docs-only changes (TypeScript paths / SCSS include paths sections of
docs/get-started/frameworks/angular-vite.mdx) require no runtime testing — review the rendered MDX for accuracy.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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
includePathsorloadPaths.Documentation