[Lens] Fully unmount React when flyout closes

[wylieconlon]

Steps to reproduce:

1. Configure a dimension
2. Open the flyout
3. Open the React devtools and look for one of the I18nProvider elements at the bottom of the inspector, which is rendered as a separate React root.
4. Close the flyout
5. Hover over each I18nProvider in the dev tools and verify that there is a matching and visible DOM element.

There is a performance penalty associated with this change. The way I compared the performance before and after was by comparing the time to re-open the flyout, and found that it was 0.3ms to render when the element is kept in the virtual DOM, vs 15.9ms when the element is rendered from the first time. Steps to check:

1. Follow all the steps above
2. Switch to the React profiler tab in dev tools
3. Start recording
4. Click the dimension to open the flyout
5. Close the flyout
6. Stop recording

Then I tabbed through each commit cycle in React, and found React executes 9 cycles with this PR, vs 2 cycles before. It also takes much longer. I believe this is a necessary slowdown because keeping the flyout element in virtual DOM causes more problems and is a potential memory leak.

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

[elasticmachine]

Pinging @elastic/kibana-app (Team:KibanaApp)

[flash1293]

The failure looks real - maybe there's a leftover render function which is not containing an unmount callback?

22:18:18                 │ERROR browser[SEVERE] http://localhost:61131/41502/bundles/kbn-ui-shared-deps/kbn-ui-shared-deps.js 381:77071 TypeError: cleanupRef.current is not a function
22:18:18                 │          at http://localhost:61131/41502/bundles/plugin/lens/lens.chunk.0.js:8:258355

[wylieconlon]

@flash1293 yes, it was a real failure. The editor frame uses an async function to do its rendering, and I am now correctly handling this case.

[flash1293]

This PR adds a way to return a cleanup function (even an async one) from the render function, but it seems like this is never used, right?

The

unmountComponentAtNode(elementRef.current);

line is basically a default implementation of the cleanup specific to react and it does all the actual cleanup here.

I guess my question is: Do we actually need the unmount callback logic? Or is it more of a future-facing thing?

[wylieconlon]

Yes, the cleanupRef is only used in the new tests It's there to maintain the existing assumption that Lens is framework independent.

[mbondyra]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 63eec62
• Storybooks Preview

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
lens	931.3KB	931.9KB	+591.0B

History

• 💚 Build #115314 succeeded 5df313c
• 💔 Build #115290 failed 502f74d
• 💔 Build #115042 failed 2334c1b

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[flash1293]

Tested and works as expected, thanks for explaining!

[kibanamachine]

💚 Backport successful

✅ 7.x / #95677

This backport PR will be merged automatically after passing CI.