Memory leak in renderToStaticNew

[paulsharratt0]

Description

In plate\packages\serializer-html\src\utils\renderToStaticMarkupClient.ts

	let root = ReactDOMClient.createRoot(div);
	ReactDOM.flushSync(() => {
		root.render(elem);
	});

If you leave the react root like this without cleaning up it'll persist. You should unmount root. Something like this:

const renderToStaticNew = (elem: ReturnType<typeof createElementWithSlate>) => {
	let div = document.createElement('div');
	let root = ReactDOMClient.createRoot(div);
	ReactDOM.flushSync(() => {
		root.render(elem);
	});
	let html = div.innerHTML;
	ReactDOM.flushSync(() => {
		root.unmount();
		root = null;
	});
	div.remove();
	div = null;
	return html;
};

Reproduction URL

No response

Reproduction steps

Open dev tools, click memory tab, allocation sampling will do, start recording, run serializeHtml 20 times on document with 20 paragraphs, note the orphaned dom elements and js listeners.

Plate version

37.0.7

Slate React version

0.107.1

Screenshots

No response

Logs

No response

Browsers

No response

Funding

• You can sponsor this specific effort via a Polar.sh pledge below
• We receive the pledge once the issue is completed & verified

[paulsharratt0]

This is the wrong solution, the root element should be reused or you're wasting a bunch of processing power killing and recreating react instances.

Reusing the same React renderer leads to another problem; that each execution takes longer than the previous. I'm not quite certain on the underlying cause but in my test project there's about a 50ms growth for each subsequent operation if we reuse the same React instance. I believe it's related to React attempting to be smart and reuse DOM elements where possible, but the content inside the DOM elements is changing drastically and leading to some kind of reactive execution penalty.

This does however hint to a superior methodology - as React is reusing the DOM elements when it can, help it out by creating multiple roots, one per plugin, and feed the render operation to the appropriate React instance. This reduces the serialization time of a very simple document I'm testing with by 50%, I expect you'll get even better performance improvements in components with more complex DOM structures. There could be some hidden bugs with this methodology - I believe React may hit some issues with update depth. I've not seen any issues, but it could happen.

Here's a code sample of a very basic implementation of this solution

let reactClients = {};
const renderToStaticNew = (
	elem: ReturnType<typeof createElementWithSlate>,
	type
) => {
	if (!reactClients[type]) {
		reactClients[type] = {};
		reactClients[type].div = document.createElement('div');
		reactClients[type].root = ReactDOMClient.createRoot(
			reactClients[type].div,
			{
				identifierPrefix: type
			}
		);
	}

	ReactDOM.flushSync(() => {
		reactClients[type].root.render(elem);
	});

	return reactClients[type].div.innerHTML;
};

identifierPrefix may not be needed as we're not attaching the React instance to the document, but I've added it just in case - my level of React knowledge is rather basic.

You'll need to pass the plugin.type param from the two calling functions:

elementToHtml line 60
leafToHtml line 48

I toyed with a cleanup function to unmount the React instances, but their resource usage is really quite low as we're reusing them for each type and between executions. Perhaps they should cleanup when the editor itself is unmounted though?

Something like this in the renderToStaticMarkClient.ts file would do the job:

export function serializorCleanup() {
	for (let type in reactClients) {
		reactClients[type].root.unmount();
		reactClients[type].div.remove();
		reactClients[type].root = null;
		reactClients[type].div = null;
	}
	reactClients = {};
}