-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Angular: Inherit options from browserTarget #32108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Angular: Inherit options from browserTarget #32108
Conversation
…m browserTarget Fixes storybookjs#32105 - Angular + Webpack not picking up styles, assets and stylePreprocessorOptions from browserTarget - Replaced shallow spread merge with deep merge in getBuilderOptions() - Added deepMerge function that preserves nested properties from browserTarget - Ensures stylePreprocessorOptions.includePaths, assets, and styles are inherited when not explicitly set in storybook target - Maintains backward compatibility and override behavior when storybook options are provided - Fixes regression introduced in v9 where nested configuration was being lost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 1 comment
code/frameworks/angular/src/server/framework-preset-angular-cli.test.ts
Outdated
Show resolved
Hide resolved
- Export deepMerge function from main file for testing - Remove duplicate deepMerge implementation in test file - Import actual deepMerge function in tests to prevent implementation drift - Add proper typing to fix TypeScript errors Addresses Greptile bot feedback about maintenance risk of code duplication
|
View your CI Pipeline Execution ↗ for commit 635f853
☁️ Nx Cloud last updated this comment at |
|
Could you please review the linting errors and the failing unit test? |
- Remove unused vi import to fix linting error - Keep vitest imports that are actually used (expect, describe, it)
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
Folks, could you please get this over the finish line? we updated our Nx workspace to latest including stroybook 9 and now all our storybooks are broken :( |
d286e59 to
2e1f6c5
Compare
Angular: Inherit options from browserTarget (cherry picked from commit b0f9e0d)
|
this PR does unfortunely not fix the problem. there is still the same merge result, where options from the browserTarget config are ignored, like styles, assets, etc tested with [email protected] |
|
@ctaepper is the problem addressed or still it persist. I can work on this issue and get this addressed with in this week |
|
The problem still persits. The source object ( my concrete objects: source: {
"stylePreprocessorOptions": {
"includePaths": []
},
"styles": [],
"assets": [],
"preserveSymlinks": false,
"sourceMap": false,
"experimentalZoneless": false
}target: {
"outputPath": "dist/apps/web/primeng-demo",
"index": "apps/web/primeng-demo/src/index.html",
"browser": "apps/web/primeng-demo/src/main.ts",
"polyfills": [
"zone.js",
"@angular/localize/init"
],
"tsConfig": "apps/web/primeng-demo/tsconfig.app.json",
"inlineStyleLanguage": "scss",
"assets": [
{
"glob": "**/*",
"input": "apps/web/primeng-demo/public"
}
],
"styles": [
"apps/web/primeng-demo/src/theme-overrides.scss",
"apps/web/primeng-demo/src/styles.scss"
],
"scripts": [],
"localize": true,
"budgets": [
{
"type": "initial",
"maximumWarning": "500kb",
"maximumError": "1mb"
},
{
"type": "anyComponentStyle",
"maximumWarning": "4kb",
"maximumError": "8kb"
}
],
"outputHashing": "all"
}and the result: {
"outputPath": "dist/apps/web/primeng-demo",
"index": "apps/web/primeng-demo/src/index.html",
"browser": "apps/web/primeng-demo/src/main.ts",
"polyfills": [
"zone.js",
"@angular/localize/init"
],
"tsConfig": "apps/web/primeng-demo/tsconfig.app.json",
"inlineStyleLanguage": "scss",
"assets": [], <-- should not be empty
"styles": [], <-- should not be empty
"scripts": [],
"localize": true,
"budgets": [
{
"type": "initial",
"maximumWarning": "500kb",
"maximumError": "1mb"
},
{
"type": "anyComponentStyle",
"maximumWarning": "4kb",
"maximumError": "8kb"
}
],
"outputHashing": "all",
"stylePreprocessorOptions": {
"includePaths": []
},
"preserveSymlinks": false,
"sourceMap": false,
"experimentalZoneless": false
}what interesting, is that the budgets (array of objects) are mergerd, but assets which is also an array of objects are not merged. and of course the styles property is missing. |
Closes #32105 - Angular + Webpack not picking up styles, assets and stylePreprocessorOptions from browserTarget
Angular framework properly inherits stylePreprocessorOptions from browserTarget
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
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.tsMake 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 canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
This PR fixes a regression in Storybook's Angular framework where nested configuration properties like
stylePreprocessorOptions.includePathswere being lost during configuration merging. The issue occurred in v9 when the framework switched to shallow spread merging ({...browserTargetOptions, ...options.angularBuilderOptions}), which overwrote entire nested objects instead of preserving inherited properties from the browserTarget.The fix introduces a custom
deepMergefunction that recursively merges nested objects while preserving arrays and handling null/undefined values properly. This ensures that when Storybook-specific options don't specify nested properties, they are inherited from the browserTarget configuration, maintaining the expected behavior from v8 while still allowing complete override when needed.The implementation adds comprehensive unit tests covering various merge scenarios including preservation of browserTarget options, override behavior, deep merging of nested objects, array handling, and complex nested structures. The
tsConfigproperty continues to be handled separately to maintain existing fallback logic for locating TypeScript configuration files.This change integrates with the existing Angular CLI webpack preset system in
framework-preset-angular-cli.ts, specifically in thegetBuilderOptionsfunction that prepares configuration for the Angular build system within Storybook's webpack pipeline.Confidence score: 4/5
• This is a well-implemented fix for a specific regression with comprehensive test coverage
• The implementation correctly handles edge cases and maintains backward compatibility
• The test file duplicates the deepMerge implementation instead of testing the actual exported function, which could lead to implementation drift