Skip to content

Core: Implement afterEach - #29584

Closed
kasperpeulen wants to merge 7 commits into
nextfrom
kasper/after-each
Closed

Core: Implement afterEach#29584
kasperpeulen wants to merge 7 commits into
nextfrom
kasper/after-each

Conversation

@kasperpeulen

@kasperpeulen kasperpeulen commented Nov 11, 2024

Copy link
Copy Markdown
Member

Closes #

What I did

Working on top of this csf canary:
ComponentDriven/csf#109

To implement this RFC:
#29583

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/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>

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 78.2 MB 78.2 MB 0 B 1.13 0%
initSize 143 MB 143 MB 2.15 kB 1.38 0%
diffSize 65.2 MB 65.2 MB 2.15 kB 0.91 0%
buildSize 6.88 MB 6.88 MB 1.32 kB 28.17 0%
buildSbAddonsSize 1.51 MB 1.51 MB 820 B 123.53 0.1%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.9 MB 1.9 MB -100 B -0.65 0%
buildSbPreviewSize 271 kB 271 kB 596 B Infinity 0.2%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.88 MB 3.88 MB 1.32 kB 28.17 0%
buildPreviewSize 3 MB 3 MB 0 B - 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 22.9s 25.8s 2.8s 1.69 🔺11%
generateTime 23s 21.9s -1s -72ms -0.11 -4.9%
initTime 15.5s 15s -452ms -0.19 -3%
buildTime 8.3s 9.1s 811ms 0.21 8.9%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 5.4s 5.6s 234ms -0.4 4.1%
devManagerResponsive 3.4s 3.6s 201ms -0.18 5.6%
devManagerHeaderVisible 519ms 589ms 70ms 0.29 11.9%
devManagerIndexVisible 594ms 621ms 27ms -0.15 4.3%
devStoryVisibleUncached 862ms 1.1s 322ms 0.41 27.2%
devStoryVisible 592ms 620ms 28ms 0.01 4.5%
devAutodocsVisible 507ms 534ms 27ms 0.08 5.1%
devMDXVisible 520ms 498ms -22ms -0.38 -4.4%
buildManagerHeaderVisible 490ms 491ms 1ms -1.19 0.2%
buildManagerIndexVisible 502ms 507ms 5ms -1.15 1%
buildStoryVisible 492ms 486ms -6ms -1.23 -1.2%
buildAutodocsVisible 467ms 419ms -48ms -0.85 -11.5%
buildMDXVisible 406ms 443ms 37ms -0.25 8.4%

Greptile Summary

Here's my summary of the PR implementing afterEach functionality in Storybook:

Adds afterEach lifecycle hook to Storybook's story execution pipeline, allowing cleanup operations to be performed after each story runs, with proper phase management and error handling.

  • Added applyAfterEach to PreparedStory type and implementation to execute finalizers in reverse order (story -> component -> project)
  • Added 'afterEach' phase to RenderPhase and updated StoryRender to run afterEach hooks before transitioning to 'played' or 'errored' states
  • Added afterEach field to composeConfigs return object using existing getArrayField helper
  • Added story.applyAfterEach(context) call in runStory function with proper abort signal handling
  • Updated CSF dependency to canary version 0.1.12--canary.109.4d1d54a.0 across multiple packages

@kasperpeulen
kasperpeulen marked this pull request as draft November 11, 2024 13:58
@kasperpeulen kasperpeulen added ci:normal Run our default set of CI jobs (choose this for most PRs). feature request labels Nov 11, 2024
@nx-cloud

nx-cloud Bot commented Nov 11, 2024

Copy link
Copy Markdown

☁️ Nx Cloud Report

CI is running/has finished running commands for commit e038e8b. 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 target

Sent with 💌 from NxCloud.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

14 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile

Comment thread code/addons/links/package.json Outdated
},
"dependencies": {
"@storybook/csf": "^0.1.11",
"@storybook/csf": "0.1.12--canary.109.4d1d54a.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Pinning to a canary version (0.1.12--canary.109.4d1d54a.0) instead of using ^0.1.11 could cause version conflicts. Consider if this specific version is required or if a caret range would be more appropriate.

Comment thread code/addons/test/package.json Outdated
},
"dependencies": {
"@storybook/csf": "^0.1.11",
"@storybook/csf": "0.1.12--canary.109.4d1d54a.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Using a canary version (0.1.12--canary) in production dependencies could introduce instability. Consider waiting for a stable release.

Comment on lines +411 to +412

await story.applyAfterEach(context);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: afterEach should run even if playFunction throws an error. Consider wrapping the play function in try/finally to ensure afterEach always executes.

Comment on lines +102 to +104
if (context.abortSignal.aborted) {
return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider handling errors from finalizers to ensure all cleanup runs even if one fails

Comment thread code/lib/blocks/package.json Outdated
},
"dependencies": {
"@storybook/csf": "^0.1.11",
"@storybook/csf": "0.1.12--canary.109.4d1d54a.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Pinning to a canary version in production dependencies could cause issues if the canary version has bugs or breaking changes. Consider waiting for a stable release.

Comment thread code/lib/codemod/package.json Outdated
"@babel/types": "^7.24.0",
"@storybook/core": "workspace:*",
"@storybook/csf": "^0.1.11",
"@storybook/csf": "0.1.12--canary.109.4d1d54a.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Using a canary version in production dependencies could cause instability. Consider waiting for a stable release.

Comment thread code/lib/source-loader/package.json Outdated
},
"dependencies": {
"@storybook/csf": "^0.1.11",
"@storybook/csf": "0.1.12--canary.109.4d1d54a.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Using a canary version in production dependencies can be unstable. Consider waiting for a stable release or document the rationale for using this specific pre-release version.

Comment thread code/lib/test/package.json Outdated
},
"dependencies": {
"@storybook/csf": "^0.1.11",
"@storybook/csf": "0.1.12--canary.109.4d1d54a.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Using a specific canary version instead of a caret range could cause issues if other packages depend on different versions of @storybook/csf

Comment thread code/package.json Outdated
"@storybook/core": "workspace:*",
"@storybook/core-webpack": "workspace:*",
"@storybook/csf": "0.1.11",
"@storybook/csf": "0.1.12--canary.109.4d1d54a.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Using a canary version (0.1.12--canary.109.4d1d54a.0) in production dependencies could lead to instability. Consider using a stable version instead.

Comment thread code/renderers/server/package.json Outdated
"dependencies": {
"@storybook/components": "workspace:*",
"@storybook/csf": "^0.1.11",
"@storybook/csf": "0.1.12--canary.109.4d1d54a.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: using a canary version in production dependencies could cause instability - consider waiting for a stable release

@kasperpeulen kasperpeulen changed the title Implement afterEach Core: Implement afterEach Nov 12, 2024
Comment on lines +320 to +323
await this.runPhase(abortSignal, 'afterEach', async () => {
await applyAfterEach(context);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would assume that this phase runs after the played event in line 327

@valentinpalkovic

Copy link
Copy Markdown
Contributor

Merged into valentin/unified-a11y-testing

@kasperpeulen
kasperpeulen deleted the kasper/after-each branch April 4, 2026 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:normal Run our default set of CI jobs (choose this for most PRs). feature request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants