Core: Improve mono repo handling for JsPackageManager class and helpers#31553
Conversation
|
View your CI Pipeline Execution ↗ for commit 1239c0f.
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
68 file(s) reviewed, 24 comment(s)
Edit PR Review Bot Settings | Greptile
| 'Force package manager for installing dependencies' | ||
| ) | ||
| .option('-c, --config-dir <dir-name>', 'Directory where to load Storybook configurations from') | ||
| .option('--skip-install', 'Skip installing deps') |
There was a problem hiding this comment.
style: Consider using the same flag format as line 51 (-s) for consistency
| let mainConfig; | ||
| let storybookVersion: string | undefined; | ||
| let mainConfig: StorybookConfigRaw | undefined; | ||
| let packageManager!: JsPackageManager; |
There was a problem hiding this comment.
logic: Using definite assignment assertion (!) here could mask initialization errors if getStorybookData fails
| migration: any, | ||
| { glob, dryRun, list, rename, parser, configDir: userSpecifiedConfigDir }: CLIOptions | ||
| ) { | ||
| export async function migrate(migration: any, { glob, dryRun, list, rename, parser }: CLIOptions) { |
There was a problem hiding this comment.
style: Migration parameter should be typed more specifically than 'any'
| export async function migrate(migration: any, { glob, dryRun, list, rename, parser }: CLIOptions) { | |
| export async function migrate(migration: string, { glob, dryRun, list, rename, parser }: CLIOptions) { |
| ) { | ||
| export async function migrate(migration: any, { glob, dryRun, list, rename, parser }: CLIOptions) { | ||
| if (list) { | ||
| listCodemods().forEach((key: any) => logger.log(key)); |
There was a problem hiding this comment.
style: Key parameter in forEach callback should be typed more specifically than 'any'
| listCodemods().forEach((key: any) => logger.log(key)); | |
| listCodemods().forEach((key: string) => logger.log(key)); |
| // Users struggle to upgrade Storybook with npm because of conflicting peer-dependencies | ||
| // GitHub Issue: https://github.com/storybookjs/storybook/issues/30306 | ||
| // Solution: Remove all Storybook packages (except 'storybook') from the package.json and install them again | ||
| // TODO: Check if this is still needed due to the resolution fix for the `storybook` package |
There was a problem hiding this comment.
style: Consider removing this TODO comment since the overrides fix is now implemented above
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 208 | 209 | 🚨 +1 🚨 |
| Self size | 606 KB | 607 KB | 🚨 +483 B 🚨 |
| Dependency size | 29.54 MB | 29.56 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 208 | 205 | 🎉 -3 🎉 |
| Self size | 28 KB | 28 KB | 🚨 +179 B 🚨 |
| Dependency size | 28.82 MB | 28.80 MB | 🎉 -21 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 534 | 531 | 🎉 -3 🎉 |
| Self size | 216 KB | 216 KB | 🚨 +10 B 🚨 |
| Dependency size | 58.69 MB | 58.67 MB | 🎉 -21 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 127 | 128 | 🚨 +1 🚨 |
| Self size | 2.39 MB | 2.39 MB | 0 B |
| Dependency size | 22.05 MB | 22.06 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 161 | 162 | 🚨 +1 🚨 |
| Self size | 35 KB | 35 KB | 0 B |
| Dependency size | 23.21 MB | 23.23 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 120 | 121 | 🚨 +1 🚨 |
| Self size | 32 KB | 32 KB | 🚨 +97 B 🚨 |
| Dependency size | 20.16 MB | 20.17 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 284 | 286 | 🚨 +2 🚨 |
| Self size | 25 KB | 25 KB | 0 B |
| Dependency size | 43.48 MB | 43.50 MB | 🚨 +15 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 175 | 177 | 🚨 +2 🚨 |
| Self size | 24 KB | 24 KB | 🚨 +171 B 🚨 |
| Dependency size | 30.32 MB | 30.33 MB | 🚨 +15 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
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: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 pull request has been released as version
0.0.0-pr-31553-sha-1239c0fa. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-31553-sha-1239c0fa sandboxor in an existing project withnpx storybook@0.0.0-pr-31553-sha-1239c0fa upgrade.More information
0.0.0-pr-31553-sha-1239c0favalentin/improve-monorepo-support-for-js-package-manager1239c0fa1748007184)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 canary-release-pr.yml --field pr=31553Greptile Summary
This PR significantly improves monorepo support in the JsPackageManager class and related helpers, focusing on package management operations and configuration handling across multiple package.json files.
primaryPackageJsonproperty to replaceretrievePackageJson(), providing better monorepo-aware package.json handlinggetAllDependencies()from async to sync for improved performance and simplified dependency lookupsgetModulePackageJSON()method withstopAtparameter to properly scope package searches in monorepos--skip-installflag to CLI commands for better control over package installations in monorepo setups