Skip to content

Fix problems with empty component ID registration #2503

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Nov 15, 2014
Merged

Fix problems with empty component ID registration #2503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 19 additions & 5 deletions src/core/ReactCompositeComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,13 @@ var ReactCompositeComponentMixin = assign({},
}
}

var renderedElement = this._renderValidatedComponent();
if (renderedElement === ReactEmptyComponent.emptyElement) {
ReactEmptyComponent.registerNullComponentID(this._rootNodeID);
}

this._renderedComponent = this._instantiateReactComponent(
this._renderValidatedComponent(),
renderedElement,
this._currentElement.type // The wrapping type
);

Expand Down Expand Up @@ -230,6 +235,11 @@ var ReactCompositeComponentMixin = assign({},
}
this._compositeLifeCycleState = null;

if (this._renderedComponent._currentElement ===
ReactEmptyComponent.emptyElement) {
ReactEmptyComponent.deregisterNullComponentID(this._rootNodeID);
}

this._renderedComponent.unmountComponent();
this._renderedComponent = null;

Expand Down Expand Up @@ -641,6 +651,13 @@ var ReactCompositeComponentMixin = assign({},
var thisID = this._rootNodeID;
var prevComponentID = prevComponentInstance._rootNodeID;
prevComponentInstance.unmountComponent();

if (nextRenderedElement === ReactEmptyComponent.emptyElement) {
ReactEmptyComponent.registerNullComponentID(this._rootNodeID);
} else if (prevRenderedElement === ReactEmptyComponent.emptyElement) {
ReactEmptyComponent.deregisterNullComponentID(this._rootNodeID);
}

this._renderedComponent = this._instantiateReactComponent(
nextRenderedElement,
this._currentElement.type
Expand Down Expand Up @@ -683,10 +700,7 @@ var ReactCompositeComponentMixin = assign({},
}
}
if (renderedComponent === null || renderedComponent === false) {
renderedComponent = ReactEmptyComponent.getEmptyComponent();
ReactEmptyComponent.registerNullComponentID(this._rootNodeID);
} else {
ReactEmptyComponent.deregisterNullComponentID(this._rootNodeID);
renderedComponent = ReactEmptyComponent.emptyElement;
}
} finally {
ReactContext.current = previousContext;
Expand Down
15 changes: 8 additions & 7 deletions src/core/ReactEmptyComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,17 @@ var ReactEmptyComponentInjection = {
}
};

/**
* @return {ReactComponent} component The injected empty component.
*/
function getEmptyComponent() {
var ReactEmptyComponentType = function() {};
ReactEmptyComponentType.prototype.render = function() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(My first bare class! Am I doing it right?)

Copy link
Member

Choose a reason for hiding this comment

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

If you uses classes it actually transforms to the same thing :)

class ReactEmptyComponentType {
  render() {
    ...
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hah, I meant my first bare React component class. I can use class if that's preferred.

invariant(
component,
'Trying to return null from a render, but no null placeholder component ' +
'was injected.'
);
return component();
}
};

var emptyElement = ReactElement.createElement(ReactEmptyComponentType);

