Skip to content

Commit

Permalink
Remove unnecessary indirection and events from the hooks (#7464)
Browse files Browse the repository at this point in the history
* Remove unnecessary indirection from the tree hook

* Replace onSetDisplayName, onSetOwner, onSetText with one event

Less events is better.
onSetDisplayName, onSetOwner, and onSetText only existed because we didn't initially track elements.

* Remove unused variables
  • Loading branch information
gaearon authored Aug 10, 2016
1 parent afa27bc commit 1f31357
Show file tree
Hide file tree
Showing 7 changed files with 108 additions and 137 deletions.
178 changes: 101 additions & 77 deletions src/isomorphic/hooks/ReactComponentTreeHook.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,21 +39,16 @@ function remove(id) {
delete itemByKey[key];
}

function update(id, updater) {
function create(id, element) {
var key = getKeyFromID(id);
if (!itemByKey[key]) {
itemByKey[key] = {
element: null,
parentID: null,
ownerID: null,
text: null,
childIDs: [],
displayName: 'Unknown',
isMounted: false,
updateCount: 0,
};
}
updater(itemByKey[key]);
itemByKey[key] = {
element,
parentID: null,
text: null,
childIDs: [],
isMounted: false,
updateCount: 0,
};
}

function purgeDeep(id) {
Expand All @@ -76,6 +71,18 @@ function describeComponentFrame(name, source, ownerName) {
);
}

function getDisplayName(element) {
if (element == null) {
return '#empty';
} else if (typeof element === 'string' || typeof element === 'number') {
return '#text';
} else if (typeof element.type === 'string') {
return element.type;
} else {
return element.type.displayName || element.type.name || 'Unknown';
}
}

function describeID(id) {
var name = ReactComponentTreeHook.getDisplayName(id);
var element = ReactComponentTreeHook.getElement(id);
Expand All @@ -94,88 +101,93 @@ function describeID(id) {
}

var ReactComponentTreeHook = {
onSetDisplayName(id, displayName) {
update(id, item => item.displayName = displayName);
},

onSetChildren(id, nextChildIDs) {
update(id, item => {
item.childIDs = nextChildIDs;

nextChildIDs.forEach(nextChildID => {
var nextChild = get(nextChildID);
invariant(
nextChild,
'Expected hook events to fire for the child ' +
'before its parent includes it in onSetChildren().'
);
invariant(
nextChild.displayName != null,
'Expected onSetDisplayName() to fire for the child ' +
'before its parent includes it in onSetChildren().'
);
invariant(
nextChild.childIDs != null || nextChild.text != null,
'Expected onSetChildren() or onSetText() to fire for the child ' +
'before its parent includes it in onSetChildren().'
);
invariant(
nextChild.isMounted,
'Expected onMountComponent() to fire for the child ' +
'before its parent includes it in onSetChildren().'
);
if (nextChild.parentID == null) {
nextChild.parentID = id;
// TODO: This shouldn't be necessary but mounting a new root during in
// componentWillMount currently causes not-yet-mounted components to
// be purged from our tree data so their parent ID is missing.
}
invariant(
nextChild.parentID === id,
'Expected onSetParent() and onSetChildren() to be consistent (%s ' +
'has parents %s and %s).',
nextChildID,
nextChild.parentID,
id
);
});
});
},

onSetOwner(id, ownerID) {
update(id, item => item.ownerID = ownerID);
var item = get(id);
item.childIDs = nextChildIDs;

for (var i = 0; i < nextChildIDs.length; i++) {
var nextChildID = nextChildIDs[i];
var nextChild = get(nextChildID);
invariant(
nextChild,
'Expected hook events to fire for the child ' +
'before its parent includes it in onSetChildren().'
);
invariant(
nextChild.childIDs != null ||
typeof nextChild.element !== 'object' ||
nextChild.element == null,
'Expected onSetChildren() to fire for a container child ' +
'before its parent includes it in onSetChildren().'
);
invariant(
nextChild.isMounted,
'Expected onMountComponent() to fire for the child ' +
'before its parent includes it in onSetChildren().'
);
if (nextChild.parentID == null) {
nextChild.parentID = id;
// TODO: This shouldn't be necessary but mounting a new root during in
// componentWillMount currently causes not-yet-mounted components to
// be purged from our tree data so their parent ID is missing.
}
invariant(
nextChild.parentID === id,
'Expected onSetParent() and onSetChildren() to be consistent (%s ' +
'has parents %s and %s).',
nextChildID,
nextChild.parentID,
id
);
}
},

onSetParent(id, parentID) {
update(id, item => item.parentID = parentID);
var item = get(id);
item.parentID = parentID;
},

onSetText(id, text) {
update(id, item => item.text = text);
onInstantiateComponent(id, element) {
create(id, element);
},

onBeforeMountComponent(id, element) {
update(id, item => item.element = element);
var item = get(id);
item.element = element;
},

onBeforeUpdateComponent(id, element) {
update(id, item => item.element = element);
var item = get(id);
if (!item || !item.isMounted) {
// We may end up here as a result of setState() in componentWillUnmount().
// In this case, ignore the element.
return;
}
item.element = element;
},

onMountComponent(id) {
update(id, item => item.isMounted = true);
var item = get(id);
item.isMounted = true;
},

onMountRootComponent(id) {
rootIDs[id] = true;
},

onUpdateComponent(id) {
update(id, item => item.updateCount++);
var item = get(id);
if (!item || !item.isMounted) {
// We may end up here as a result of setState() in componentWillUnmount().
// In this case, ignore the element.
return;
}
item.updateCount++;
},

onUnmountComponent(id) {
update(id, item => item.isMounted = false);
var item = get(id);
item.isMounted = false;
unmountedIDs[id] = true;
delete rootIDs[id];
},
Expand Down Expand Up @@ -234,8 +246,11 @@ var ReactComponentTreeHook = {
},

getDisplayName(id) {
var item = get(id);
return item ? item.displayName : 'Unknown';
var element = ReactComponentTreeHook.getElement(id);
if (!element) {
return null;
}
return getDisplayName(element);
},

getElement(id) {
Expand All @@ -244,8 +259,11 @@ var ReactComponentTreeHook = {
},

getOwnerID(id) {
var item = get(id);
return item ? item.ownerID : null;
var element = ReactComponentTreeHook.getElement(id);
if (!element || !element._owner) {
return null;
}
return element._owner._debugID;
},

getParentID(id) {
Expand All @@ -261,8 +279,14 @@ var ReactComponentTreeHook = {
},

getText(id) {
var item = get(id);
return item ? item.text : null;
var element = ReactComponentTreeHook.getElement(id);
if (typeof element === 'string') {
return element;
} else if (typeof element === 'number') {
return '' + element;
} else {
return null;
}
},

getUpdateCount(id) {
Expand Down
8 changes: 2 additions & 6 deletions src/renderers/dom/shared/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -270,16 +270,12 @@ if (__DEV__) {
}

this._contentDebugID = contentDebugID;
var text = '' + content;

ReactInstrumentation.debugTool.onSetDisplayName(contentDebugID, '#text');
ReactInstrumentation.debugTool.onSetParent(contentDebugID, debugID);
ReactInstrumentation.debugTool.onSetText(contentDebugID, text);

if (hasExistingContent) {
ReactInstrumentation.debugTool.onBeforeUpdateComponent(contentDebugID, content);
ReactInstrumentation.debugTool.onUpdateComponent(contentDebugID);
} else {
ReactInstrumentation.debugTool.onInstantiateComponent(contentDebugID, content);
ReactInstrumentation.debugTool.onSetParent(contentDebugID, debugID);
ReactInstrumentation.debugTool.onBeforeMountComponent(contentDebugID, content);
ReactInstrumentation.debugTool.onMountComponent(contentDebugID);
ReactInstrumentation.debugTool.onSetChildren(debugID, [contentDebugID]);
Expand Down
10 changes: 0 additions & 10 deletions src/renderers/dom/shared/ReactDOMTextComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
var DOMChildrenOperations = require('DOMChildrenOperations');
var DOMLazyTree = require('DOMLazyTree');
var ReactDOMComponentTree = require('ReactDOMComponentTree');
var ReactInstrumentation = require('ReactInstrumentation');

var escapeTextContentForBrowser = require('escapeTextContentForBrowser');
var invariant = require('invariant');
Expand Down Expand Up @@ -67,8 +66,6 @@ Object.assign(ReactDOMTextComponent.prototype, {
context
) {
if (__DEV__) {
ReactInstrumentation.debugTool.onSetText(this._debugID, this._stringText);

var parentInfo;
if (hostParent != null) {
parentInfo = hostParent._ancestorInfo;
Expand Down Expand Up @@ -142,13 +139,6 @@ Object.assign(ReactDOMTextComponent.prototype, {
commentNodes[1],
nextStringText
);

if (__DEV__) {
ReactInstrumentation.debugTool.onSetText(
this._debugID,
nextStringText
);
}
}
}
},
Expand Down
11 changes: 0 additions & 11 deletions src/renderers/native/ReactNativeTextComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

'use strict';

var ReactInstrumentation = require('ReactInstrumentation');
var ReactNativeComponentTree = require('ReactNativeComponentTree');
var ReactNativeTagHandles = require('ReactNativeTagHandles');
var UIManager = require('UIManager');
Expand All @@ -29,10 +28,6 @@ var ReactNativeTextComponent = function(text) {
Object.assign(ReactNativeTextComponent.prototype, {

mountComponent: function(transaction, hostParent, hostContainerInfo, context) {
if (__DEV__) {
ReactInstrumentation.debugTool.onSetText(this._debugID, this._stringText);
}

// TODO: hostParent should have this context already. Stop abusing context.
invariant(
context.isInAParentText,
Expand Down Expand Up @@ -70,12 +65,6 @@ Object.assign(ReactNativeTextComponent.prototype, {
'RCTRawText',
{text: this._stringText}
);
if (__DEV__) {
ReactInstrumentation.debugTool.onSetText(
this._debugID,
nextStringText
);
}
}
}
},
Expand Down
12 changes: 2 additions & 10 deletions src/renderers/shared/ReactDebugTool.js
Original file line number Diff line number Diff line change
Expand Up @@ -283,26 +283,18 @@ var ReactDebugTool = {
onSetState() {
emitEvent('onSetState');
},
onSetDisplayName(debugID, displayName) {
checkDebugID(debugID);
emitEvent('onSetDisplayName', debugID, displayName);
},
onSetChildren(debugID, childDebugIDs) {
checkDebugID(debugID);
childDebugIDs.forEach(checkDebugID);
emitEvent('onSetChildren', debugID, childDebugIDs);
},
onSetOwner(debugID, ownerDebugID) {
checkDebugID(debugID);
emitEvent('onSetOwner', debugID, ownerDebugID);
},
onSetParent(debugID, parentDebugID) {
checkDebugID(debugID);
emitEvent('onSetParent', debugID, parentDebugID);
},
onSetText(debugID, text) {
onInstantiateComponent(debugID, element) {
checkDebugID(debugID);
emitEvent('onSetText', debugID, text);
emitEvent('onInstantiateComponent', debugID, element);
},
onMountRootComponent(debugID) {
checkDebugID(debugID);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,21 +41,6 @@ function getDeclarationErrorAddendum(owner) {
return '';
}

function getDisplayName(instance) {
var element = instance._currentElement;
if (element == null) {
return '#empty';
} else if (typeof element === 'string' || typeof element === 'number') {
return '#text';
} else if (typeof element.type === 'string') {
return element.type;
} else if (instance.getName) {
return instance.getName() || 'Unknown';
} else {
return element.type.displayName || element.type.name || 'Unknown';
}
}

/**
* Check if the type reference is a known internal type. I.e. not a user
* provided composite type.
Expand Down Expand Up @@ -144,12 +129,7 @@ function instantiateReactComponent(node, shouldHaveDebugID) {
if (shouldHaveDebugID) {
var debugID = nextDebugID++;
instance._debugID = debugID;
var displayName = getDisplayName(instance);
ReactInstrumentation.debugTool.onSetDisplayName(debugID, displayName);
var owner = node && node._owner;
if (owner) {
ReactInstrumentation.debugTool.onSetOwner(debugID, owner._debugID);
}
ReactInstrumentation.debugTool.onInstantiateComponent(debugID, node);
} else {
instance._debugID = 0;
}
Expand Down
Loading

0 comments on commit 1f31357

Please sign in to comment.