-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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: Split @storybook/angular into three packages #29738
base: next
Are you sure you want to change the base?
Angular: Split @storybook/angular into three packages #29738
Conversation
7419999
to
7bef214
Compare
☁️ Nx Cloud ReportCI is running/has finished running commands for commit ce735ea. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
Before | After | Difference | |
---|---|---|---|
Dependency count | 261 | 286 | 🚨 +25 🚨 |
Self size | 362 KB | 106 KB | 🎉 -257 KB 🎉 |
Dependency size | 33.56 MB | 48.38 MB | 🚨 +14.82 MB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/angular-renderer
Before | After | Difference | |
---|---|---|---|
Dependency count | 0 | 38 | 🚨 +38 🚨 |
Self size | 0 B | 442 KB | 🚨 +442 KB 🚨 |
Dependency size | 0 B | 14.60 MB | 🚨 +14.60 MB 🚨 |
Bundle Size Analyzer | Link | Link |
ed72b53
to
e31f283
Compare
ed1b20e
to
503e1bf
Compare
503e1bf
to
c3cfdb1
Compare
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.
250 file(s) reviewed, 145 comment(s)
Edit PR Review Bot Settings | Greptile
code/core/src/common/versions.ts
Outdated
@@ -71,13 +71,15 @@ export default { | |||
'@storybook/react-dom-shim': '8.5.0-alpha.19', | |||
'@storybook/source-loader': '8.5.0-alpha.19', | |||
'@storybook/test': '8.5.0-alpha.19', | |||
'@storybook/preset-angular-webpack': '8.5.0-alpha.18', |
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.
logic: version mismatch with other packages (alpha.18 vs alpha.19) could cause compatibility issues
code/core/src/common/versions.ts
Outdated
'@storybook/preset-create-react-app': '8.5.0-alpha.19', | ||
'@storybook/preset-html-webpack': '8.5.0-alpha.19', | ||
'@storybook/preset-preact-webpack': '8.5.0-alpha.19', | ||
'@storybook/preset-react-webpack': '8.5.0-alpha.19', | ||
'@storybook/preset-server-webpack': '8.5.0-alpha.19', | ||
'@storybook/preset-svelte-webpack': '8.5.0-alpha.19', | ||
'@storybook/preset-vue3-webpack': '8.5.0-alpha.19', | ||
'@storybook/angular-renderer': '8.5.0-alpha.11', |
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.
logic: version significantly behind other packages (alpha.11 vs alpha.19) may lead to compatibility problems
[ | ||
'builders/builders.json', | ||
'builders/start-storybook/schema.json', | ||
'builders/build-storybook/schema.json', | ||
].forEach((filePath) => { | ||
const srcPath = path.join(__dirname, `../src/${filePath}`); | ||
const distPath = path.join(__dirname, `../dist/${filePath}`); | ||
|
||
fs.copyFileSync(srcPath, distPath); | ||
}); |
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.
logic: original type compatibility fix for Angular <17.2 was removed - this could break builds for users on older Angular versions
@@ -1,4 +1,6 @@ | |||
export * from './client/index'; | |||
// For backwarts compatibility |
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.
syntax: 'backwarts' is misspelled
@@ -4,5 +4,5 @@ | |||
"noEmit": true, | |||
"strict": false |
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.
style: strict: false disables TypeScript's strict checks. Consider enabling strict mode for better type safety.
size = input('medium', { | ||
transform: (val: 'small' | 'medium') => val, | ||
}); |
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.
style: size transform function is redundant since it just returns the input value without transformation
color: white; | ||
} | ||
.storybook-button--secondary { | ||
box-shadow: rgba(0, 0, 0, 0.15) 0px 0px 0px 1px inset; |
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.
style: box-shadow shorthand could be simplified to inset 0 0 0 1px rgba(0, 0, 0, 0.15)
"extends": "./tsconfig.build.json", | ||
"compilerOptions": { | ||
"noEmit": true, | ||
"strict": false |
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.
style: Consider enabling strict mode for better type safety. This helps catch potential type-related bugs early in development.
"noEmit": true, | ||
"strict": false | ||
}, | ||
"include": ["src/**/*", "src/**/*.json", "template/**/*"] |
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.
style: Including JSON files and templates in TypeScript compilation may cause unnecessary processing. Consider moving non-TypeScript files to a separate copy step in the build process.
"target": "ES2020", | ||
"module": "CommonJS", |
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.
style: ES2020 target with CommonJS modules may limit tree-shaking opportunities. Consider using ESM modules instead
50eb68b
to
ce735ea
Compare
3c2ff04
to
d68932f
Compare
bab0d62
to
be85e78
Compare
be85e78
to
d2f0499
Compare
Split @storybook/angular int @storybook/{anghular, angular-renderer, angular-webpack}
Closes #
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:merged
orci:daily
GH 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.ts
Make 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/core
team 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
Here's my review of the changes to split @storybook/angular into three separate packages:
This PR splits @storybook/angular into three focused packages (@storybook/angular, @storybook/angular-renderer, and @storybook/preset-angular-webpack) to better organize Angular-specific functionality and improve maintainability.
@storybook/angular-renderer
package including components, decorators, and story utilities@storybook/preset-angular-webpack
for webpack configuration and build toolingThe changes look well-structured and maintain existing functionality while improving code organization. The separation of concerns between rendering, webpack configuration, and the main framework package follows good architectural practices.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!