PDA

View Full Version : [FIXED] React component lifecycle method componentWillUnmount is not called by ExtJS



geniny
25 May 2017, 1:13 PM
When you make one of the native React components a child of the ExtReact React component, the native React component's componentWillUnmount will never be called potentially resulting in memory and resource leaks as well as other side effects.

I have the following sample layout:


<App>
<BrowserRouter>
<Container>
<Transition>
<Switch>
<Route path="/1" component={Test1} />
<Route path="/2" component={Test2} />
</Switch>
</Transition>
</Container>
</App>

Test1 renders this:

<Container>
<SomeReactComponentWithLifecycleMethods1 />
</Container>

Test2 renders this:

<Container>
<SomeReactComponentWithLifecycleMethods2 />
</Container>

SomeReactComponentWithLifecycleMethods1 and SomeReactComponentWithLifecycleMethods2 components have both componentWillMount and componentWillUnmount methods. When you switch a route from /1 -> /2 -> /1, etc. only componentWillMount is ever called. componentWillUnmount is never called.

The problem seems to be in the ExtJSComponent.unmountComponent where child components are not iterated and their lifecycle methods are not called is never called.



Thank you,
Yuri

geniny
25 May 2017, 3:18 PM
I found a fix.

All you have to do is modify your ExtJSComponent.unmountComponent (changes are in bold):



unmountComponent(safely) {
if (this.cmp) {
if (this.cmp.destroying || this.cmp.$reactorConfig) return;

this.unmountChildren(safely);

const parentCmp = this.cmp.ownerCt /* classic */ || this.cmp.getParent(); /* modern */

// remember the parent and position in parent for dangerouslyReplaceNodeWithMarkup
// this not needed in fiber
let indexInParent;

if (parentCmp) {
if (parentCmp.indexOf) {
// modern
indexInParent = parentCmp.indexOf(this.cmp);
} else if (parentCmp.items && parentCmp.items.indexOf) {
// classic
indexInParent = parentCmp.items.indexOf(this.cmp);
}
}

if (this.reactorSettings.debug) console.log('destroy', this.cmp.$className);

if (Ext.navigation && Ext.navigation.View && parentCmp && parentCmp instanceof Ext.navigation.View) {
parentCmp.pop();
} else {
this.cmp.destroy();
}

// remember the parent and position in parent for dangerouslyReplaceNodeWithMarkup
// this not needed in fiber
this.el._extIndexInParent = indexInParent;
this.el._extParent = parentCmp;
}
}

Mark.Brocato
26 May 2017, 6:18 AM
I'm not about to reproduce this. I added a componentWillUnmount() method to the About component in reactor-boilerplate and it gets called as expected. Here is my code:



import React from 'react';
import { Container } from '@extjs/ext-react';


export default class About extends React.Component {


componentWillUnmount() {
console.log('Unmount about');
}


render() {
return (
<Container padding="20">
<h1>About this App</h1>
<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Etiam eget leo sed mi imperdiet dictum a id turpis. Suspendisse a ante eu lorem lacinia vestibulum. Suspendisse volutpat malesuada ante, sed fermentum massa auctor in. Praesent semper sodales feugiat. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Mauris mauris ante, suscipit id metus non, venenatis tempor tortor. In ornare tempor ipsum. Sed rhoncus augue urna, ut dapibus odio fringilla vitae. Phasellus malesuada mauris ut nulla varius sodales. Sed et leo luctus, venenatis felis sit amet, vehicula nibh. Curabitur fringilla fringilla nibh, porttitor lacinia urna vestibulum eu. Integer ac aliquet risus. Curabitur imperdiet quis purus at consectetur. Sed ornare vitae felis a scelerisque. Donec mi purus, auctor sit amet molestie nec, imperdiet auctor mauris.</p>
</Container>
)


}
}

geniny
26 May 2017, 6:56 AM
As I said, You have to make the component reside under ExtJS component. Put a native React component (with componentWillUnmount) directly under Container and you will see it.

The reason it works in your case is that About is a direct child of the Route component and it does not have any ExtJSComponent-based anscestor. But anything you put under Container (ExtJSComponent) will not be properly cleaned up.

I'm actually quite surprised that it made it into the release as lifecycle methods are a bread-and-butter of React.

Mark.Brocato
26 May 2017, 7:25 AM
Yep you're totally right. Sorry, I didn't see the fix you posted until just now. I arrived at the same one without seeing it so it must be right :) Let me do some testing first and I'll push out a prerelease shortly.

Mark.Brocato
26 May 2017, 7:36 AM
Good news: that also seems to fix the bug with componentWillUnmount not being called on components rendered by Templates. Thank you for helping with this!

geniny
26 May 2017, 7:50 AM
Actually, it should not. The template uses ReactDOM.render which renders it into an independent subtree.

So technically, it is not part of the pipeline. Try to use it with a paged grid and see if it is called when the new page is rendered.

geniny
26 May 2017, 9:05 AM
Just confirmed that componentWillUnmount is not called when a component is embedded in the template and the template goes out of scope. This is expected as ReactDOM.render renders into an independent DOM tree so the upstream React pipelines do not know anything about it. Plus, Facebook docs explicitly say that you have to call unmountComponentFromNode to do that:

https://facebook.github.io/react/docs/react-dom.html#unmountcomponentatnode

geniny
26 May 2017, 9:20 AM
And I also noticed that in your POC implementation of the ExtJSRenderer for fiber you do not call ReactDOM.unmountComponentFromNode either.

Mark.Brocato
26 May 2017, 12:29 PM
Thanks for the report! I have opened a bug in our bug tracker.

Mark.Brocato
16 Jun 2017, 11:08 AM
The latest prerelease of @extjs/reactor fixes the issues reported here. You can install it with:


npm i @extjs/[email protected]

sbobetdome
18 Jun 2017, 8:47 PM
thanks