fix(fela-renderer): fix memleak in Provider component

[layershifter]

Current Behavior

There is a memory leak 🚨

New Behavior

There is no a memory leak ✅

From code it's not clear what was happening, but... JSX is just sugar around JSX pragma:

function App() {
  return <RendererProvider targetDocument={target} />;
}

// ⬇️⬇️⬇️

function App() {
  return React.createElement(RendererProvider, { targetDocument: target });
}

As props (second param are and object) we have stable reference of target (in this case it's an HTML document). As createRenderer() is used in WeakMap:

fluentui/packages/fluentui/react-northstar/src/utils/mergeProviderContexts.ts

Lines 12 to 13 in 165a46a

	const defaultDocument = { document: 'document' };
	const registeredRenderers = new WeakMap<Document | typeof defaultDocument, Renderer>();

We are actually getting:

const map = new WeakMap();

// 💣 this will be never released as a value references its key
map.set(document, { target: document });

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 598f1ac:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

[size-auditor]

Asset size changes

Project	Bundle	Baseline Size	New Size	Difference
office-ui-fabric-react	fluentui-react-northstar-Provider	93.402 kB	93.421 kB	19 bytes

 Over Tolerance (1024 B)  Over Baseline  Below Baseline  New   Removed 1 kB = 1000 B

Baseline commit: b8663b78ad0421793da984de809cc50b8128c1e4 (build)

[fabricteam]

📊 Bundle size report

_{🤖 This report was generated against b8663b78ad0421793da984de809cc50b8128c1e4}

[fabricteam]

Perf Analysis (@fluentui/react)

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
FluentProviderWithTheme	mount	149	134	10	Possible regression

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
Avatar	mount	786	787	5000
BaseButton	mount	812	816	5000
Breadcrumb	mount	2302	2332	1000
ButtonNext	mount	437	440	5000
Checkbox	mount	1359	1342	5000
CheckboxBase	mount	1134	1126	5000
ChoiceGroup	mount	4114	4063	5000
ComboBox	mount	848	861	1000
CommandBar	mount	8922	8972	1000
ContextualMenu	mount	7385	7410	1000
DefaultButton	mount	1003	983	5000
DetailsRow	mount	3230	3208	5000
DetailsRowFast	mount	3254	3189	5000
DetailsRowNoStyles	mount	3067	3051	5000
Dialog	mount	2238	2240	1000
DocumentCardTitle	mount	156	173	1000
Dropdown	mount	2824	2798	5000
FluentProviderNext	mount	1754	1711	5000
FluentProviderWithTheme	mount	149	134	10	Possible regression
FluentProviderWithTheme	virtual-rerender	93	100	10
FluentProviderWithTheme	virtual-rerender-with-unmount	173	160	10
FocusTrapZone	mount	1584	1602	5000
FocusZone	mount	1573	1598	5000
IconButton	mount	1531	1514	5000
Label	mount	331	322	5000
Layer	mount	2564	2580	5000
Link	mount	433	433	5000
MakeStyles	mount	1471	1465	50000
MenuButton	mount	1270	1305	5000
MessageBar	mount	1748	1759	5000
Nav	mount	2848	2844	1000
OverflowSet	mount	967	983	5000
Panel	mount	2187	2169	1000
Persona	mount	763	751	1000
Pivot	mount	1249	1251	1000
PrimaryButton	mount	1107	1120	5000
Rating	mount	6726	6665	5000
SearchBox	mount	1201	1169	5000
Shimmer	mount	2194	2194	5000
Slider	mount	1717	1722	5000
SpinButton	mount	4329	4302	5000
Spinner	mount	392	388	5000
SplitButton	mount	2744	2706	5000
Stack	mount	463	466	5000
StackWithIntrinsicChildren	mount	1990	1984	5000
StackWithTextChildren	mount	4557	4537	5000
SwatchColorPicker	mount	9840	9924	5000
TagPicker	mount	2245	2314	5000
TeachingBubble	mount	11377	11322	5000
Text	mount	389	392	5000
TextField	mount	1234	1226	5000
ThemeProvider	mount	1053	1051	5000
ThemeProvider	virtual-rerender	547	553	5000
ThemeProvider	virtual-rerender-with-unmount	1620	1580	5000
Toggle	mount	732	737	5000
buttonNative	mount	140	142	5000

Perf Analysis (@fluentui/react-northstar)

⚠️ No perf measurements available

[ling1726]

Hmm the actual weakmap usage that I understand from this PR is more like

weakMap.set(document: { target: document })

or am I missing smth ?

[ling1726]

should the CreateRenderer type be updated to stop accepting a target as an arg ?

[layershifter]

There should be a bigger refactor if we will decide to implement compat mode with Griffel. That refactor will remove that WeakMap and resolve the issue from a different angle.

[ling1726]

Is this that big of a refactor ? There should only be a couple of CreateRenderer functions in the library, and leaving the target parameter there is just going to make this easier to repeat, since looking at the types you are encouraged to pass a target in there at creation time.

[layershifter]

Is this that big of a refactor ? There should only be a couple of CreateRenderer functions in the library, and leaving the target parameter there is just going to make this easier to repeat, since looking at the types you are encouraged to pass a target in there at creation time.

Yes. Emotion renderer has different implementation:

fluentui/packages/fluentui/react-northstar-emotion-renderer/src/createEmotionRenderer.tsx

Line 35 in 165a46a

return (target?: Document): Renderer => {

(and yeah, it's probably leaking)

If you're interested, check #20611 and changes in mergeProviderContexts.ts. It aligns implementation with v9 and removes the need in Weakmap. I would rather do that change than try to remove target from arguments.

[layershifter]

Hmm the actual weakmap usage that I understand from this PR is more like

weakMap.set(document: { target: document })

or am I missing smth ?

Yes, it was just for example. I updated the snippet to clarify.