/**
* Mark the component as having rendered to null.
Expand All @@ -63,9 +63,10 @@ function isNullComponentID(id) {
}

var ReactEmptyComponent = {
deregisterNullComponentID: deregisterNullComponentID,
getEmptyComponent: getEmptyComponent,
emptyElement: emptyElement,
injection: ReactEmptyComponentInjection,

deregisterNullComponentID: deregisterNullComponentID,
isNullComponentID: isNullComponentID,
registerNullComponentID: registerNullComponentID
};
Expand Down
151 changes: 0 additions & 151 deletions src/core/__tests__/ReactCompositeComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ var ReactMount;
var ReactPropTypes;
var ReactServerRendering;
var ReactTestUtils;
var TogglingComponent;

var cx;
var reactComponentExpect;
Expand Down Expand Up @@ -78,23 +77,6 @@ describe('ReactCompositeComponent', function() {
}
});

TogglingComponent = React.createClass({
getInitialState: function() {
return {component: this.props.firstComponent};
},
componentDidMount: function() {
console.log(this.getDOMNode());
this.setState({component: this.props.secondComponent});
},
componentDidUpdate: function() {
console.log(this.getDOMNode());
},
render: function() {
var Component = this.state.component;
return Component ? <Component /> : null;
}
});

warn = console.warn;
console.warn = mocks.getMockFunction();
});
Expand Down Expand Up @@ -147,139 +129,6 @@ describe('ReactCompositeComponent', function() {
.toBeDOMComponentWithTag('a');
});

it('should render null and false as a noscript tag under the hood', () => {
var Component1 = React.createClass({
render: function() {
return null;
}
});
var Component2 = React.createClass({
render: function() {
return false;
}
});

var instance1 = ReactTestUtils.renderIntoDocument(<Component1 />);
var instance2 = ReactTestUtils.renderIntoDocument(<Component2 />);
reactComponentExpect(instance1)
.expectRenderedChild()
.toBeDOMComponentWithTag('noscript');
reactComponentExpect(instance2)
.expectRenderedChild()
.toBeDOMComponentWithTag('noscript');
});

it('should still throw when rendering to undefined', () => {
var Component = React.createClass({
render: function() {}
});
expect(function() {
ReactTestUtils.renderIntoDocument(<Component />);
}).toThrow(
'Invariant Violation: Component.render(): A valid ReactComponent must ' +
'be returned. You may have returned undefined, an array or some other ' +
'invalid object.'
);
});

it('should be able to switch between rendering null and a normal tag', () => {
spyOn(console, 'log');

var instance1 =
<TogglingComponent
firstComponent={null}
secondComponent={'div'}
/>;
var instance2 =
<TogglingComponent
firstComponent={'div'}
secondComponent={null}
/>;

expect(function() {
ReactTestUtils.renderIntoDocument(instance1);
ReactTestUtils.renderIntoDocument(instance2);
}).not.toThrow();

expect(console.log.argsForCall.length).toBe(4);
expect(console.log.argsForCall[0][0]).toBe(null);
expect(console.log.argsForCall[1][0].tagName).toBe('DIV');
expect(console.log.argsForCall[2][0].tagName).toBe('DIV');
expect(console.log.argsForCall[3][0]).toBe(null);
});

it('should distinguish between a script placeholder and an actual script tag',
() => {
spyOn(console, 'log');

var instance1 =
<TogglingComponent
firstComponent={null}
secondComponent={'script'}
/>;
var instance2 =
<TogglingComponent
firstComponent={'script'}
secondComponent={null}
/>;

expect(function() {
ReactTestUtils.renderIntoDocument(instance1);
}).not.toThrow();
expect(function() {
ReactTestUtils.renderIntoDocument(instance2);
}).not.toThrow();

expect(console.log.argsForCall.length).toBe(4);
expect(console.log.argsForCall[0][0]).toBe(null);
expect(console.log.argsForCall[1][0].tagName).toBe('SCRIPT');
expect(console.log.argsForCall[2][0].tagName).toBe('SCRIPT');
expect(console.log.argsForCall[3][0]).toBe(null);
}
);

it('should have getDOMNode return null when multiple layers of composite ' +
'components render to the same null placeholder', () => {
spyOn(console, 'log');

var GrandChild = React.createClass({
render: function() {
return null;
}
});

var Child = React.createClass({
render: function() {
return <GrandChild />;
}
});

var instance1 =
<TogglingComponent
firstComponent={'div'}
secondComponent={Child}
/>;
var instance2 =
<TogglingComponent
firstComponent={Child}
secondComponent={'div'}
/>;

expect(function() {
ReactTestUtils.renderIntoDocument(instance1);
}).not.toThrow();
expect(function() {
ReactTestUtils.renderIntoDocument(instance2);
}).not.toThrow();

expect(console.log.argsForCall.length).toBe(4);
expect(console.log.argsForCall[0][0].tagName).toBe('DIV');
expect(console.log.argsForCall[1][0]).toBe(null);
expect(console.log.argsForCall[2][0]).toBe(null);
expect(console.log.argsForCall[3][0].tagName).toBe('DIV');
}
);

it('should not thrash a server rendered layout with client side one', () => {
var Child = React.createClass({
render: function() {
Expand Down
Loading