Storybook: [app: react] HMR causes story components to unmount way too aggressively

[rhendric]

Describe the bug

In a plain, completely unmodified create-react-app project, I can make edits with hot module replacement and my components don't unmount most of the time.

In a Storybook, if I sneeze, my components unmount.

Okay, I'm joking about the sneeze, but seriously: I can have a story from file A open, and edit file B, with no dependency relationship between the two files at all (except for the fact that they are both automatically depended on by the preview entry point), and everything in my current story will unmount. Most of the time, the re-rendering is fast enough that this isn't noticeable, but when mounting and unmounting matters, it really matters.

To Reproduce

Steps to reproduce the behavior:

1. Clone https://github.com/rhendric/super-duper-computing-machine
2. Run yarn storybook
3. Select the MountCounter story
4. Edit src/stories/Other.stories.js
5. Watch the unmount count tick upwards
6. Edit the prop field in src/stories/MountCounter.stories.js.
7. Watch the unmount count keep going
8. For comparison, run yarn start and make edits to src/index.js, and watch the unmount count not budge.

Expected behavior

Hot module replacement should not force a remount of the current story, unless the current story no longer exists or something catastrophic like that. It's fine if React itself unmounts a component because its code was replaced, but I think the only time Storybook should force a remount is if Storybook changed the currently active story.

System:

  System:
    OS: Linux 5.6 Arch Linux
    CPU: (2) x64 Intel(R) Core(TM)2 Duo CPU     P8600  @ 2.40GHz
  Binaries:
    Node: 14.0.0 - /usr/bin/node
    Yarn: 1.22.4 - ~/.local/bin/yarn
    npm: 6.14.4 - /usr/bin/npm
  npmPackages:
    @storybook/preset-create-react-app: ^2.1.1 => 2.1.1 
    @storybook/react: ^5.3.18 => 5.3.18 

[shilman]

cc @ndelangen @tmeasday

[tmeasday]

I’d like to look at this as part of 6.0 qa

[stale[bot]]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[tmeasday]

OK, so this is a problem that carried over from the current SB to SB6.

The basic reason for it is our mechanism to know whether to re-render a story based on HMR is way to rough -- we simply have a revision counter on the store that we globally update whenever HMR happens.

I was looking at this and I think we can instead simply check if the getDecorated value on the store data matches the previous version. We update that whenever we run setStory on the store, which happens when we reload the module that the story is defined in.

I'll try putting a PR together.

[rhendric]

Hey, that sounds like good progress, but will it address the case of HMRing the current story? From my last look at the SB5 code (from several weeks ago; my memory may be rusty), calling setStory passes down a boolean that ultimately causes SB to force an unmount of the story before rendering, which makes sense if the story is actually changing, but if the story is just a new revision of the same story, I don't think it should be force-unmounted (React should be smart enough to unmount things if it needs to). From your description, it doesn't sound like that's going to change; is that correct, and if so why?

[tmeasday]

@rhendric in your example, when you edit Other.stories.js the stories defined in MountCounter.stories.js won't be re-added to the store. So if you have one of those on the screen it will not re-render.

All the stories from Other.stories.js will be cleared and re-added to the store however. There's no way to tell if they are changed or not though.

[rhendric]

Ah, so you're saying that removing the stories from the store is what unmounts them?

Even so, couldn't the stories be replaced in the store instead of removing and re-adding them? Just remove the story IDs that no longer appear in the newly loaded version of Other.stories.js.

Maybe other frameworks need to know whether a story has changed or not, but one of the strengths of React is that it keeps track of what needs changing; you only have to feed it the component tree you want. So for React, I still don't see why SB would need to force an unmount if the story that is on screen gets updated. Forcing an unmount should only be reserved for when the app is switching to a different story, because it would be bad for component state to leak between two different stories if parts of their component tree happen to coincide.

I see that integrating react-refresh is in progress in #10694; making sure force unmounts of the current story don't happen is surely a prerequisite for that to have any benefit, although maybe that means you'd want to consider changing how stories are reloaded in the story store as part of that work instead of part of this issue.

