update template to avoid potential memory leaks

[achim-k]

Changelog

None

Docs

None

Description

Updates the panel template with

• a comment to discourage adding new dependencies to the main useLayoutEffect hook
• cleanup function for the useLayoutEffet

[jtbandes]

This seems ok, but it's still a bit vague as to why this would lead to memory leaks. In some ways, it makes sense that onRender accesses the current state to perform some logic. Perhaps we need to find a better pattern for onRender if we are suggesting that onRender should not have a dependency on the state. cc @defunctzombie for ideas

[defunctzombie]

// Adding other dependencies here is discouraged as it can lead to memory leaks. Consider using other effects for your panel logic.

As a reader this does not tell me how it can lead to memory leaks and what it means to use other effects for the panel logic.

What would happen if you didn't have that comment but added this cleanup logic?

I also don't understand why we need to this in the panel if the layoutEffect is about to assign a new value? If the panel is removed we should be clearing that at the ExtensionAdapter layer?

[achim-k]

We don't need a closure here as the panel extension adapter creates a new function for every render. It turned out that removing this closure fixes the memory leak I was encountering.

[jtbandes]

This change is wrong — I think you might be misreading the existing code. It is not () => done() but () => done. The reason is since useState supports a function argument (oldState) => newState, it assumes the given done function is supposed to be called and calls it immediately rather than updating the state variable. See docs: https://react.dev/reference/react/useState#im-trying-to-set-state-to-a-function-but-it-gets-called-instead (we should probably link to these docs)

[achim-k]

// Adding other dependencies here is discouraged as it can lead to memory leaks. Consider using other effects for your panel logic.

As a reader this does not tell me how it can lead to memory leaks and what it means to use other effects for the panel logic.

I have removed that comment again.

What would happen if you didn't have that comment but added this cleanup logic?

I also don't understand why we need to this in the panel if the layoutEffect is about to assign a new value? If the panel is removed we should be clearing that at the ExtensionAdapter layer?

It turned out that the cleanup logic is not helping with the memory leak. The main change that fixed it was to remove the closure from setRenderDone(() => done). It's still a bit unclear to me why this fixed it, but the memory retainer tree below indicated that it had to do something with the renderDone callback.

significant changes