-
-
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
Test: Add mount property to the story context #28383
Conversation
…fore mounting in the play function
code/lib/preview-api/src/modules/preview-web/PreviewWithSelection.tsx
Outdated
Show resolved
Hide resolved
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 9678d6f. 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. |
code/lib/preview-api/src/modules/preview-web/render/mount-utils.ts
Outdated
Show resolved
Hide resolved
code/lib/preview-api/src/modules/store/csf/portable-stories.test.ts
Outdated
Show resolved
Hide resolved
# Conflicts: # code/frameworks/angular/src/client/decorators.test.ts
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.
In general I think the approach is good, but it's definitely missing tests, which I think would help understand what it is doing.
code/lib/preview-api/src/modules/preview-web/render/mount-utils.test.ts
Outdated
Show resolved
Hide resolved
code/lib/preview-api/src/modules/preview-web/render/StoryRender.ts
Outdated
Show resolved
Hide resolved
await this.runPhase(abortSignal, 'rendering', async () => { | ||
const teardown = await this.renderToScreen(renderContext, canvasElement); | ||
this.teardownRender = teardown || (() => {}); | ||
mounted = 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.
I don't understand why the runPhase
code is up here, it seems like the wrong place? Shouldn't just be down on line 245, in flow with the rest of it?
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.
The reason is that this is also called when calling mount
from inside the play function and line 245 is only called, when mount
is not used.
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.
Ok, I see. I guess this control flow from StoryRender
-> story.mount
(in prepareStory
) -> renderToCanvas
, or StoryRender
-> play
-> context.mount
-> renderToCanvas
is pretty hard to understand off the bat.
Is there something we can do to make it easier to follow (either in code or just comments?)
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.
Will refactor this bit in a new PR.
// Play is not supported in docs yet, and when mount is used, the mounting is happening in play itself. | ||
tags = tags.filter((tag) => tag !== 'autodocs'); |
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.
What does this do exactly?
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.
I tried to improve the message.
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.
// Don't show stories where mount is used in docs.
// As the play function is not running in docs, and when mount is used, the mounting is happening in play itself.
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.
Maybe:
// Hide stories that mount inside the `play` function from docs
// Docs doesn't currently run the `play` function, so such stories would never be mounted.
This does seem like the sort of thing that will confuse users though ("why has my story disappeared from docs?"). I wonder if there's some way we could make it more obvious for folks.
Also I wonder if we should alter the tag in the index rather than dynamically here. WDYT @shilman?
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.
Working on a new PR that fixes this
# Conflicts: # code/renderers/react/src/__test__/Button.stories.tsx
const teardown = await this.renderToScreen(renderContext, canvasElement); | ||
this.teardownRender = teardown || (() => {}); | ||
}); | ||
const isMountDestructured = playFunction && mountDestructured(playFunction); |
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.
Maybe we should store this property on the prepared story rather than checking in multiple places?
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.
I am working on a new PR that fixes this.
const mountUsed = mountDestructured(playFunction); | ||
|
||
if (!render && !mountUsed) { | ||
// TODO Make this a named 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.
reminder to accommodate this TODO in a followup PR or this one if you prefer
What I did
This PR implements most of the following RFC:
[RFC] Allow play to mount the component of the story
The idea is simply that you can do something after and before mounting in the play function:
storybook/code/lib/test/template/stories/mount-in-play.stories.ts
Lines 16 to 45 in 540083c
To make this a non-breaking change, we need to know if
mount
is used inside of the play function.We implemented for this, something similar as playwright/vitest fixtures. We analyze the "deps" of the play function by looking at what properties of the context are destructured. If you use
mount
without destructuring you get an error:storybook/code/lib/preview-api/src/modules/preview-web/render/StoryRender.ts
Lines 242 to 271 in 540083c
Portable stories
For portable stories, we introduce a
Story.play
which plays the whole story, including cleanup, loaders, beforeEach, render and play. It can be called like this:storybook/code/renderers/react/src/__test__/portable-stories.test.tsx
Lines 119 to 125 in 540083c
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/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>