-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
Release: Prerelease 9.1.0-alpha.6 #31737
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
Conversation
CLI: Add prompts abstraction
…r' into yann/detect-multiple-config-dirs
…support-for-js-package-manager Core: Improve mono repo handling for JsPackageManager class and helpers
…into yann/detect-multiple-config-dirs
…ents CLI: Improve support for upgrading Storybook in monorepos
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.
196 files reviewed, 21 comments
Edit PR Review Bot Settings | Greptile
export const LOCK_FILES = [ | ||
NPM_LOCKFILE, | ||
PNPM_LOCKFILE, | ||
YARN_LOCKFILE, | ||
BUN_LOCKFILE, | ||
BUN_LOCKFILE_BINARY, | ||
]; |
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 using as const assertion for LOCK_FILES array to make it readonly and more type-safe
export const LOCK_FILES = [ | |
NPM_LOCKFILE, | |
PNPM_LOCKFILE, | |
YARN_LOCKFILE, | |
BUN_LOCKFILE, | |
BUN_LOCKFILE_BINARY, | |
]; | |
export const LOCK_FILES = [ | |
NPM_LOCKFILE, | |
PNPM_LOCKFILE, | |
YARN_LOCKFILE, | |
BUN_LOCKFILE, | |
BUN_LOCKFILE_BINARY, | |
] as const; |
|
||
await blocker.check({ |
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: missing assertion - result of blocker.check() is not being verified
await blocker.check({ | |
const result = await blocker.check({ | |
packageJson: { | |
dependencies: {}, | |
devDependencies: { | |
'@storybook/experimental-addon-test': '^1.0.0', | |
}, | |
}, | |
packageManager: mockPackageManager as any, | |
} as any); | |
expect(result).toBe(false); |
// @ts-expect-error Ignore abstract class error | ||
jsPackageManager = new JsPackageManager(); |
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 creating a proper test implementation class instead of using @ts-expect-error to instantiate abstract class
switch (data.packageName) { | ||
case '@storybook/preact-webpack5': | ||
case '@storybook/preset-preact-webpack': | ||
title = 'Preact Webpack5 support removed'; | ||
break; | ||
case '@storybook/vue3-webpack5': | ||
case '@storybook/preset-vue3-webpack': | ||
title = 'Vue3 Webpack5 support removed'; | ||
break; | ||
case '@storybook/html-webpack5': | ||
case '@storybook/preset-html-webpack': | ||
title = 'HTML Webpack5 support removed'; | ||
break; | ||
case '@storybook/web-components-webpack5': | ||
title = 'Web Components Webpack5 support removed'; | ||
break; | ||
case '@storybook/svelte-webpack5': | ||
title = 'Svelte Webpack5 support removed'; | ||
break; | ||
} |
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 using an object map for title lookups instead of switch statement for better maintainability
// is either not selected by the user (for a particular proejct) | ||
// therefore we need to check for configDir as well as fix id matches |
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: Typo in comment 'proejct' -> 'project'
// is either not selected by the user (for a particular proejct) | |
// therefore we need to check for configDir as well as fix id matches | |
// is either not selected by the user (for a particular project) | |
// therefore we need to check for configDir as well as fix id matches |
return { | ||
title: 'Downgrade Not Supported', | ||
message: dedent` | ||
Your Storybook version (v${data.currentVersion}) is newer than the target release (v${versions.storybook}).Downgrading is not supported. |
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: Missing space after period in error message
Your Storybook version (v${data.currentVersion}) is newer than the target release (v${versions.storybook}).Downgrading is not supported. | |
Your Storybook version (v${data.currentVersion}) is newer than the target release (v${versions.storybook}). Downgrading is not supported. |
- Vitest configuration is in `code/vitest.workspace.ts` | ||
- Test files typically follow the pattern `*.test.ts`, `*.test.tsx`, `*.spec.ts`, or `*.spec.tsx` |
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: Test pattern list is incomplete. Should also include .stories.test.ts
which is commonly used for story testing in Storybook.
"wallaby": { | ||
"command": "node", | ||
"args": [ | ||
"~/.wallaby/mcp/" |
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: Using '~/' for home directory path may cause issues on Windows. Consider using platform-agnostic path resolution
storybookProjects.some((project) => { | ||
if (!project.isCanary && lt(project.currentCLIVersion, project.beforeVersion)) { | ||
throw new UpgradeStorybookToLowerVersionError({ |
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: Use every
instead of some
to ensure all projects are checked. Current code stops checking after first valid project
storybookProjects.some((project) => { | |
if (!project.isCanary && lt(project.currentCLIVersion, project.beforeVersion)) { | |
throw new UpgradeStorybookToLowerVersionError({ | |
storybookProjects.forEach((project) => { | |
if (!project.isCanary && lt(project.currentCLIVersion, project.beforeVersion)) { | |
throw new UpgradeStorybookToLowerVersionError({ |
const files = await searchFiles({ | ||
searchQuery, | ||
cwd: projectRoot, | ||
cwd: getProjectRoot(), | ||
}); |
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 caching getProjectRoot() result to avoid multiple calls in the map function
View your CI Pipeline Execution ↗ for commit 2506beb.
☁️ Nx Cloud last updated this comment at |
Core: Delete shim addon packages
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
Before | After | Difference | |
---|---|---|---|
Dependency count | 6 | 6 | 0 |
Self size | 630 KB | 553 KB | 🎉 -77 KB 🎉 |
Dependency size | 1.49 MB | 1.49 MB | 0 B |
Bundle Size Analyzer | Link | Link |
storybook
Before | After | Difference | |
---|---|---|---|
Dependency count | 49 | 49 | 0 |
Self size | 31.22 MB | 31.88 MB | 🚨 +662 KB 🚨 |
Dependency size | 17.41 MB | 17.41 MB | 0 B |
Bundle Size Analyzer | Link | Link |
@storybook/angular
Before | After | Difference | |
---|---|---|---|
Dependency count | 208 | 209 | 🚨 +1 🚨 |
Self size | 606 KB | 607 KB | 🚨 +549 B 🚨 |
Dependency size | 29.78 MB | 29.79 MB | 🚨 +15 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.94 MB | 28.93 MB | 🎉 -18 KB 🎉 |
Bundle Size Analyzer | Link | Link |
@storybook/nextjs
Before | After | Difference | |
---|---|---|---|
Dependency count | 534 | 531 | 🎉 -3 🎉 |
Self size | 217 KB | 217 KB | 🚨 +10 B 🚨 |
Dependency size | 58.94 MB | 58.92 MB | 🎉 -18 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.12 MB | 22.14 MB | 🚨 +15 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 | 🚨 +129 B 🚨 |
Dependency size | 23.29 MB | 23.30 MB | 🚨 +15 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.23 MB | 20.24 MB | 🚨 +15 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.66 MB | 43.68 MB | 🚨 +19 KB 🚨 |
Bundle Size Analyzer | Link | Link |
sb
Before | After | Difference | |
---|---|---|---|
Dependency count | 50 | 50 | 0 |
Self size | 1 KB | 1 KB | 0 B |
Dependency size | 48.63 MB | 49.29 MB | 🚨 +662 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/cli
Before | After | Difference | |
---|---|---|---|
Dependency count | 324 | 324 | 0 |
Self size | 244 KB | 235 KB | 🎉 -9 KB 🎉 |
Dependency size | 98.82 MB | 99.89 MB | 🚨 +1.07 MB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/codemod
Before | After | Difference | |
---|---|---|---|
Dependency count | 267 | 267 | 0 |
Self size | 31 KB | 31 KB | 🚨 +209 B 🚨 |
Dependency size | 82.11 MB | 82.77 MB | 🚨 +666 KB 🚨 |
Bundle Size Analyzer | Link | Link |
create-storybook
Before | After | Difference | |
---|---|---|---|
Dependency count | 1 | 1 | 0 |
Self size | 12.13 MB | 12.53 MB | 🚨 +402 KB 🚨 |
Dependency size | 98 KB | 98 KB | 0 B |
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.49 MB | 30.51 MB | 🚨 +19 KB 🚨 |
Bundle Size Analyzer | Link | Link |
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.
196 files reviewed, 28 comments
Edit PR Review Bot Settings | Greptile
appDir: `${dir}/apps`, | ||
rootDir: projectRoot, | ||
pagesDir: `${projectRoot}/pages`, | ||
appDir: `${projectRoot}/apps`, |
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: Path is hardcoded to 'apps' which may not match Next.js app directory convention of 'app'. Consider using nextConfig.appDir or defaulting to 'app'
newLogger.error( | ||
msg.replace(message.toString(), colors.red(message.toString())).replaceAll(process.cwd(), '.') | ||
); |
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: Using replaceAll on process.cwd() could cause issues if the path contains regex special characters
|
||
export interface ProjectDoctorResults { | ||
configDir: string; | ||
status: 'healthy' | 'has_issues' | 'check_error'; |
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: Use enum for status values instead of string literal type for consistency with other status handling
let removeAddon = removeAddonBase; | ||
const packageManager = JsPackageManagerFactory.getPackageManager({ | ||
configDir: options.configDir, | ||
}); |
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: Package manager initialization should be inside the if block to avoid unnecessary instantiation when telemetry is disabled
|
||
fireEvent.submit(dialog.getElementsByTagName('form')[0]); | ||
await fireEvent.submit(dialog.getElementsByTagName('form')[0]); |
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 using queryByRole('form') instead of getElementsByTagName for consistency with other queries
if (ignoreError) { | ||
execaProcess.catch((err) => { | ||
// Silently ignore errors when ignoreError is true | ||
}); | ||
} |
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: Error suppression in catch block could hide important errors - at minimum log to debug level
vi.spyOn(JsPackageManager, 'getPackageJson').mockImplementation((args) => { | ||
return { | ||
dependencies: {}, | ||
devDependencies: { | ||
'@storybook/manager-webpack5': 'x.x.x', | ||
'@storybook/react': 'x.x.x', | ||
}, | ||
}; | ||
}); |
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: args parameter in mockImplementation is unused - remove from function signature
vi.spyOn(JsPackageManager, 'getPackageJson').mockImplementation((args) => { | |
return { | |
dependencies: {}, | |
devDependencies: { | |
'@storybook/manager-webpack5': 'x.x.x', | |
'@storybook/react': 'x.x.x', | |
}, | |
}; | |
}); | |
vi.spyOn(JsPackageManager, 'getPackageJson').mockImplementation(() => { | |
return { | |
dependencies: {}, | |
devDependencies: { | |
'@storybook/manager-webpack5': 'x.x.x', | |
'@storybook/react': 'x.x.x', | |
}, | |
}; | |
}); |
if ( | ||
beforeVersion === null || | ||
afterVersion === null || | ||
allDependencies[packageName] === null |
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: allDependencies[packageName] === null check is redundant since getAllDependencies() returns Record<string, string>
import { promises as fs } from 'node:fs'; | ||
import path, { join } from 'node:path'; | ||
|
||
import { cleanLog } from '../../../../lib/cli-storybook/src/automigrate/helpers/cleanLog'; |
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: Importing cleanLog from a relative path that traverses up multiple directories can be fragile. Consider moving cleanLog to a shared utilities package or updating the import structure
@@ -1,9 +1,11 @@ | |||
import { dirname, join } from 'node:path'; | |||
|
|||
import { sync as findUpSync } from 'find-up'; | |||
import { getProjectRoot } from 'storybook/internal/common'; |
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: Internal imports should come after external dependencies
…ters CSF: Improve docs parameter types
Docs: Update eslint plugin configuration guide
CLI: Fix package manager instantiation in empty directories
56340df
to
2506beb
Compare
This is an automated pull request that bumps the version from
9.1.0-alpha.5
to9.1.0-alpha.6
.Once this pull request is merged, it will trigger a new release of version
9.1.0-alpha.6
.If you're not a core maintainer with permissions to release you can ignore this pull request.
To do
Before merging the PR, there are a few QA steps to go through:
And for each change below:
This is a list of all the PRs merged and commits pushed directly to
next
, that will be part of this release:If you've made any changes doing the above QA (change PR titles, revert PRs), manually trigger a re-generation of this PR with this workflow and wait for it to finish. It will wipe your progress in this to do, which is expected.
Feel free to manually commit any changes necessary to this branch after you've done the last re-generation, following the Make Manual Changes section in the docs, especially if you're making changes to the changelog.
When everything above is done:
Generated changelog
9.1.0-alpha.6