Core: Allow a teardown function to be returned from renderToDOM

[tmeasday]

Issue: Stories (both in Canvas and when rendered in modernInlineRender) had no way to be properly torn down, leading to memory/event listener leaks when browsing around (as noticed in Angular by @kroeder).

What I did

Added some stories to highlight the problem to react-ts (not sure this is where they should go, and if we should have a way to properly test this @shilman).

Notice the following (without the changes to react/../render.tsx):

1. Browse to a buttonX story
2. Go to docs tab
3. Notice event listener for Canvas is still active (you will see click handlers fire one more time than the number of rendered stories in the docs page).
4. Go to docs tab for other buttonX story
5. Notice event listeners from previous docs page are not torn down.

The Canvas event listeners eventually do get torn down because we are ultimately rendering a different story to the same DOM element, and React handles this for us.

But
a) it should happen earlier I think and
b) that doesn't happen automatically in Angular as I understand it.

Solution

I allowed renderToDOM to return a teardown function that will be called whenever a StoryRender is taken off the screen.

How to test

• Is this testable with Jest or Chromatic screenshots?

See reproduction above. I will also add a bunch of PreviewWeb integration tests once we are satisfied this is the right approach.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 661011f. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 1 target

• nx run-many --target=prepare --all --parallel=2 --max-parallel=15 -- --optimized

---

Sent with 💌 from NxCloud.

[shilman]

Code LGTM, but we definitely need some tests for this! 👍

[kroeder]

@tmeasday can I already create a second draft pr with a fix for angular?

[shilman]

@kroeder You should be able to telescope off this PR and create a second PR on top of it

[kroeder]

Already working on it and it looks promising!

[tmeasday]

@kroeder -- discussed with @shilman and a plan to get this into 6.5 with a minimum of disruptive changes would be as follows:

1. Implement the TeardownRenderToDOM more or less as done here in the core.
2. Only actually implement a RenderToDOM that returns the above in angular, maybe in docs mode only.

React (and likely others) should also return a teardown function, at least for docs mode, but we are concerned about making breaking changes especially to the way components/stories are unmounted when changing stories. I would suggest we should be careful for angular in doing that also, which is why I suggest only returning a teardown function in docs mode, at least for now.

As part of the 7.0 cycle we plan to look closely at various existing and closed issues around remounting and get some test in place that'll give us confidence to make this change everywhere.

[tmeasday]

I added some integration tests. We don't currently have a mechanism for full integration tests with the docs stack (i.e. <Story> in modern inline mode) but I would like to add it as part of other canvas tests.

[shilman]

@tmeasday please update snapshots

[shilman]

LGTM. Let's get this working in 7.0 Angular before deciding whether to patch to 6.5

[kroeder]

Awesome! Workin on that starting today :) @shilman

[tmeasday]

If we patch this back we should make sure we don't include this change (in fact should we take it out until we've considered this properly?)