Skip to content
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

Core: Refactor phases to run in order loading -> rendering -> playing #28431

Merged
merged 6 commits into from
Jul 3, 2024

Conversation

kasperpeulen
Copy link
Contributor

@kasperpeulen kasperpeulen commented Jul 3, 2024

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:

  • 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/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>

Greptile Summary

  • Refactored phases in /code/core/src/preview-api/modules/preview-web/Preview.tsx to ensure order: loading -> rendering -> playing
  • Added composeBeforeAllHooks function in /code/core/src/preview-api/modules/store/csf/beforeAll.ts and corresponding tests in /code/core/src/preview-api/modules/store/csf/beforeAll.test.ts
  • Updated key generation in /code/addons/interactions/src/components/MethodCall.tsx for unique keys
  • Modified globby function call in /code/core/src/core-server/utils/StoryIndexGenerator.ts for absolute paths
  • Removed node-fetch imports across multiple files, simplifying HTTP request handling

@kasperpeulen kasperpeulen added maintenance User-facing maintenance tasks ci:normal labels Jul 3, 2024
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

  • Refactored StoryRender.ts to enforce phase order: loading -> rendering -> playing (/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts)
  • Updated tests in StoryRender.test.ts to align with new phase order logic (/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts)
  • Added beforeEach hook in tests to reset mocks (/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts)
  • Introduced mountSpy to replace inline mount functions in tests (/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts)
  • Added NoStoryMountedError class for improved error handling (/code/core/src/preview-errors.ts)

3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

nx-cloud bot commented Jul 3, 2024

☁️ Nx Cloud Report

CI 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 target

Sent with 💌 from NxCloud.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Added checkIfAborted in runPhase method for robust phase transitions (/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts)
  • Introduced NoStoryMountedError for handling unmounted stories (/code/core/src/preview-errors.ts)
  • Refactored tests to verify phase order and added mountSpy function (/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts)
  • Added beforeEach block 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 () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

@kasperpeulen Can we add a test for calling mount twice? Or did I miss it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@shilman shilman Jul 3, 2024

Choose a reason for hiding this comment

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

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.
}

Copy link
Member

Choose a reason for hiding this comment

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

I agree, let's add a test that we throw in that scenario! But I agree this change is an improvement.

Comment on lines 216 to 225
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;
},
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines +292 to +294
if (!mounted) {
throw new NoStoryMountedError();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yannbf this should solve your problem with a endless loaders, if you don't mount

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

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 renderToCanvas function call to align with new phase order (/code/core/src/preview-api/modules/preview-web/PreviewWithSelection.tsx)
  • Added showStoryDuringRender callback to mainStoryCallbacks method (/code/core/src/preview-api/modules/preview-web/PreviewWithSelection.tsx)
  • Introduced showStoryDuringRender callback 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 shilman changed the title Core: Refactor phases to always run in order loading -> rendering -> playing Core: Refactor phases to run in order loading -> rendering -> playing Jul 3, 2024
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

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 showStoryDuringRender callback 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 kasperpeulen added ci:daily Run the CI jobs that normally run in the daily job. and removed ci:normal labels Jul 3, 2024
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

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 showStoryDuringRender callback 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

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Minor request for testing. Otherwise LGTM. One high level question is how do we ensure this is not a breaking change?

await context.renderToCanvas();
return context.canvas;
});
it('does not call mount twice if mount called in play function', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

@kasperpeulen Can we add a test for calling mount twice? Or did I miss it?

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

LGTM

45 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@kasperpeulen kasperpeulen merged commit 3adada0 into next Jul 3, 2024
89 of 105 checks passed
@kasperpeulen kasperpeulen deleted the kasper/phases branch July 3, 2024 18:58
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

LGTM, we might want to add the extra test @shilman suggests.

await context.renderToCanvas();
return context.canvas;
});
it('does not call mount twice if mount called in play function', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

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 () => {
Copy link
Member

Choose a reason for hiding this comment

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

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:daily Run the CI jobs that normally run in the daily job. maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants