Only purgeID on ReactDOMComponent unmount #1568
Conversation
| @@ -406,6 +406,7 @@ ReactDOMComponent.Mixin = { | |||
| this.unmountChildren(); | |||
| ReactEventEmitter.deleteAllListeners(this._rootNodeID); | |||
| ReactComponent.Mixin.unmountComponent.call(this); | |||
| ReactMount.purgeID(this._rootNodeID); | |||
There was a problem hiding this comment.
nit: can you move this above the super call?
There was a problem hiding this comment.
Makes sense, will do.
|
Looks good to me. |
|
Technically, the neatest solution would be for ReactTextComponent to use a |
|
Or perhaps add it to ReactBrowserComponentMixin? Not sure. |
|
Hmm, yeah that's a good point. I guess the upside of using |
|
react-art also uses it: https://github.com/facebook/react-art/blob/c719f03/src/ReactART.js#L184-L189. |
|
Right, but the solution would be the same for it then, use |
|
Related to discussion, #1598 |
| @@ -97,8 +88,6 @@ var ReactComponent = { | |||
| 'ReactComponent: injectEnvironment() can only be called once.' | |||
| ); | |||
| mountImageIntoNode = ReactComponentEnvironment.mountImageIntoNode; | |||
| unmountIDFromEnvironment = | |||
| ReactComponentEnvironment.unmountIDFromEnvironment; | |||
There was a problem hiding this comment.
This was the only place using ReactComponentEnvironment.unmountIDFromEnvironment - can we probably get rid of that now.
There was a problem hiding this comment.
@zpao Hmm? That's exactly what's going on here? :) (I find no occurences of unmountIDFromEnvironment)
|
Btw, rebased this too. Test plan: |
|
@sebmarkbage Rewrote to fix conflicts, it's still the same basically, but... I chose to use EDIT: I've also ran this PR against our app and I have witnessed no stale nodes in the nodeCache, except for #2988 which is not caused by this PR.
|
Only purgeID on ReactDOMComponent and ReactDOMTextComponent unmount
Fixes #1567