search

storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
84.08k stars 9.25k forks source link

Core: Abort play function on HMR or when args/globals change #16431

Open yannbf opened 2 years ago

yannbf commented 2 years ago

Describe the bug

If you are stepping through a story's interaction in the panel, and while you're pause you interact with controls or globals (such as a toolbar item), the story will fail in the next interaction.

Here's a commented video to show the steps:

https://user-images.githubusercontent.com/1671563/138229662-b27a1d18-6f5b-466d-9515-9e4d0769720d.mov

To Reproduce

┆Issue is synchronized with this Asana task by Unito

yannbf commented 2 years ago

I think this will generate an interesting discussion about running phases. @ghengeveld @tmeasday

tmeasday commented 2 years ago

@yannbf and I discussed this already and I think we have an important decision to make here.

So far we've said that if a story re-renders without remounting (i.e. args/globals changes), we basically "ignore" the play function. So:

  1. If the play function hasn't started yet from a previous render, we allow it to run, assuming it can cope with the updated story[1]
  2. If the play function has already completed from a previous render [this is the typical case], we don't run it again, but assume that the story will re-render "underneath" the changes the play function made (e.g. if an input is filled out, the play function may change the size of the input but the "filled out-ness" will probably remain and that's good).
  3. If the play function is mid-way through executing, we let it continue, even though we are re-rendering underneath it.
  4. If the play function is paused, we leave it, and allow it to continue later, even though we are re-rendering underneath it.

3&4 are probably similar with respect to this problem, although 4 is far more likely and easier to reproduce. The issue is that user code could easily retain references to elements that are no longer around after the args have changed. Then the play function is going to throw weird errors when it is restarted.

I think there is a strong argument in favour of "correctness" here that says we should abort the play function if running/paused when you re-render. The user can always hit the "replay" function if they want to run it again from scratch.

[1] This one seems fine, there's probably nothing else we can do that makes sense.

shilman commented 2 years ago

@tmeasday I agree that we should side with correctness in all testing/debugging things.

ghengeveld commented 2 years ago

Discussed this with @tmeasday, we came up with the following approach:

  1. On args/globals change (i.e. rerender) while playing: continue playing (don't rerun play function). In case this blows up, show the error on the Interactions tab rather than in a redbox.
  2. On args/globals change (i.e. rerender) while debugging: remount and play to step on next (FORCE_REMOUNT with isDebugging: true)
  3. On HMR while playing: remount and rerun play from start (FORCE_REMOUNT event)
  4. On HMR while debugging: remount and play to step

@tmeasday will deal with HMR. I'll deal with args/globals changes. Parts 2 and 4 we'll handle when we deal with other debugger issues for 6.4 stabilization.

yannbf commented 2 years ago

Discussed this with @tmeasday, we came up with the following approach:

  1. On args/globals change (i.e. rerender) while playing: continue playing (don't rerun play function). In case this blows up, show the error on the Interactions tab rather than in a redbox.
  2. On args/globals change (i.e. rerender) while debugging: remount and play to step on next (FORCE_REMOUNT with isDebugging: true)
  3. On HMR while playing: remount and rerun play from start (FORCE_REMOUNT event)
  4. On HMR while debugging: remount and play to step

@tmeasday will deal with HMR. I'll deal with args/globals changes. Parts 2 and 4 we'll handle when we deal with other debugger issues for 6.4 stabilization.

Sounds logical and perfect! 🚀

ndelangen commented 2 years ago

I'm also 💯 onboard with this approach!

shilman commented 2 years ago

In case 1 (args change during play fn) we should probably focus the interactions tab to make sure people know what went wrong when there's an error

tmeasday commented 2 years ago

@ghengeveld just to be clear I can confirm that the full rendering stack reruns when HMR occurs -- c.f. these tests:

https://github.com/storybooks/storybook/blob/5df3e142d7373fd6987042df28c221cfbcdd2fa2/lib/preview-web/src/PreviewWeb.test.ts#L2016-L2154

shilman commented 2 years ago

Yippee!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.4.0-beta.27 containing PR #16574 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.

tmeasday commented 2 years ago

@ghengeveld should we re-open this, given the changes so far only affect scenarios 1&2 above? Or create a new 6.5 ticket for the remaining two?

ghengeveld commented 2 years ago

@tmeasday I created a ticket in Linear for it. CH-900

yannbf commented 1 year ago

@ghengeveld can you update us regarding this? Linear changed and I couldn't find that ticket anymore.

ghengeveld commented 1 year ago

Reopening so we don't lose track of the required HMR behavior change (3+4 above). Here's the Linear ticket