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

Broken story render in Storybook V7 #105

Closed
vtx-anton-chashchin opened this issue Jan 9, 2024 · 8 comments · Fixed by #107
Closed

Broken story render in Storybook V7 #105

vtx-anton-chashchin opened this issue Jan 9, 2024 · 8 comments · Fixed by #107

Comments

@vtx-anton-chashchin
Copy link

vtx-anton-chashchin commented Jan 9, 2024

When Happo configured to use prebuild package of the storybook it doesnt wait for story rendered event and runs all hooks like beforeScreenshot before the actual DOM nodes will be exist. It seems like the problem in detecting should wait for render story or not

const shouldWaitForCompletedEvent = (channel.events || {})

As I guess previous versions of Channel API in Storybook contained all possible events as empty arrays and that detection worked fine, but from some release of storybook it started to have undefined until some listener be registred, it seems like this detection never worked since some release of storybook (I dont know which exact) and that's why Happo didn't wait for the actual story render

Easy way to prove it:

  • Download and run Happo build
npx http-server package-123
  • Register any storyRenderPhaseChanged message listener on the channel and immediately unregister it
const channel = window.__STORYBOOK_ADDONS_CHANNEL__;
const callback = (...args) => console.log(...args);
channel.on('storyRenderPhaseChanged', callback);
channel.off('storyRenderPhaseChanged', callback);
  • Run Happo next example and see that now it logs story render phases and waits for story be rendered
await window.happo.nextExample()
@trotzig
Copy link
Contributor

trotzig commented Jan 10, 2024

Hi @vtx-anton-chashchin -- thanks for the detailed write-up! I'll have a look at this together with #104.

@trotzig
Copy link
Contributor

trotzig commented Jan 11, 2024

I'm having some issues reproducing this. From your description, it seems something has to register a listener for an event for it to appear in channel.events? I'm doing the following to attempt to reproduce:

  • Make a production build of a storybook: NODE_ENV=production yarn build-local-storybook
  • Start a server in the output foulder: npx http-server -c-1 .happo-out
  • Open the iframe.html page in Chrome: http://localhost:8080/iframe.html
  • Open Chrome dev tools, call window.happo.nextExample()
  • See that all render phase events are logged:
window.happo.nextExample()
Promise {<pending>}
main.0280c89a.iframe.bundle.js:1 {newPhase: 'preparing', storyId: 'stories--themed'}
main.0280c89a.iframe.bundle.js:1 {newPhase: 'loading', storyId: 'stories--themed'}
main.0280c89a.iframe.bundle.js:1 {newPhase: 'rendering', storyId: 'stories--themed'}
main.0280c89a.iframe.bundle.js:1 {newPhase: 'completed', storyId: 'stories--themed'}

If I understood you correctly, there would be no render phase related logs here.

Can you help me reproduce? Maybe you can email me ([email protected]) a link to a Happo report where this isn't working? I can debug using the assets for that report.

@vtx-anton-chashchin
Copy link
Author

@trotzig I sent you a link to diff report, please be noticed that build which called before contains the default storybook build and build after contains storybook vite build (this detail doesn't affect this bug)
Both of the packages used storybook 7.6.6
In build which called before the bug is reproducible, and in build which called after it is fixed by my post install script using const shouldWaitForCompletedEvent = true;

So the steps to reproduce the bug:

  • Download build before
  • Run npx http-server -c-1 package-before
  • Run window.happo.nextExample()
  • See no logs related to render phases
  • Register any storyRenderPhaseChanged message listener on the channel and immediately unregister it
const channel = window.__STORYBOOK_ADDONS_CHANNEL__;
const callback = (...args) => console.log(...args);
channel.on('storyRenderPhaseChanged', callback);
channel.off('storyRenderPhaseChanged', callback);
  • Run window.happo.nextExample() again and see the render phases logs, which means happo waits for the story render

Check the build after

  • Download build after
  • Run npx http-server -c-1 package-after
  • Run window.happo.nextExample() and see the render phases logs without any additional hacks

@trotzig
Copy link
Contributor

trotzig commented Jan 11, 2024

Thanks for the link and the context @vtx-anton-chashchin! I'm starting to understand how relying on the storyRenderPhaseChanged array to be present is a bad idea. It works in most cases because Storybook itself will register a listener for the event. But as you have demonstrated, there's no guarantee for that. I think when I was writing the code for this, I thought the events object contained all the supported events. But that's not the case -- it contains the events that are or have been listened to. I can even add arbitrary events (like foobar and it will show up in the events object).

I'm currently trying to figure out a different way of doing the feature detection to see if the current Storybook supports the storyRenderPhaseChanged event or not. The event was introdued in v6.4.0 (storybookjs/storybook@2fd1d50#diff-ac7ad14f70e16dfc2c76befa9869b5440ef955b2d97570cd547f50c257e22da3).

I should have a fix soon.

@vtx-anton-chashchin
Copy link
Author

@trotzig glad to read that it is clarified how)
I have some thought about this, so maybe you would find it helpful
Maybe better not to use kinda "universal" package to handle all possible versions of storybook, but better align versions of this plugin with storybook versions, for example [email protected] and higher versions will just use mentioned feature without any additional checks, but the lower versions will not do it at all, I see 2 benefits of this approach

  • it follows the storybook versioning approach, so users could easily check is this version compatible with their version of storybook or not
  • you could simplify and easily maintain multiply versions of plugin without breaking changes/compromises/complex detections of supported features

trotzig added a commit that referenced this issue Jan 11, 2024
And allow people to disable the behavior by calling
`setShouldWaitForCompletedEvent(false)`.

At first I was trying to find a feature-detection method of knowing when
this event was present or not. But it turned out that was quite hard. So
I went the simpler route by defaulting the variable to true and then
allowing people to override it in case they are on Storybook earlier
than v6.4.0.

The issue with the previous feature detection approach was that we
relied on a listener for the event to be set up elsewhere. If no
listener were in place for storyRenderPhaseChanged, we wouldn't use it.
In most cases, the listener is there, but we've just come across
a Storybook where that's not the case.

Fixes #105
@trotzig
Copy link
Contributor

trotzig commented Jan 11, 2024

I agree that it could be a better approach. I struggled with getting the tests to pass for Storybook v6 as well when preparing the previous bugfix (#106).

At this point however, I'm going to push a new release through and see if that fixes your issue. If so, I'll likely move on to something different. If not, I might take you up on the change in approach. Appreciate the feedback! 🙏

@trotzig
Copy link
Contributor

trotzig commented Jan 11, 2024

Released in v4.0.0 -- give it a try and let me know how it goes!
https://github.com/happo/happo-plugin-storybook/releases/tag/v4.0.0

@vtx-anton-chashchin
Copy link
Author

@trotzig It works fine, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants