renderComponentToString memory leak fix for #954 #960
Conversation
renderComponentToString registers events. If the calling code does not unmount the component or unregister the listeners this will leak memory. Passing this test should fix this.
Flag on EventPluginHub to disable event registration. For the purposes of server side rendering not registering events. Could be cleaner if used a transaction with an optional step. Alternatively could be an ExecutionEnvironment variable. Could also be flag passed to mount component, however non DOM components have no comprehension of events.
EventPluginHub.setRegistrationEnabled(boolean) will toggle whether putListener will register event listeners.
|
Hey @glenn-murray-bse -- Super sorry about not making this clear but I actually just landed a fix for this internally which will be synced soon. I ended up doing the diff a little differently -- I added a new type of queue to ReactReconcileTransaction that would call putListener() after the DOM flush so we wouldn't need to pass around a flag like this. I want to highlight that this was a great PR -- it's awesome to see you jump headfirst into the event system and write tests. Though we don't need this particular PR, we'd certainly love more contributions from you in the future :) Thanks again, and my apologies, Pete |
|
@petehunt I wonder why putListener() is called even if container is not found? https://github.com/facebook/react/blob/0af9c3e/src/core/ReactDOMComponent.js#L76 |
|
No worries, adding an optional step to TRANSACTION_WRAPPERS was an initial idea I had, I agree that transactions are a better way of dealing with this than flags. I'll better get my head around the transaction implementation next time I dive in. |
3 participants
Fixes #954, changes
React.renderComponentToStringto no longer register eventsNew exposed methods