[shilman]

Boo-yah!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.0-beta.15 containing PR #10908 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

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

[tmeasday]

@rhendric - I think there are two situations to think about here:

1. The story file changes
2. The actual component implementation changes (potentially somewhere down the hierarchy from the component the story is rendering)

Case 1 is what I am looking at here. In this case we definitely need to re-render the story which from React's perspective means unmounting and remounting the component right? I might be missing something but even with something like React Refresh I don't think it is possible to maintain the mounted component if its implementation changes (in this case the component that's changing is the __story__, which is a SFC).

Case 2 is more interesting. I don't know much about it but I sort of assume if React Refresh is handling the HMR event at a deeper level (i.e. for a particular component) then the module.hot.X events won't end up firing at the level of the story file and so the store will be unchanged. Does that sound possible?

[tmeasday]

Having said that I am open to discussing the "remount" behaviour of SB in React generally. If you look at the implementation:

https://github.com/storybookjs/storybook/blob/e4ffcdb5263fafa3f6fe4f7c68c238b8e33e6595/app/react/src/client/preview/render.tsx#L65:L72

You'll see the only time we don't force a remount of the component is when a force rerender happens (that's for knobs or controls, etc). The reason given is a super-old issue that is lost to the depth of time as far as I can tell.

Now, I am not sure if that affects this case (like I said, I think that if the story implementation changes React is going to do a remount anyway) but in the changing story for a single component case I've always wonder if that's what people want. You say:

Forcing an unmount should only be reserved for when the app is switching to a different story, because it would be bad for component state to leak between two different stories if parts of their component tree happen to coincide.

Do you mean two stories of the same component? Or of different components?

[rhendric]

Case 1 is what I am looking at here. In this case we definitely need to re-render the story which from React's perspective means unmounting and remounting the component right? I might be missing something but even with something like React Refresh I don't think it is possible to maintain the mounted component if its implementation changes (in this case the component that's changing is the story, which is a SFC).

Ah, I see that the implementation does use the story function as a function component, which makes what you say true. However, if instead of rendering <ErrorBoundary><StoryFn /></ErrorBoundary>, you rendered <ErrorBoundary>{storyFn()}</ErrorBoundary>, then the story implementation could change, and React wouldn't unmount unless the component types returned by the story function were to change. I'm not sure what the advantage of treating the story function as a function component would be, but maybe I'm missing it. It doesn't seem to be a documented feature; the documentation for CSF seems to just call the exported things ‘story functions’ and doesn't give any hints that specific frameworks might treat them as more than just functions.

But also, and don't ask me how it works because I have no idea, react-refresh does seem to do something special that prevents some inner unmounts in this way. As in, maybe <StoryFn />'s implementation changes, but as long as the same subtree gets rendered by that component, the components in the subtree don't need to be unmounted. So maybe with react-refresh that change would be unnecessary.

Case 2 is more interesting. I don't know much about it but I sort of assume if React Refresh is handling the HMR event at a deeper level (i.e. for a particular component) then the module.hot.X events won't end up firing at the level of the story file and so the store will be unchanged. Does that sound possible?

Don't quote me but I think that's correct.

Forcing an unmount should only be reserved for when the app is switching to a different story, because it would be bad for component state to leak between two different stories if parts of their component tree happen to coincide.

Do you mean two stories of the same component? Or of different components?

I wrote that before I understood that every story is a component in itself, which should already prevent any such leaks—now I don't understand why that forced unmount is there at all! But if story functions were called instead of being made into components, then two stories of different components (meaning, to be clear, two story functions that return React components of different types, not implying anything about whether those story functions are in the same story file) can't leak state to each other, because React would unmount the old one when rendering the other. The force unmount is only relevant if the component types match. But to be safe, I support forcing an unmount any time a new story is selected, regardless of whether it's strictly necessary—just not when a currently selected story is reloaded.

[tmeasday]

I'm not sure what the advantage of treating the story function as a function component would be, but maybe I'm missing it.

The main reason is if you treat the (decorated) story function as a component you can use hooks inside stories and decorators.

But also, and don't ask me how it works because I have no idea, react-refresh does seem to do something special that prevents some inner unmounts in this way. As in, maybe 's implementation changes, but as long as the same subtree gets rendered by that component, the components in the subtree don't need to be unmounted. So maybe with react-refresh that change would be unnecessary.

Hmm, yeah maybe we should find out about that magic.

The force unmount is only relevant if the component types match.

I guess this is the use case that's interesting, if the component types match but the story is different do we want it to unmount? What I am hearing is yes.

But to be safe, I support forcing an unmount any time a new story is selected, regardless of whether it's strictly necessary—just not when a currently selected story is reloaded.

I think we could do this by passing a flag into the render function I linked to above that tells it "we are re-rendering the same story due to HMR, so don't unmount". We could give it a try?

[tmeasday]

I think we could do this by passing a flag into the render function I linked to above that tells it "we are re-rendering the same story due to HMR, so don't unmount". We could give it a try?

I tried it and it remounts it anyway (at least in this version of HMR)

[rhendric]

The main reason is if you treat the (decorated) story function as a component you can use hooks inside stories and decorators.

So yeah, that can be useful, but (A) is that documented anywhere? and (B) I don't think it's that big a deal to write

const TestHarness = () => {
  useSomeHooks();
  return <ComponentUnderTest/>;
};
export const MyStory = () => <TestHarness/>;

instead of

export const MyStory = () => {
  useSomeHooks();
  return <ComponentUnderTest/>;
};

if hooks are needed in a story? If the fact that story functions are React components is in fact undocumented, I imagine this may be what most developers are doing already if they want hooks.

Decorators are even more likely to follow this pattern; is it presuming too much to expect any reasonably complicated decorator to look roughly like storyFn => <MyDecorator>{storyFn()}</MyDecorator>, where hooks can be used in the implementation of MyDecorator?

These are pretty speculative opinions from a guy who just recently started using Storybook, mind; I'm prepared for my serving of crow if I'm wrong about what everyone else is likely to want/do.

if the component types match but the story is different do we want it to unmount? What I am hearing is _yes_.

Yes indeed, if by ‘the story is different’ we mean something like ‘the story has a different story ID’, and not something like ‘the story implementation has been updated’.

I tried it and it remounts it anyway (at least in this version of HMR)

What exactly did you try? I think at this point in the conversation, we've identified three-ish possible things that _all_ need to change in order to prevent a remount:

1. When a story module hot-reloads, its stories need to be modified in place in the store, not removed and re-added (assuming that this comment meant removing a story from the store immediately triggers some event that causes the story to be unmounted, which is not something we went over in much detail so I may have misunderstood).
2. The force render flag needs to be set.
3. _Either_ <StoryFn /> needs to be changed to {storyFn()}, _or_ react-refresh needs to be involved somehow, because without react-refresh and if the story function is the implementation of a component, altering the story function causes React to unmount it.

[tmeasday]

I guess I just did 2. in your list above. I should try 2&3, will let you know when I get an opportunity.

I’m not sure what you 1. means though. How can you update the implementation without changing the story function?

Ps if 3. Is required for this to work (the changing part of code not be rendered via JSX/createElement) but we ask folks to refactor their stories to render the important bits via JSX (so they can use hooks) then there’s a problem right?

[rhendric]

I’m not sure what you 1. means though. How can you update the implementation without changing the story function?

Maybe you should just explain to me a little more what you meant by this comment, in which you said:

All the stories from Other.stories.js will be cleared and re-added to the store however.

From that, I made the leap that clearing a story from the store would cause it to be unmounted—maybe there's some kind of storyRemoved event that gets emitted, which causes an unmount if the removed story is the current story—and so my 1. above was about suppressing that event. I envisioned something like calling store.replaceStory(storyId, newStoryImplementation) instead of store.remove(storyId); store.addStory(storyId, newStoryImplementation), although I don't know what the actual API is. I did look up the story store implementation, and I didn't see anything like what you're talking about, so it seems likely I just misunderstood what you meant, in which case 1. likely isn't a thing. But I could just have been looking in the wrong place.

Ps if 3. Is required for this to work (the changing part of code not be rendered via JSX/createElement) but we ask folks to refactor their stories to render the important bits via JSX (so they can use hooks) then there’s a problem right?

Maybe? One, that's why I asked if this was documented; it wasn't obvious to me that it would work the way it does and I wonder how many folks would actually need to do this. And two, this is work for a major release, right? If there's a time for breaking changes, it seems like now might be it, especially if not many people are affected due to the previous point.

I don't know. IMHO, it wouldn't be terrible if the JSX change didn't happen as long as react-refresh can hold up that pillar instead. Seems like there's a good amount of support behind getting that done, and I personally could settle for waiting for it to land. My concern is just that we might not know if react-refresh can or can't do that until it's actually tried, since it seems neither of us know enough about it to say conclusively, and by then it very likely would be too late for breaking changes for a while.

[tmeasday]

All the stories from Other.stories.js will be cleared and re-added to the store however.

It just means that the old functions in the store from the Other.stories.js file (which no longer "exist" according to webpack but I guess are in memory still maybe) get removed and the new functions (some of which may have the same implementation but point to new locations in memory) get added.

Nothing actually happens on the rendering side until we try and re-render the current story, which we do because we see the implementation changed: https://github.com/storybookjs/storybook/blob/a7da781ff462b443e1219bc78d6ca9692ddc8e43/lib/core/src/client/preview/StoryRenderer.tsx#L162-L163

This is the right thing to do (re-render the function), as it is different. It could be completely different, we have no way to know!

The question is whether we should force an unmount when we do that (rather than let React naturally do one if required).

---

I did some more experiments with our current setup and here's what I discovered. What I did:

1. Changed our React layer's rendering to render the story as a function (not an element), and to not force unmounting (what we referred to as 2&3 above).

2. Initially this broke the rendering story (unsurprisingly as it uses hooks).

3. I then moved the "story component" into a separate file and made the story trivial (export const MyCounter = () => <Counter />, where Counter was moved to a separate file). This led to the following behaviour:
   a. If I changed the story file, the component re-rendered without unmounting (success!)
   b. If I changed the "story component", the component re-mounted.

4. Then I tried putting the story component in the same file, but keeping the story super simple (as above). Now if I made any change in the story file the component re-mounted.

I don't think there are any great surprises here, excepting maybe that you have to put it in a separate file, which is a pain! The question is: is it worth breaking the "stories are React components" behaviour[1] in order to enable 3a?

Let's talk about that for a second. My point is that 3a is not interesting if we force folks to move all their story "logic" into a separate file. Apart from the inconvenience of doing so, people are way more likely to edit the actual implementation of the story that the trivial bit that just renders that implementation. So my feeling is that we would have gained a fairly pyrrhic victory in doing so. WDYT?

[1] This may not be currently documented but we are reworking the docs, and I can tell you that plenty of folks assume it is true and have complained when it didn't work that way in the past.

[rhendric]

My point is that 3a is _not interesting_ if we force folks to move all their story "logic" into a separate file. Apart from the inconvenience of doing so, people are way more likely to edit the actual implementation of the story that the trivial bit that just renders that implementation. So my feeling is that we would have gained a fairly pyrrhic victory in doing so. WDYT?

Totally agree with your analysis for the case of stories that use hooks. Also, I will take your word for it that a non-trivial number of users needs hooks in their stories. But I would propose a fifth experiment, of a story that doesn't use hooks and only returns the component under test with a particular set of properties (possibly using framework-agnostic addons such as knobs). I would expect such an experiment to yield results similar to 3a, but without needing the extra file or the trivial wrapper. In this case, the victory is not pyrrhic; something of real value is gained for no cost (to this particular story/user).

Assuming I'm right about that, I would frame the tradeoff as imposing a refactoring cost (making the trivial story wrapper in order for hooks to work, but not moving the bulk of the story to its own file) on some fraction of stories which would receive no benefit, in exchange for a HMR benefit for the rest of the stories (which conversely would suffer no cost). If the fraction of stories that need hooks is a, the benefit of having HMR not force an unmount on a single non-hook-using story is b, and the cost of refactoring a single hook-using story to have a trivial wrapper is c, then the change is worth considering if . I'm not expecting exact quantification here, but what's your general sense of the relative values of those ratios? For example, if b and c are roughly equal, then we should consider this if hooks are present in fewer than half of all stories; if c is five times worse then b is good, then this is only worth talking about if hooks are used less than one-sixth of the time. Personally, I think I'd place the b/c ratio at around ½, but I might be biased in favor of my own needs, and I clearly have no idea where a is.

[tmeasday]

I'm pretty sure you are correct that HMR would work as you wanted in such cases. I also see your point, what you are saying is we tell people: stories are not components, if you want your story to be a component, you need to refactor it a little bit and make a "story component", keeping the story function simple, and by the way you'll lose this HMR benefit if you do so.

Apart from the cost benefit you talk about (I'm not really sure I am well placed to judge it but I would say I've not heard people complain about stories re-mounting on HMR before[1]) there's also a documentation cost -- how do we tell people to write stories in the documentation and tutorials like Learn Storybook? Do we keep it simple and avoid hooks at all costs, mentioning them as an advanced feature with attendant caveats, or do we use "story components" from the start, with a note about getting HMR benefits if they do it another way?

[1] Can you tell me a bit more about the use case? What do these stories look like that you are editing repeatedly and want to remain mounted?

[rhendric]

I also see your point, what you are saying is we tell people: stories _are not components_

Yup, exactly! Just like how in other frameworks, stories aren't Vue or Angular components, are they? Stories are functions that return objects of a type specific to the framework. The current behavior for React is an exception to the rule that would get ironed out.

(I haven't actually tried any other frameworks; this is just based on reading the documentation. If the other frameworks also support the equivalent of hooks in their story functions, disregard the above paragraph.)

how do we tell people to write stories in the documentation and tutorials like Learn Storybook? Do we keep it simple and avoid hooks at all costs, mentioning them as an advanced feature with attendant caveats, or do we use "story components" from the start, with a note about getting HMR benefits if they do it another way?

The first one, I would think! For someone new to Storybook who discovers that they want hooks in their stories, creating a distinct story component is probably the first thing they'd try after using hooks in the story doesn't work. I wouldn't even call it a feature of Storybook, advanced or not; it's just the logical conclusion from the premises that React hooks work in React components, and that you can return any React component in a story. The pattern only really merits documenting as a breaking change for people already used to the current way.

But this reminds me of a thought I had a few days back and forgot to propose: I wonder if, after the change from stories-are-components to stories-are-functions, the old behavior could be restored by adding StoryFn => <StoryFn /> as a decorator? In that case, refactoring wouldn't be necessary at all; you could add the decorator once globally and be done. You'd still be sacrificing the no-remounting benefit, but since that's no change from the status quo, no big deal.

[1] Can you tell me a bit more about the use case? What do these stories look like that you are editing repeatedly and want to remain mounted?

They have iframes in them. Any time a new iframe is created, there's a significant lag while the browser initializes the distinct browsing context. My iframe-wrapping components take care to reuse the existing iframe, but of course they clean up when they are unmounted. Reloading these stories on every edit is painful.

I also have a small number of components that have an intro animation effect and use local state to remember that the animation has been played. While sometimes I do want to start the animation again after an edit, it's more common that I'm making an edit to something that only affects the post-animation state of the component, and in that case it's annoying to have to wait for the animation to complete to see the update. It also disturbs the ability to detect small changes in the rendered output. I'd rather have the default behavior be to reuse the current state, and trigger animation restarts manually by switching stories.

[tmeasday]

When you say edits in this case, you mean edits to the components not the stories though right? So would this whole conversation be a concern for you, assuming React Refresh worked and such edits didn't involve HMR-ing the story?

[rhendric]

Well, I guess I mean both, but I was actually thinking about edits to the stories. When debugging some visual glitch, I'll iterate on the story to try to find the configuration of props that puts the component in the most glitchy state, then fix the glitch, and leave the story as-is as a sort of regression test. Lots of times, the fix will be CSS-only, which is already fast even without react-refresh; the slow part is pinning down the glitch when editing props in the story.

[tmeasday]

OK, so I discussed this a bit further with @shilman.

Firstly, the pattern of iterating on the story inputs until you find the misbehaving configuration and saving it as a story is totally in line with how we think Storybook should be used. One way we think that'll work well in 6.0 is with the new Args feature, where you write stories like:

const ComponentStory = (args) => <Component {...args} />

export const MyStory = ComponentStory.bind({});
MyStory.args = { /* input props here */ };

Then if you use the controls addon you can tweak those args in the browser until you find the combination to work on. In the future we'd even like to make an easy way to save the args out of the browser into a new story, which would complete the circle here.

If you work in that fashion, you won't have the remounting issue as the story "force re-renders" (which doesn't involve a remount) when you change args.

---

Having said that, in the future if (as we suspect) we want to double down on this approach, we think we probably would want you to have the same experience if you choose to edit the arg values in the editor rather than the browser.

For that to be the case, as we've discussed here, we'd have to change the React story renderer to not render the story as an element.

As we see it in 6.0 that seems a bit too much of a change to make (as we know that issues with using hooks in stories come up a lot, so a lot of people are relying on the story rendering as a element), as we are also pushing this change towards using args as a first class thing.

[One thing In particular: if you were relying on the above, every decorator in your list of decorators would need to be careful not to render the story as an element (that's fine for any of the cross-framework decorators of course)].

[A second thing: if any of your decorators return elements (for instance if they add a provider to the render tree), this is going to break down. I'm not sure if that's going to be a deal breaker or not].

However, one thing we'd be interested in experimenting with is an opt-in feature to get this behaviour in 6.0, I guess with a view to making it default in 7.0. Would you be interested in that?

[rhendric]

An opt-in feature would be an elegant solution as far as I'm concerned. That would also buy time for the react-refresh stuff to land and then we could reevaluate whether any change would even be necessary for 7.0.

Two questions: the opt-in wouldn't depend on using the new args pattern, would it? And I don't understand your caveat about decorators returning elements—are you saying that would be a problem for the args pattern or for the opt-in story rendering change or both?

[tmeasday]

Two questions: the opt-in wouldn't depend on using the new args pattern, would it?

No I guess it wouldn't.

And I don't understand your caveat about decorators returning elements—are you saying that would be a problem for the args pattern or for the opt-in story rendering change or both?

The only way React won't remount your component if the story function changes is if the old and new story functions are pure functions that ultimately return <SomeComponent> where SomeComponent hasn't changed. (SomeComponent can be a function too of course).

However if somewhere in your decorator list you have something like:

const myDecorator = (StoryFn) => <ThemeProvider ...><StoryFn /></ThemeProvider>`

Then from React's perspective the returned element tree from your story fn has changed (as StoryFn has changed).

So you'd need to be careful to ensure that it returns

const myDecorator = (StoryFn) => <ThemeProvider ...>{StoryFn()}</ThemeProvider>`

Incidentally this is exact opposite of what we'll be documenting people do when they add such theme providers!

[rhendric]

Oh good, that's all as I would have hoped. Solid :+1: from me then!

[tmeasday]

OK I created a ticket: https://github.com/storybookjs/storybook/issues/11212

I've added it to the 6.0 milestone. Not sure when I'll get to it but hopefully before then!