Core: Refactor phases to run in order loading -> rendering -> playing
#28431
Conversation
![greptile-apps[bot]](https://github.com/avatars/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- Refactored
StoryRender.tsto enforce phase order: loading -> rendering -> playing (/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts) - Updated tests in
StoryRender.test.tsto align with new phase order logic (/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts) - Added
beforeEachhook in tests to reset mocks (/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts) - Introduced
mountSpyto replace inline mount functions in tests (/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts) - Added
NoStoryMountedErrorclass for improved error handling (/code/core/src/preview-errors.ts)
3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
☁️ Nx Cloud ReportCI is running/has finished running commands for commit cbade14. 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. |
kasperpeulen
force-pushed
the
kasper/phases
branch
from
9218c78
to
771b92b
Compare
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added
checkIfAbortedinrunPhasemethod for robust phase transitions (/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts) - Introduced
NoStoryMountedErrorfor handling unmounted stories (/code/core/src/preview-errors.ts) - Refactored tests to verify phase order and added
mountSpyfunction (/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts) - Added
beforeEachblock to reset mocks before each test (/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts)
3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
| await context.renderToCanvas(); | ||
| return context.canvas; | ||
| }); | ||
| it('does not call mount twice if mount called in play function', async () => { |
There was a problem hiding this comment.
@tmeasday I changed the assertion. As we prevent calling mount twice, if the user wants to mount in play.
It will always be called once. If not, we throw an error.
There was a problem hiding this comment.
@kasperpeulen Can we add a test for calling mount twice? Or did I miss it?
There was a problem hiding this comment.
We kind of auto-mount before the play function, except when you destructure mount.
So this test ensures that mount is only called once in that scenario and not twice.
There was a problem hiding this comment.
I see. What if I have a play function like this though?
play: async ({ mount }) => {
// some setup
await mount();
// who knows what
await mount();
// etc.
}
There was a problem hiding this comment.
I agree, let's add a test that we throw in that scenario! But I agree this change is an improvement.
| mount: async (...args) => { | ||
| let mountReturn: Awaited<ReturnType<StoryContext['mount']>> = null!; | ||
| await this.runPhase(abortSignal, 'rendering', async () => { | ||
| const teardown = await this.renderToScreen(renderContext, canvasElement); | ||
| this.teardownRender = teardown || (() => {}); | ||
| mounted = true; | ||
| mountReturn = await story.mount(context)(...args); | ||
| }); | ||
|
|
||
| if (isMountDestructured) { | ||
| // put the phase back to playing if mount is used inside a play function | ||
| await this.runPhase(abortSignal, 'playing', async () => {}); | ||
| } | ||
| // start playing phase if mount is used inside a play function | ||
| if (isMountDestructured) await this.runPhase(abortSignal, 'playing'); | ||
| return mountReturn; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Before we did the phase change in renderToCanvas.
But it actually needs to be done here. As renderToCanvas "fixture" might not be used inside of the mount "fixture". In theory, the mount fixture can do everything itself, although that is not how it is currently implemented in our own renderers.
| if (!mounted) { | ||
| throw new NoStoryMountedError(); | ||
| } |
There was a problem hiding this comment.
@yannbf this should solve your problem with a endless loaders, if you don't mount
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Refactored rendering phases to always run in order: loading -> rendering -> playing (
/code/core/src/preview-api/modules/preview-web/PreviewWithSelection.tsx) - Modified
renderToCanvasfunction call to align with new phase order (/code/core/src/preview-api/modules/preview-web/PreviewWithSelection.tsx) - Added
showStoryDuringRendercallback tomainStoryCallbacksmethod (/code/core/src/preview-api/modules/preview-web/PreviewWithSelection.tsx) - Introduced
showStoryDuringRendercallback for story display during rendering phase (/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts) - Improved error handling to remove loading screen on errors before rendering (
/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts)
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
shilman
changed the title
loading -> rendering -> playing
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Refactored phases to ensure order: loading -> rendering -> playing (
/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts) - Added checks for abort signal during phase transitions (
/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts) - Made
showStoryDuringRendercallback optional (/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts) - Improved consistency and reliability of story rendering process (
/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts)
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
kasperpeulen
added
ci:daily
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Refactored phases to ensure order: loading -> rendering -> playing (
/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts) - Added checks for abort signal during phase transitions (
/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts) - Made
showStoryDuringRendercallback optional (/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts) - Improved consistency and reliability of story rendering process (
/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts)
No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
| await context.renderToCanvas(); | ||
| return context.canvas; | ||
| }); | ||
| it('does not call mount twice if mount called in play function', async () => { |
There was a problem hiding this comment.
@kasperpeulen Can we add a test for calling mount twice? Or did I miss it?
There was a problem hiding this comment.
LGTM
45 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
| await context.renderToCanvas(); | ||
| return context.canvas; | ||
| }); | ||
| it('does not call mount twice if mount called in play function', async () => { |
There was a problem hiding this comment.
I agree, let's add a test that we throw in that scenario! But I agree this change is an improvement.
| @@ -207,16 +198,40 @@ describe('StoryRender', () => { | |||
| expect(view.showException).toHaveBeenCalled(); | |||
| }); | |||
|
|
|||
| it('errors if play function destructures mount but does not call it', async () => { | |||
Closes #
What I did
Refactor phases to always run in order loading -> rendering -> playing
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/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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam 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>Greptile Summary
/code/core/src/preview-api/modules/preview-web/Preview.tsxto ensure order: loading -> rendering -> playingcomposeBeforeAllHooksfunction in/code/core/src/preview-api/modules/store/csf/beforeAll.tsand corresponding tests in/code/core/src/preview-api/modules/store/csf/beforeAll.test.ts/code/addons/interactions/src/components/MethodCall.tsxfor unique keysglobbyfunction call in/code/core/src/core-server/utils/StoryIndexGenerator.tsfor absolute pathsnode-fetchimports across multiple files, simplifying HTTP request handling