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

Pass shouldHaveDebugID flag to instantiateComponent #7193

Merged
merged 3 commits into from
Jul 5, 2016
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 0 additions & 5 deletions src/isomorphic/devtools/ReactComponentTreeDevtool.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,6 @@ function updateTree(id, update) {
isMounted: false,
updateCount: 0,
};
// TODO: We need to do this awkward dance because TopLevelWrapper "never
// gets mounted" but its display name gets set in instantiateReactComponent
// before its debug ID is set to 0.
unmountedIDs[id] = true;
}
update(tree[id]);
}
Expand Down Expand Up @@ -148,7 +144,6 @@ var ReactComponentTreeDevtool = {

onMountComponent(id) {
updateTree(id, item => item.isMounted = true);
delete unmountedIDs[id];
},

onMountRootComponent(id) {
Expand Down
8 changes: 1 addition & 7 deletions src/renderers/dom/client/ReactMount.js
Original file line number Diff line number Diff line change
Expand Up @@ -342,13 +342,7 @@ var ReactMount = {
);

ReactBrowserEventEmitter.ensureScrollValueMonitoring();
var componentInstance = instantiateReactComponent(nextElement);

if (__DEV__) {
// Mute future events from the top level wrapper.
// It is an implementation detail that devtools should not know about.
componentInstance._debugID = 0;
}
var componentInstance = instantiateReactComponent(nextElement, false);

// The initial render is synchronous but any updates that happen during
// rendering, in componentWillMount or componentDidMount, will be batched
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/dom/server/ReactServerRendering.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ function renderToStringImpl(element, makeStaticMarkup) {
if (__DEV__) {
ReactInstrumentation.debugTool.onBeginFlush();
}
var componentInstance = instantiateReactComponent(element);
var componentInstance = instantiateReactComponent(element, true);
var markup = ReactReconciler.mountComponent(
componentInstance,
transaction,
Expand Down
10 changes: 2 additions & 8 deletions src/renderers/native/ReactNativeMount.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,17 +135,11 @@ var ReactNativeMount = {

ReactNativeTagHandles.assertRootTag(containerTag);

var instance = instantiateReactComponent(nextWrappedElement);
var instance = instantiateReactComponent(nextWrappedElement, false);
ReactNativeMount._instancesByContainerID[containerTag] = instance;

if (__DEV__) {
// Mute future events from the top level wrapper.
// It is an implementation detail that devtools should not know about.
instance._debugID = 0;

if (__DEV__) {
ReactInstrumentation.debugTool.onBeginFlush();
}
ReactInstrumentation.debugTool.onBeginFlush();
}

// The initial render is synchronous but any updates that happen during
Expand Down
1 change: 1 addition & 0 deletions src/renderers/shared/ReactDebugTool.js
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ var ReactDebugTool = {
},
onSetChildren(debugID, childDebugIDs) {
checkDebugID(debugID);
childDebugIDs.forEach(checkDebugID);
emitEvent('onSetChildren', debugID, childDebugIDs);
},
onSetOwner(debugID, ownerDebugID) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1716,103 +1716,137 @@ describe('ReactComponentTreeDevtool', () => {
expect(ReactComponentTreeTestUtils.getRegisteredDisplayNames()).toEqual([]);
});

it('creates current stack addenda', () => {
function getAddendum(element) {
var addendum = ReactComponentTreeDevtool.getCurrentStackAddendum(element);
return addendum.replace(/\(at .+?:\d+\)/g, '(at **)');
}
describe('stack addenda', () => {
it('gets created', () => {
function getAddendum(element) {
var addendum = ReactComponentTreeDevtool.getCurrentStackAddendum(element);
return addendum.replace(/\(at .+?:\d+\)/g, '(at **)');
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These changes are just indentation. I moved an existing test into a common describe and added two more tests based on the issue reports.

var Anon = React.createClass({displayName: null, render: () => null});
var Orange = React.createClass({render: () => null});

expect(getAddendum()).toBe(
''
);
expect(getAddendum(<div />)).toBe(
'\n in div (at **)'
);
expect(getAddendum(<Anon />)).toBe(
'\n in Unknown (at **)'
);
expect(getAddendum(<Orange />)).toBe(
'\n in Orange (at **)'
);
expect(getAddendum(React.createElement(Orange))).toBe(
'\n in Orange'
);

var renders = 0;
var rOwnedByQ;

function Q() {
return (rOwnedByQ = React.createElement(R));
}
function R() {
return <div><S /></div>;
}
class S extends React.Component {
componentDidMount() {
// Check that the parent path is still fetched when only S itself is on
// the stack.
this.forceUpdate();
var Anon = React.createClass({displayName: null, render: () => null});
var Orange = React.createClass({render: () => null});

expect(getAddendum()).toBe(
''
);
expect(getAddendum(<div />)).toBe(
'\n in div (at **)'
);
expect(getAddendum(<Anon />)).toBe(
'\n in Unknown (at **)'
);
expect(getAddendum(<Orange />)).toBe(
'\n in Orange (at **)'
);
expect(getAddendum(React.createElement(Orange))).toBe(
'\n in Orange'
);

var renders = 0;
var rOwnedByQ;

function Q() {
return (rOwnedByQ = React.createElement(R));
}
render() {
expect(getAddendum()).toBe(
'\n in S (at **)' +
'\n in div (at **)' +
'\n in R (created by Q)' +
'\n in Q (at **)'
);
expect(getAddendum(<span />)).toBe(
'\n in span (at **)' +
'\n in S (at **)' +
'\n in div (at **)' +
'\n in R (created by Q)' +
'\n in Q (at **)'
);
expect(getAddendum(React.createElement('span'))).toBe(
'\n in span (created by S)' +
'\n in S (at **)' +
'\n in div (at **)' +
'\n in R (created by Q)' +
'\n in Q (at **)'
);
renders++;
return null;
function R() {
return <div><S /></div>;
}
}
ReactDOM.render(<Q />, document.createElement('div'));
expect(renders).toBe(2);
class S extends React.Component {
componentDidMount() {
// Check that the parent path is still fetched when only S itself is on
// the stack.
this.forceUpdate();
}
render() {
expect(getAddendum()).toBe(
'\n in S (at **)' +
'\n in div (at **)' +
'\n in R (created by Q)' +
'\n in Q (at **)'
);
expect(getAddendum(<span />)).toBe(
'\n in span (at **)' +
'\n in S (at **)' +
'\n in div (at **)' +
'\n in R (created by Q)' +
'\n in Q (at **)'
);
expect(getAddendum(React.createElement('span'))).toBe(
'\n in span (created by S)' +
'\n in S (at **)' +
'\n in div (at **)' +
'\n in R (created by Q)' +
'\n in Q (at **)'
);
renders++;
return null;
}
}
ReactDOM.render(<Q />, document.createElement('div'));
expect(renders).toBe(2);

// Make sure owner is fetched for the top element too.
expect(getAddendum(rOwnedByQ)).toBe(
'\n in R (created by Q)'
);
});
// Make sure owner is fetched for the top element too.
expect(getAddendum(rOwnedByQ)).toBe(
'\n in R (created by Q)'
);
});

it('creates stack addenda by ID', () => {
function getAddendum(id) {
var addendum = ReactComponentTreeDevtool.getStackAddendumByID(id);
return addendum.replace(/\(at .+?:\d+\)/g, '(at **)');
}
it('can be retrieved by ID', () => {
function getAddendum(id) {
var addendum = ReactComponentTreeDevtool.getStackAddendumByID(id);
return addendum.replace(/\(at .+?:\d+\)/g, '(at **)');
}

class Q extends React.Component {
render() {
return null;
class Q extends React.Component {
render() {
return null;
}
}
}

var q = ReactDOM.render(<Q />, document.createElement('div'));
expect(getAddendum(ReactInstanceMap.get(q)._debugID)).toBe(
'\n in Q (at **)'
);

spyOn(console, 'error');
getAddendum(-17);
expect(console.error.calls.count()).toBe(1);
expect(console.error.calls.argsFor(0)[0]).toBe(
'Warning: ReactComponentTreeDevtool: Missing React element for debugID ' +
'-17 when building stack'
);
});
var q = ReactDOM.render(<Q />, document.createElement('div'));
expect(getAddendum(ReactInstanceMap.get(q)._debugID)).toBe(
'\n in Q (at **)'
);

spyOn(console, 'error');
getAddendum(-17);
expect(console.error.calls.count()).toBe(1);
expect(console.error.calls.argsFor(0)[0]).toBe(
'Warning: ReactComponentTreeDevtool: Missing React element for ' +
'debugID -17 when building stack'
);
});

it('is created during mounting', () => {
// https://github.com/facebook/react/issues/7187
var el = document.createElement('div');
var portalEl = document.createElement('div');
class Foo extends React.Component {
componentWillMount() {
ReactDOM.render(<div />, portalEl);
}
render() {
return <div><div /></div>;
}
}
ReactDOM.render(<Foo />, el);
});

it('is created when calling renderToString during render', () => {
// https://github.com/facebook/react/issues/7190
var el = document.createElement('div');
class Foo extends React.Component {
render() {
return (
<div>
<div>
{ReactDOMServer.renderToString(<div />)}
</div>
</div>
);
}
}
ReactDOM.render(<Foo />, el);
});
})
});
4 changes: 2 additions & 2 deletions src/renderers/shared/stack/reconciler/ReactChildReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ function instantiateChild(childInstances, child, name, selfDebugID) {
);
}
if (child != null && keyUnique) {
childInstances[name] = instantiateReactComponent(child);
childInstances[name] = instantiateReactComponent(child, true);
}
}

Expand Down Expand Up @@ -128,7 +128,7 @@ var ReactChildReconciler = {
ReactReconciler.unmountComponent(prevChild, false);
}
// The child must be instantiated before it's mounted.
var nextChildInstance = instantiateReactComponent(nextElement);
var nextChildInstance = instantiateReactComponent(nextElement, true);
nextChildren[name] = nextChildInstance;
// Creating mount image now ensures refs are resolved in right order
// (see https://github.com/facebook/react/pull/7101 for explanation).
Expand Down
12 changes: 8 additions & 4 deletions src/renderers/shared/stack/reconciler/ReactCompositeComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -473,9 +473,11 @@ var ReactCompositeComponentMixin = {
renderedElement = this._renderValidatedComponent();
}

this._renderedNodeType = ReactNodeTypes.getType(renderedElement);
var nodeType = ReactNodeTypes.getType(renderedElement);
this._renderedNodeType = nodeType;
var child = this._instantiateReactComponent(
renderedElement
renderedElement,
nodeType !== ReactNodeTypes.EMPTY /* shouldHaveDebugID */
);
this._renderedComponent = child;
if (__DEV__) {
Expand Down Expand Up @@ -990,9 +992,11 @@ var ReactCompositeComponentMixin = {
var oldHostNode = ReactReconciler.getHostNode(prevComponentInstance);
ReactReconciler.unmountComponent(prevComponentInstance, false);

this._renderedNodeType = ReactNodeTypes.getType(nextRenderedElement);
var nodeType = ReactNodeTypes.getType(nextRenderedElement);
this._renderedNodeType = nodeType;
var child = this._instantiateReactComponent(
nextRenderedElement
nextRenderedElement,
nodeType !== ReactNodeTypes.EMPTY /* shouldHaveDebugID */
);
this._renderedComponent = child;
if (__DEV__) {
Expand Down
13 changes: 9 additions & 4 deletions src/renderers/shared/stack/reconciler/ReactMultiChild.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,15 @@ if (__DEV__) {
}
};
setChildrenForInstrumentation = function(children) {
ReactInstrumentation.debugTool.onSetChildren(
getDebugID(this),
children ? Object.keys(children).map(key => children[key]._debugID) : []
);
var debugID = getDebugID(this);
// TODO: React Native empty components are also multichild.
// This means they still get into this method but don't have _debugID.
if (debugID !== 0) {
ReactInstrumentation.debugTool.onSetChildren(
debugID,
children ? Object.keys(children).map(key => children[key]._debugID) : []
);
}
};
}

Expand Down
Loading