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

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

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

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.

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! 🚀

I'm also 💯 onboard with this approach!

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

@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

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.

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

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

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

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