Re-rendering components into a document leaks components #423
Conversation
|
|
||
| var getTestDocument; | ||
|
|
||
| var testDocument; | ||
|
|
||
| // TODO: Same as defined in ReactMount, should we export it from there? |
There was a problem hiding this comment.
Maybe there's a better way to test if we can get a reference to a component by a DOM node?
There was a problem hiding this comment.
Maybe move this function to its own module and require() it from ReactMount and here?
There was a problem hiding this comment.
That wil introduce circular dependeny between new module and ReactMount. I think I can factor out getID, setID and purgeID functions into a separate module to mitigate that... what would be the best name for this module? ReactNodeCache?
There was a problem hiding this comment.
That's probably a good idea. @benjamn ?
There was a problem hiding this comment.
Nope, it's not so easy as I thought — too much coupling with ReactMount internals. Can we export getReactRootID for use in tests?
There was a problem hiding this comment.
I mean export via ReactMount.getReactRootID.
There was a problem hiding this comment.
Sorry for the delay, I didn't realize you were waiting on us!
About the separate module, I think we actually went in the opposite direction (I think we had a ReactID module) and coupled this more tightly to avoid some circular dependencies.
So for now go ahead and export that function.
| @@ -18,12 +18,22 @@ | |||
|
|
|||
| "use strict"; | |||
|
|
|||
| var DOC_NODE_TYPE = 9; | |||
There was a problem hiding this comment.
This is already defined in the codebase. Maybe create a DOMNodeTypes module and put that constant in there?
There was a problem hiding this comment.
Can we use Node.DOCUMENT_NODE for that or it's not standardised?
There was a problem hiding this comment.
I've thought about this before but ended up letting us add a couple constants. I think we should only be accessing these in places where we know we're in a browser environment (since we're checking nodeType), but I wasn't sure at the time so didn't press the matter. Since then though we've added the constants in a couple other places and so long as it's supported in all browsers, we should probably just use Node.*. If anybody knows for sure or wants to test all browsers to make sure, chime up :)
There was a problem hiding this comment.
For the time being, let's just keep this in like this and then followup to replace everything across the codebase separately.
If we are to unmount a component mounted into a document element we should unmount it from document.documentElement and not from document.firstChild which is a doctype element in this specific case.
Re-rendering components into a document leaks components
|
Awesome. Thanks a lot! |
This PR does three things:
Merge test cases only, make sure they are sane, make sure they fail without further commits, then merge the rest and see everything is green.