-
Notifications
You must be signed in to change notification settings - Fork 47.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor how composite type changes work, fix memory leak in ReactMount caching #4983
Conversation
@sebmarkbage A nice bug fix and some small steps towards not using hierarchical IDs. This should let us try out createElement again. |
This reduces our reliance on hierarchical IDs. If facebook#4983 merges, this can look at `._nativeNode` when present.
This reduces our reliance on hierarchical IDs. If facebook#4983 merges, this can look at `._nativeNode` when present.
This reduces our reliance on hierarchical IDs. If facebook#4983 merges, this can look at `._nativeNode` when present.
@@ -164,10 +175,11 @@ function setID(node, id) { | |||
* @internal | |||
*/ | |||
function getNode(id) { | |||
if (!nodeCache.hasOwnProperty(id) || !isValid(nodeCache[id], id)) { | |||
nodeCache[id] = ReactMount.findReactNodeByID(id); | |||
if (nodeCache.hasOwnProperty(id)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
var node = getNodeIfCached(id);
if(node) return node;
else return nodeCache[id] = ReactMount.findReactNodeByID(id);
At the very least that saves an object lookup (object lookups are surprisingly expensive in javascript). It also reads a little better IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure. I was mostly keeping it how it was.
Squash and ship! (at least, I have no objection :) ) |
Too much for 0.14 unless it fixes a new regression. |
Yeah, wasn't going to do it in 0.14. Also was planning to leave the commits separate since they're separate things. |
Before, if you had ``` container = <div data-reactid=".0"><div data-reactid=".0.0" /></div>; ``` and did `ReactDOM.render(<span />, container)` you would get ``` <div data-reactid=".0"><span data-reactid=".0" /></div>; ``` (along with a warning not to replace React-rendered children with a new tree like that). But that makes no sense -- the span should have a new index, not truncate the ID of the old child it's replacing. (Now tests pass again with useCreateElement on; before they threw a "valid but unequal" on our test for this warning.)
This is probably slightly slower for unmounts in the case that no updates were ever performed, but caching the node on the instance should make updates faster. In any case, the more important consequence of this change is that we can fix the current memory leak that happens when swapping composite types.
With this change, all unmounted components should be properly purged from ReactMount's cache.
It never really made sense for us to have "invalid" nodes in the cache -- when we unmount things, we should always remove them from the cache properly. Now that swapping composite types doesn't repopulate the cache, we should be okay to now assume that everything in the cache is good.
Refactor how composite type changes work, fix memory leak in ReactMount caching
This reduces our reliance on hierarchical IDs. If facebook#4983 merges, this can look at `._nativeNode` when present.
This reduces our reliance on hierarchical IDs. If facebook#4983 merges, this can look at `._nativeNode` when present.
No description provided.