-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Release: Prerelease 8.5.0-alpha.14 #29752
Conversation
…torybook into jeppe/vitest-coverage-backend
…torybook into jeppe/vitest-coverage-backend
…ybook into jeppe/vitest-coverage-backend
…test-coverage-backend
…test-coverage-backend
…th config.coverage
Build: Fix portable stories tests by moving from jsdom to happy-dom
…s/storybook into jeppe/vitest-coverage-backend
Test: Add coverage feature
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.
21 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
watching: true, | ||
config: { | ||
a11y: true, | ||
coverage: true, // should be automatically disabled in the UI |
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: Coverage should be disabled in watch mode, but this comment suggests it's handled in the UI rather than enforced in the state. Consider enforcing this at the state level to prevent inconsistencies.
// @ts-expect-error ListItem doesn't include all anchor attributes in types, but it is an achor element | ||
target="_blank" |
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: The ts-expect-error comment suggests ListItem types need updating to include anchor attributes. Consider adding these types to prevent needing this override.
const coverageDetails: Details['coverageSummary'] = { | ||
percentage, | ||
status: | ||
percentage < lowWatermark | ||
? 'negative' | ||
: percentage < highWatermark | ||
? 'warning' | ||
: 'positive', | ||
}; |
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: check that these status thresholds align with Storybook's standard status indicators elsewhere in the UI
async restartVitest({ watchMode, coverage }: { watchMode: boolean; coverage: boolean }) { | ||
await this.vitestManager.vitest?.runningPromise; | ||
await this.vitestManager.closeVitest(); | ||
await this.vitestManager.startVitest(watchMode); | ||
await this.vitestManager.startVitest({ watchMode, coverage }); | ||
} |
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 adding error handling here in case any of the async operations fail during restart
'@storybook/experimental-addon-test/internal/coverage-reporter', | ||
{ | ||
testManager: this.testManager, | ||
coverageOptions: this.vitest?.config?.coverage as ResolvedCoverageOptions<'v8'>, |
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: potential race condition accessing this.vitest?.config before initialization
export const staticDirs: PresetPropertyFn<'staticDirs'> = async (values = [], options) => { | ||
if (options.configType === 'PRODUCTION') { | ||
return values; | ||
} |
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: Coverage directory should be available in production builds to view historical coverage reports
export type TestingModuleRunRequestPayload< | ||
Config extends { [key: string]: any } = NonNullable<unknown>, | ||
> = { | ||
providerId: TestProviderId; | ||
// TODO: Avoid needing to do a fetch request server-side to retrieve the index | ||
indexUrl: string; // e.g. http://localhost:6006/index.json | ||
storyIds?: string[]; // ['button--primary', 'button--secondary'] | ||
config?: TestProviderState['config']; | ||
config?: Config; | ||
}; |
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: The generic Config type parameter should be constrained to require at least the base config properties needed by TestProviderState. Consider adding a base interface that Config must extend.
db43ee4
to
742ddd2
Compare
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 742ddd2. 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 2 targetsSent with 💌 from NxCloud. |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
Before | After | Difference | |
---|---|---|---|
Dependency count | 7 | 7 | 0 |
Self size | 1.89 MB | 1.89 MB | 0 B |
Dependency size | 10.25 MB | 10.52 MB | 🚨 263 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/experimental-addon-test
Before | After | Difference | |
---|---|---|---|
Dependency count | 61 | 61 | 0 |
Self size | 616 KB | 695 KB | 🚨 79 KB 🚨 |
Dependency size | 13.86 MB | 13.86 MB | 0 B |
Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
Before | After | Difference | |
---|---|---|---|
Dependency count | 93 | 102 | 🚨 9 🚨 |
Self size | 41 KB | 41 KB | 🚨 459 B 🚨 |
Dependency size | 16.16 MB | 17.87 MB | 🚨 1.71 MB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/cli
Before | After | Difference | |
---|---|---|---|
Dependency count | 390 | 390 | 0 |
Self size | 484 KB | 484 KB | 0 B |
Dependency size | 74.63 MB | 74.89 MB | 🚨 266 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/codemod
Before | After | Difference | |
---|---|---|---|
Dependency count | 270 | 270 | 0 |
Self size | 612 KB | 612 KB | 0 B |
Dependency size | 64.62 MB | 64.89 MB | 🚨 266 KB 🚨 |
Bundle Size Analyzer | Link | Link |
@storybook/source-loader
Before | After | Difference | |
---|---|---|---|
Dependency count | 5 | 5 | 0 |
Self size | 41 KB | 41 KB | 0 B |
Dependency size | 10.20 MB | 10.46 MB | 🚨 263 KB 🚨 |
Bundle Size Analyzer | Link | Link |
This is an automated pull request that bumps the version from
8.5.0-alpha.13
to8.5.0-alpha.14
.Once this pull request is merged, it will trigger a new release of version
8.5.0-alpha.14
.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
8.5.0-alpha.14