Skip to content
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

Remove unnecessary indirection and events from the hooks #7464

Merged
merged 3 commits into from
Aug 10, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now the only place where we create entries. All others have been changed to vanilla get.

},

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().
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I didn't think we should hit this on unmounted components. setState should bail out before we get to this point. Is there a test that fails without this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised too. Yes, ReactCompositeComponent test about setState in componentWillUnmount fails specifically. I will be taking a closer look later this week when I'll be tracking down errant warnings.

// 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
6 changes: 2 additions & 4 deletions src/renderers/dom/shared/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -272,14 +272,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
9 changes: 0 additions & 9 deletions src/renderers/dom/shared/ReactDOMTextComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,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 +140,6 @@ Object.assign(ReactDOMTextComponent.prototype, {
commentNodes[1],
nextStringText
);

if (__DEV__) {
ReactInstrumentation.debugTool.onSetText(
this._debugID,
nextStringText
);
}
}
}
},
Expand Down
10 changes: 0 additions & 10 deletions src/renderers/native/ReactNativeTextComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,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 +66,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
4 changes: 2 additions & 2 deletions src/test/ReactTestUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ var NoopInternalComponent = function(element) {
this._renderedOutput = element;
this._currentElement = element;
this._debugID = nextDebugID++;
ReactInstrumentation.debugTool.onInstantiateComponent(this._debugID, element);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might also resolve some issues we saw regarding missing elements in stacks

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which were those?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking #7240 (comment) and further reports. But not sure if that was related or not.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(y)

};

NoopInternalComponent.prototype = {
Expand Down Expand Up @@ -412,8 +413,7 @@ var ShallowComponentWrapper = function(element) {
// TODO: Consolidate with instantiateReactComponent
if (__DEV__) {
this._debugID = nextDebugID++;
var displayName = element.type.displayName || element.type.name || 'Unknown';
ReactInstrumentation.debugTool.onSetDisplayName(this._debugID, displayName);
ReactInstrumentation.debugTool.onInstantiateComponent(this._debugID, element);
}

this.construct(element);
Expand Down