@samitouri / QOS-React-2 / commits / 795290d1b0

Fix problems with empty component ID registration

Fixes #2493, also closes #2353 as it incorporates a variant of that fix. - Instead of having getEmptyComponent return `<noscript />` directly, wrap it in a class that we can easily identify later as being an empty component - Cache the empty component element instead of recreating one each time - Avoid touching the nullComponentIdsRegistry dictionary at all when not dealing with empty components - Move empty-component tests to a separate file Test Plan: jest

Ben Alpert committed Nov 11, 2014 at 21:22 UTC 795290d1b080c5c5960c24389333870bfe5931b5
5 files changed +253 -163
src/core/ReactCompositeComponent.js
+19 -5
@@ -196,8 +196,13 @@ var ReactCompositeComponentMixin = assign({},
196 }
197 }
198
199 + var renderedElement = this._renderValidatedComponent();
200 + if (renderedElement === ReactEmptyComponent.emptyElement) {
201 + ReactEmptyComponent.registerNullComponentID(this._rootNodeID);
202 + }
203 +
204 this._renderedComponent = this._instantiateReactComponent(
200 - this._renderValidatedComponent(),
205 + renderedElement,
206 this._currentElement.type // The wrapping type
207 );
208
@@ -230,6 +235,11 @@ var ReactCompositeComponentMixin = assign({},
235 }
236 this._compositeLifeCycleState = null;
237
238 + if (this._renderedComponent._currentElement ===
239 + ReactEmptyComponent.emptyElement) {
240 + ReactEmptyComponent.deregisterNullComponentID(this._rootNodeID);
241 + }
242 +
243 this._renderedComponent.unmountComponent();
244 this._renderedComponent = null;
245
@@ -641,6 +651,13 @@ var ReactCompositeComponentMixin = assign({},
651 var thisID = this._rootNodeID;
652 var prevComponentID = prevComponentInstance._rootNodeID;
653 prevComponentInstance.unmountComponent();
654 +
655 + if (nextRenderedElement === ReactEmptyComponent.emptyElement) {
656 + ReactEmptyComponent.registerNullComponentID(this._rootNodeID);
657 + } else if (prevRenderedElement === ReactEmptyComponent.emptyElement) {
658 + ReactEmptyComponent.deregisterNullComponentID(this._rootNodeID);
659 + }
660 +
661 this._renderedComponent = this._instantiateReactComponent(
662 nextRenderedElement,
663 this._currentElement.type
@@ -683,10 +700,7 @@ var ReactCompositeComponentMixin = assign({},
700 }
701 }
702 if (renderedComponent === null || renderedComponent === false) {
686 - renderedComponent = ReactEmptyComponent.getEmptyComponent();
687 - ReactEmptyComponent.registerNullComponentID(this._rootNodeID);
688 - } else {
689 - ReactEmptyComponent.deregisterNullComponentID(this._rootNodeID);
703 + renderedComponent = ReactEmptyComponent.emptyElement;
704 }
705 } finally {
706 ReactContext.current = previousContext;
src/core/ReactEmptyComponent.js
+8 -7
@@ -26,17 +26,17 @@ var ReactEmptyComponentInjection = {
26 }
27 };
28
29 -/**
30 - * @return {ReactComponent} component The injected empty component.
31 - */
32 -function getEmptyComponent() {
29 +var ReactEmptyComponentType = function() {};
30 +ReactEmptyComponentType.prototype.render = function() {
31 invariant(
32 component,
33 'Trying to return null from a render, but no null placeholder component ' +
34 'was injected.'
35 );
36 return component();
39 -}
37 +};
38 +
39 +var emptyElement = ReactElement.createElement(ReactEmptyComponentType);
40
41 /**
42 * Mark the component as having rendered to null.
@@ -63,9 +63,10 @@ function isNullComponentID(id) {
63 }
64
65 var ReactEmptyComponent = {
66 - deregisterNullComponentID: deregisterNullComponentID,
67 - getEmptyComponent: getEmptyComponent,
66 + emptyElement: emptyElement,
67 injection: ReactEmptyComponentInjection,
68 +
69 + deregisterNullComponentID: deregisterNullComponentID,
70 isNullComponentID: isNullComponentID,
71 registerNullComponentID: registerNullComponentID
72 };
src/core/__tests__/ReactCompositeComponent-test.js
-151
@@ -21,7 +21,6 @@ var ReactMount;
21 var ReactPropTypes;
22 var ReactServerRendering;
23 var ReactTestUtils;
24 -var TogglingComponent;
24
25 var cx;
26 var reactComponentExpect;
@@ -78,23 +77,6 @@ describe('ReactCompositeComponent', function() {
77 }
78 });
79
81 - TogglingComponent = React.createClass({
82 - getInitialState: function() {
83 - return {component: this.props.firstComponent};
84 - },
85 - componentDidMount: function() {
86 - console.log(this.getDOMNode());
87 - this.setState({component: this.props.secondComponent});
88 - },
89 - componentDidUpdate: function() {
90 - console.log(this.getDOMNode());
91 - },
92 - render: function() {
93 - var Component = this.state.component;
94 - return Component ? <Component /> : null;
95 - }
96 - });
97 -
80 warn = console.warn;
81 console.warn = mocks.getMockFunction();
82 });
@@ -147,139 +129,6 @@ describe('ReactCompositeComponent', function() {
129 .toBeDOMComponentWithTag('a');
130 });
131
150 - it('should render null and false as a noscript tag under the hood', () => {
151 - var Component1 = React.createClass({
152 - render: function() {
153 - return null;
154 - }
155 - });
156 - var Component2 = React.createClass({
157 - render: function() {
158 - return false;
159 - }
160 - });
161 -
162 - var instance1 = ReactTestUtils.renderIntoDocument(<Component1 />);
163 - var instance2 = ReactTestUtils.renderIntoDocument(<Component2 />);
164 - reactComponentExpect(instance1)
165 - .expectRenderedChild()
166 - .toBeDOMComponentWithTag('noscript');
167 - reactComponentExpect(instance2)
168 - .expectRenderedChild()
169 - .toBeDOMComponentWithTag('noscript');
170 - });
171 -
172 - it('should still throw when rendering to undefined', () => {
173 - var Component = React.createClass({
174 - render: function() {}
175 - });
176 - expect(function() {
177 - ReactTestUtils.renderIntoDocument(<Component />);
178 - }).toThrow(
179 - 'Invariant Violation: Component.render(): A valid ReactComponent must ' +
180 - 'be returned. You may have returned undefined, an array or some other ' +
181 - 'invalid object.'
182 - );
183 - });
184 -
185 - it('should be able to switch between rendering null and a normal tag', () => {
186 - spyOn(console, 'log');
187 -
188 - var instance1 =
189 - <TogglingComponent
190 - firstComponent={null}
191 - secondComponent={'div'}
192 - />;
193 - var instance2 =
194 - <TogglingComponent
195 - firstComponent={'div'}
196 - secondComponent={null}
197 - />;
198 -
199 - expect(function() {
200 - ReactTestUtils.renderIntoDocument(instance1);
201 - ReactTestUtils.renderIntoDocument(instance2);
202 - }).not.toThrow();
203 -
204 - expect(console.log.argsForCall.length).toBe(4);
205 - expect(console.log.argsForCall[0][0]).toBe(null);
206 - expect(console.log.argsForCall[1][0].tagName).toBe('DIV');
207 - expect(console.log.argsForCall[2][0].tagName).toBe('DIV');
208 - expect(console.log.argsForCall[3][0]).toBe(null);
209 - });
210 -
211 - it('should distinguish between a script placeholder and an actual script tag',
212 - () => {
213 - spyOn(console, 'log');
214 -
215 - var instance1 =
216 - <TogglingComponent
217 - firstComponent={null}
218 - secondComponent={'script'}
219 - />;
220 - var instance2 =
221 - <TogglingComponent
222 - firstComponent={'script'}
223 - secondComponent={null}
224 - />;
225 -
226 - expect(function() {
227 - ReactTestUtils.renderIntoDocument(instance1);
228 - }).not.toThrow();
229 - expect(function() {
230 - ReactTestUtils.renderIntoDocument(instance2);
231 - }).not.toThrow();
232 -
233 - expect(console.log.argsForCall.length).toBe(4);
234 - expect(console.log.argsForCall[0][0]).toBe(null);
235 - expect(console.log.argsForCall[1][0].tagName).toBe('SCRIPT');
236 - expect(console.log.argsForCall[2][0].tagName).toBe('SCRIPT');
237 - expect(console.log.argsForCall[3][0]).toBe(null);
238 - }
239 - );
240 -
241 - it('should have getDOMNode return null when multiple layers of composite ' +
242 - 'components render to the same null placeholder', () => {
243 - spyOn(console, 'log');
244 -
245 - var GrandChild = React.createClass({
246 - render: function() {
247 - return null;
248 - }
249 - });
250 -
251 - var Child = React.createClass({
252 - render: function() {
253 - return <GrandChild />;
254 - }
255 - });
256 -
257 - var instance1 =
258 - <TogglingComponent
259 - firstComponent={'div'}
260 - secondComponent={Child}
261 - />;
262 - var instance2 =
263 - <TogglingComponent
264 - firstComponent={Child}
265 - secondComponent={'div'}
266 - />;
267 -
268 - expect(function() {
269 - ReactTestUtils.renderIntoDocument(instance1);
270 - }).not.toThrow();
271 - expect(function() {
272 - ReactTestUtils.renderIntoDocument(instance2);
273 - }).not.toThrow();
274 -
275 - expect(console.log.argsForCall.length).toBe(4);
276 - expect(console.log.argsForCall[0][0].tagName).toBe('DIV');
277 - expect(console.log.argsForCall[1][0]).toBe(null);
278 - expect(console.log.argsForCall[2][0]).toBe(null);
279 - expect(console.log.argsForCall[3][0].tagName).toBe('DIV');
280 - }
281 - );
282 -
132 it('should not thrash a server rendered layout with client side one', () => {
133 var Child = React.createClass({
134 render: function() {
src/core/__tests__/ReactEmptyComponent-test.js new
+225
@@ -0,0 +1,225 @@
1 +/**
2 + * Copyright 2014, Facebook, Inc.
3 + * All rights reserved.
4 + *
5 + * This source code is licensed under the BSD-style license found in the
6 + * LICENSE file in the root directory of this source tree. An additional grant
7 + * of patent rights can be found in the PATENTS file in the same directory.
8 + *
9 + * @emails react-core
10 + */
11 +
12 +"use strict";
13 +
14 +var React;
15 +var ReactEmptyComponent;
16 +var ReactTestUtils;
17 +var TogglingComponent;
18 +
19 +var reactComponentExpect;
20 +
21 +describe('ReactEmptyComponent', function() {
22 + beforeEach(function() {
23 + require('mock-modules').dumpCache();
24 +
25 + React = require('React');
26 + ReactEmptyComponent = require('ReactEmptyComponent');
27 + ReactTestUtils = require('ReactTestUtils');
28 +
29 + reactComponentExpect = require('reactComponentExpect');
30 +
31 + TogglingComponent = React.createClass({
32 + getInitialState: function() {
33 + return {component: this.props.firstComponent};
34 + },
35 + componentDidMount: function() {
36 + console.log(this.getDOMNode());
37 + this.setState({component: this.props.secondComponent});
38 + },
39 + componentDidUpdate: function() {
40 + console.log(this.getDOMNode());
41 + },
42 + render: function() {
43 + var Component = this.state.component;
44 + return Component ? <Component /> : null;
45 + }
46 + });
47 + });
48 +
49 + it('should render null and false as a noscript tag under the hood', () => {
50 + var Component1 = React.createClass({
51 + render: function() {
52 + return null;
53 + }
54 + });
55 + var Component2 = React.createClass({
56 + render: function() {
57 + return false;
58 + }
59 + });
60 +
61 + var instance1 = ReactTestUtils.renderIntoDocument(<Component1 />);
62 + var instance2 = ReactTestUtils.renderIntoDocument(<Component2 />);
63 + reactComponentExpect(instance1)
64 + .expectRenderedChild()
65 + .toBeComponentOfType(ReactEmptyComponent.emptyElement.type);
66 + reactComponentExpect(instance2)
67 + .expectRenderedChild()
68 + .toBeComponentOfType(ReactEmptyComponent.emptyElement.type);
69 + });
70 +
71 + it('should still throw when rendering to undefined', () => {
72 + var Component = React.createClass({
73 + render: function() {}
74 + });
75 + expect(function() {
76 + ReactTestUtils.renderIntoDocument(<Component />);
77 + }).toThrow(
78 + 'Invariant Violation: Component.render(): A valid ReactComponent must ' +
79 + 'be returned. You may have returned undefined, an array or some other ' +
80 + 'invalid object.'
81 + );
82 + });
83 +
84 + it('should be able to switch between rendering null and a normal tag', () => {
85 + spyOn(console, 'log');
86 +
87 + var instance1 =
88 + <TogglingComponent
89 + firstComponent={null}
90 + secondComponent={'div'}
91 + />;
92 + var instance2 =
93 + <TogglingComponent
94 + firstComponent={'div'}
95 + secondComponent={null}
96 + />;
97 +
98 + expect(function() {
99 + ReactTestUtils.renderIntoDocument(instance1);
100 + ReactTestUtils.renderIntoDocument(instance2);
101 + }).not.toThrow();
102 +
103 + expect(console.log.argsForCall.length).toBe(4);
104 + expect(console.log.argsForCall[0][0]).toBe(null);
105 + expect(console.log.argsForCall[1][0].tagName).toBe('DIV');
106 + expect(console.log.argsForCall[2][0].tagName).toBe('DIV');
107 + expect(console.log.argsForCall[3][0]).toBe(null);
108 + });
109 +
110 + it('should distinguish between a script placeholder and an actual script tag',
111 + () => {
112 + spyOn(console, 'log');
113 +
114 + var instance1 =
115 + <TogglingComponent
116 + firstComponent={null}
117 + secondComponent={'script'}
118 + />;
119 + var instance2 =
120 + <TogglingComponent
121 + firstComponent={'script'}
122 + secondComponent={null}
123 + />;
124 +
125 + expect(function() {
126 + ReactTestUtils.renderIntoDocument(instance1);
127 + }).not.toThrow();
128 + expect(function() {
129 + ReactTestUtils.renderIntoDocument(instance2);
130 + }).not.toThrow();
131 +
132 + expect(console.log.argsForCall.length).toBe(4);
133 + expect(console.log.argsForCall[0][0]).toBe(null);
134 + expect(console.log.argsForCall[1][0].tagName).toBe('SCRIPT');
135 + expect(console.log.argsForCall[2][0].tagName).toBe('SCRIPT');
136 + expect(console.log.argsForCall[3][0]).toBe(null);
137 + }
138 + );
139 +
140 + it('should have getDOMNode return null when multiple layers of composite ' +
141 + 'components render to the same null placeholder', () => {
142 + spyOn(console, 'log');
143 +
144 + var GrandChild = React.createClass({
145 + render: function() {
146 + return null;
147 + }
148 + });
149 +
150 + var Child = React.createClass({
151 + render: function() {
152 + return <GrandChild />;
153 + }
154 + });
155 +
156 + var instance1 =
157 + <TogglingComponent
158 + firstComponent={'div'}
159 + secondComponent={Child}
160 + />;
161 + var instance2 =
162 + <TogglingComponent
163 + firstComponent={Child}
164 + secondComponent={'div'}
165 + />;
166 +
167 + expect(function() {
168 + ReactTestUtils.renderIntoDocument(instance1);
169 + }).not.toThrow();
170 + expect(function() {
171 + ReactTestUtils.renderIntoDocument(instance2);
172 + }).not.toThrow();
173 +
174 + expect(console.log.argsForCall.length).toBe(4);
175 + expect(console.log.argsForCall[0][0].tagName).toBe('DIV');
176 + expect(console.log.argsForCall[1][0]).toBe(null);
177 + expect(console.log.argsForCall[2][0]).toBe(null);
178 + expect(console.log.argsForCall[3][0].tagName).toBe('DIV');
179 + }
180 + );
181 +
182 + it('works when switching components', function() {
183 + var assertions = 0;
184 + var Inner = React.createClass({
185 + render: function() {
186 + return <span />;
187 + },
188 + componentDidMount: function() {
189 + // Make sure the DOM node resolves properly even if we're replacing a
190 + // `null` component
191 + expect(this.getDOMNode()).not.toBe(null);
192 + assertions++;
193 + },
194 + componentWillUnmount: function() {
195 + // Even though we're getting replaced by `null`, we haven't been
196 + // replaced yet!
197 + expect(this.getDOMNode()).not.toBe(null);
198 + assertions++;
199 + }
200 + });
201 + var Wrapper = React.createClass({
202 + render: function() {
203 + return this.props.showInner ? <Inner /> : null;
204 + }
205 + });
206 +
207 + var el = document.createElement('div');
208 + var component;
209 +
210 + // Render the <Inner /> component...
211 + component = React.render(<Wrapper showInner={true} />, el);
212 + expect(component.getDOMNode()).not.toBe(null);
213 +
214 + // Switch to null...
215 + component = React.render(<Wrapper showInner={false} />, el);
216 + expect(component.getDOMNode()).toBe(null);
217 +
218 + // ...then switch back.
219 + component = React.render(<Wrapper showInner={true} />, el);
220 + expect(component.getDOMNode()).not.toBe(null);
221 +
222 + expect(assertions).toBe(3);
223 + });
224 +});
225 +
src/test/reactComponentExpect.js
+1
@@ -61,6 +61,7 @@ assign(reactComponentExpect.prototype, {
61 expectRenderedChild: function() {
62 this.toBeCompositeComponent();
63 var child = this._instance._renderedComponent;
64 + // TODO: Hide ReactEmptyComponent instances here?
65 return new reactComponentExpect(child);
66 },
67