From 8181272fe8e4897391335b14b0c9aac3db4bd3c9 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 16 Nov 2014 00:37:23 -0800 Subject: [PATCH 1/5] Drop inaccessible methods These are no longer accessible and the isOwnedBy check is only used in a method that is not accessible. --- src/core/ReactComponent.js | 28 ---------------------------- src/core/ReactOwner.js | 7 ------- 2 files changed, 35 deletions(-) diff --git a/src/core/ReactComponent.js b/src/core/ReactComponent.js index 564c7311cc..896788db70 100644 --- a/src/core/ReactComponent.js +++ b/src/core/ReactComponent.js @@ -416,34 +416,6 @@ var ReactComponent = { mountImageIntoNode(markup, container, shouldReuseMarkup); }, - /** - * Checks if this component is owned by the supplied `owner` component. - * - * @param {ReactComponent} owner Component to check. - * @return {boolean} True if `owners` owns this component. - * @final - * @internal - */ - isOwnedBy: function(owner) { - return this._owner === owner; - }, - - /** - * Gets another component, that shares the same owner as this one, by ref. - * - * @param {string} ref of a sibling Component. - * @return {?ReactComponent} the actual sibling Component. - * @final - * @internal - */ - getSiblingByRef: function(ref) { - var owner = this._owner; - if (!owner || !owner.refs) { - return null; - } - return owner.refs[ref]; - }, - /** * Get the publicly accessible representation of this component - i.e. what * is exposed by refs and renderComponent. Can be null for stateless diff --git a/src/core/ReactOwner.js b/src/core/ReactOwner.js index 22e493493e..45f85b17bb 100644 --- a/src/core/ReactOwner.js +++ b/src/core/ReactOwner.js @@ -126,13 +126,6 @@ var ReactOwner = { * @private */ attachRef: function(ref, component) { - // TODO: Remove this invariant. This is never exposed and cannot be called - // by user code. The unit test is already removed. - invariant( - component.isOwnedBy(this), - 'attachRef(%s, ...): Only a component\'s owner can store a ref to it.', - ref - ); var inst = this.getPublicInstance(); var refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs; refs[ref] = component.getPublicInstance(); From 519ee322ca53f03a4b9ed6150295340b59588dd6 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 16 Nov 2014 01:44:30 -0800 Subject: [PATCH 2/5] Drop this.props --- src/browser/ui/ReactDOMComponent.js | 16 +++++++++------- .../ui/__tests__/ReactDOMComponent-test.js | 4 ++-- src/core/ReactComponent.js | 5 ----- 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/browser/ui/ReactDOMComponent.js b/src/browser/ui/ReactDOMComponent.js index 862e1af6a1..3bdae4cb36 100644 --- a/src/browser/ui/ReactDOMComponent.js +++ b/src/browser/ui/ReactDOMComponent.js @@ -173,8 +173,8 @@ ReactDOMComponent.Mixin = { transaction, mountDepth ); - assertValidProps(this.props); this._previousStyleCopy = null; + assertValidProps(this._currentElement.props); var closeTag = omittedCloseTags[this._tag] ? '' : ''; return ( this._createOpenTagMarkupAndPutListeners(transaction) + @@ -197,7 +197,7 @@ ReactDOMComponent.Mixin = { * @return {string} Markup of opening tag. */ _createOpenTagMarkupAndPutListeners: function(transaction) { - var props = this.props; + var props = this._currentElement.props; var ret = '<' + this._tag; for (var propKey in props) { @@ -253,16 +253,18 @@ ReactDOMComponent.Mixin = { prefix = '\n'; } + var props = this._currentElement.props; + // Intentional use of != to avoid catching zero/false. - var innerHTML = this.props.dangerouslySetInnerHTML; + var innerHTML = props.dangerouslySetInnerHTML; if (innerHTML != null) { if (innerHTML.__html != null) { return prefix + innerHTML.__html; } } else { var contentToUse = - CONTENT_TYPES[typeof this.props.children] ? this.props.children : null; - var childrenToUse = contentToUse != null ? null : this.props.children; + CONTENT_TYPES[typeof props.children] ? props.children : null; + var childrenToUse = contentToUse != null ? null : props.children; if (contentToUse != null) { return prefix + escapeTextForBrowser(contentToUse); } else if (childrenToUse != null) { @@ -338,7 +340,7 @@ ReactDOMComponent.Mixin = { * @param {ReactReconcileTransaction} transaction */ _updateDOMProperties: function(lastProps, transaction) { - var nextProps = this.props; + var nextProps = this._currentElement.props; var propKey; var styleName; var styleUpdates; @@ -427,7 +429,7 @@ ReactDOMComponent.Mixin = { * @param {ReactReconcileTransaction} transaction */ _updateDOMChildren: function(lastProps, transaction) { - var nextProps = this.props; + var nextProps = this._currentElement.props; var lastContent = CONTENT_TYPES[typeof lastProps.children] ? lastProps.children : null; diff --git a/src/browser/ui/__tests__/ReactDOMComponent-test.js b/src/browser/ui/__tests__/ReactDOMComponent-test.js index 3ab38722bf..58216c236e 100644 --- a/src/browser/ui/__tests__/ReactDOMComponent-test.js +++ b/src/browser/ui/__tests__/ReactDOMComponent-test.js @@ -226,7 +226,7 @@ describe('ReactDOMComponent', function() { var ReactReconcileTransaction = require('ReactReconcileTransaction'); var NodeStub = function(initialProps) { - this.props = initialProps || {}; + this._currentElement = { props: initialProps }; this._rootNodeID = 'test'; }; assign(NodeStub.prototype, ReactDOMComponent.Mixin); @@ -276,7 +276,7 @@ describe('ReactDOMComponent', function() { var ReactReconcileTransaction = require('ReactReconcileTransaction'); var NodeStub = function(initialProps) { - this.props = initialProps || {}; + this._currentElement = { props: initialProps }; this._rootNodeID = 'test'; }; assign(NodeStub.prototype, ReactDOMComponent.Mixin); diff --git a/src/core/ReactComponent.js b/src/core/ReactComponent.js index 896788db70..0e16b5b98c 100644 --- a/src/core/ReactComponent.js +++ b/src/core/ReactComponent.js @@ -225,10 +225,6 @@ var ReactComponent = { * @internal */ construct: function(element) { - // This is the public exposed props object after it has been processed - // with default props. The element's props represents the true internal - // state of the props. - this.props = element.props; // Record the component responsible for creating this component. // This is accessible through the element but we maintain an extra // field for compatibility with devtools and as a way to make an @@ -337,7 +333,6 @@ var ReactComponent = { var prevElement = this._currentElement; var nextElement = this._pendingElement; this._currentElement = nextElement; - this.props = nextElement.props; this._owner = nextElement._owner; this._pendingElement = null; this.updateComponent(transaction, prevElement, nextElement); From 03ae0a906b8cf168361113a2afe56f482fe4de66 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 16 Nov 2014 01:54:51 -0800 Subject: [PATCH 3/5] Drop ReactOwner.Mixin This is not used anywhere else. To avoid overabstraction we should just inline this. --- src/core/ReactCompositeComponent.js | 33 ++++++++++++++++++++--- src/core/ReactOwner.js | 41 ----------------------------- 2 files changed, 29 insertions(+), 45 deletions(-) diff --git a/src/core/ReactCompositeComponent.js b/src/core/ReactCompositeComponent.js index 7d5e66e5fe..58bc4475b4 100644 --- a/src/core/ReactCompositeComponent.js +++ b/src/core/ReactCompositeComponent.js @@ -16,12 +16,12 @@ var ReactContext = require('ReactContext'); var ReactCurrentOwner = require('ReactCurrentOwner'); var ReactElement = require('ReactElement'); var ReactInstanceMap = require('ReactInstanceMap'); -var ReactOwner = require('ReactOwner'); var ReactPerf = require('ReactPerf'); var ReactPropTypeLocations = require('ReactPropTypeLocations'); var ReactUpdates = require('ReactUpdates'); var assign = require('Object.assign'); +var emptyObject = require('emptyObject'); var invariant = require('invariant'); var keyMirror = require('keyMirror'); var shouldUpdateReactComponent = require('shouldUpdateReactComponent'); @@ -100,8 +100,7 @@ var CompositeLifeCycle = keyMirror({ * @lends {ReactCompositeComponent.prototype} */ var ReactCompositeComponentMixin = assign({}, - ReactComponent.Mixin, - ReactOwner.Mixin, { + ReactComponent.Mixin, { /** * Base constructor for all composite component. @@ -114,13 +113,13 @@ var ReactCompositeComponentMixin = assign({}, this._instance.props = element.props; this._instance.state = null; this._instance.context = null; + this._instance.refs = emptyObject; this._pendingState = null; this._compositeLifeCycleState = null; // Children can be either an array or more than one argument ReactComponent.Mixin.construct.apply(this, arguments); - ReactOwner.Mixin.construct.apply(this, arguments); }, /** @@ -699,6 +698,32 @@ var ReactCompositeComponentMixin = assign({}, } ), + /** + * Lazily allocates the refs object and stores `component` as `ref`. + * + * @param {string} ref Reference name. + * @param {component} component Component to store as `ref`. + * @final + * @private + */ + attachRef: function(ref, component) { + var inst = this.getPublicInstance(); + var refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs; + refs[ref] = component.getPublicInstance(); + }, + + /** + * Detaches a reference name. + * + * @param {string} ref Name to dereference. + * @final + * @private + */ + detachRef: function(ref) { + var refs = this.getPublicInstance().refs; + delete refs[ref]; + }, + /** * Get the publicly accessible representation of this component - i.e. what * is exposed by refs and renderComponent. Can be null for stateless diff --git a/src/core/ReactOwner.js b/src/core/ReactOwner.js index 45f85b17bb..29da2705a9 100644 --- a/src/core/ReactOwner.js +++ b/src/core/ReactOwner.js @@ -11,7 +11,6 @@ "use strict"; -var emptyObject = require('emptyObject'); var invariant = require('invariant'); /** @@ -103,46 +102,6 @@ var ReactOwner = { if (owner.getPublicInstance().refs[ref] === component.getPublicInstance()) { owner.detachRef(ref); } - }, - - /** - * A ReactComponent must mix this in to have refs. - * - * @lends {ReactOwner.prototype} - */ - Mixin: { - - construct: function() { - var inst = this.getPublicInstance(); - inst.refs = emptyObject; - }, - - /** - * Lazily allocates the refs object and stores `component` as `ref`. - * - * @param {string} ref Reference name. - * @param {component} component Component to store as `ref`. - * @final - * @private - */ - attachRef: function(ref, component) { - var inst = this.getPublicInstance(); - var refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs; - refs[ref] = component.getPublicInstance(); - }, - - /** - * Detaches a reference name. - * - * @param {string} ref Name to dereference. - * @final - * @private - */ - detachRef: function(ref) { - var refs = this.getPublicInstance().refs; - delete refs[ref]; - } - } }; From dac59d10323e35b52d379a1ae7e9815cc5a55aa5 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 16 Nov 2014 16:58:45 -0800 Subject: [PATCH 4/5] Drop the _owner and _lifeCycle field on internal instances The _owner field is unnecessary since it's reachable from _currentElement. The _lifeCycle field is unnecessary because an internal component should not even need to exist at all if it's unmounted. It should be dereferenced internally, and never exposed externally. The only case where it's important is for batching updates where we currently avoid calling performUpdateIfNecessary if it's mounted. However, this function is already only executed "if necessary" so we just make sure that it's not necessary after unmount by resetting all the pending fields. --- src/core/ReactComponent.js | 67 ++----------------- src/core/ReactCompositeComponent.js | 21 ++---- src/core/ReactUpdates.js | 31 +++++---- .../__tests__/ReactComponentLifeCycle-test.js | 31 ++++++--- src/core/__tests__/ReactUpdates-test.js | 42 ++++++++++++ src/test/ReactDefaultPerf.js | 3 +- 6 files changed, 92 insertions(+), 103 deletions(-) diff --git a/src/core/ReactComponent.js b/src/core/ReactComponent.js index 0e16b5b98c..39f7381ccb 100644 --- a/src/core/ReactComponent.js +++ b/src/core/ReactComponent.js @@ -20,21 +20,6 @@ var assign = require('Object.assign'); var invariant = require('invariant'); var keyMirror = require('keyMirror'); -/** - * Every React component is in one of these life cycles. - */ -var ComponentLifeCycle = keyMirror({ - /** - * Mounted components have a DOM node representation and are capable of - * receiving new props. - */ - MOUNTED: null, - /** - * Unmounted components are inactive and cannot receive new props. - */ - UNMOUNTED: null -}); - var injected = false; /** @@ -115,11 +100,6 @@ var ReactComponent = { } }, - /** - * @internal - */ - LifeCycle: ComponentLifeCycle, - /** * Injected module that provides ability to mutate individual properties. * Injected into the base class because many different subclasses need access @@ -137,17 +117,6 @@ var ReactComponent = { */ Mixin: { - /** - * Checks whether or not this component is mounted. - * - * @return {boolean} True if mounted, false otherwise. - * @final - * @protected - */ - isMounted: function() { - return this._lifeCycleState === ComponentLifeCycle.MOUNTED; - }, - /** * Sets a subset of the props. * @@ -175,10 +144,6 @@ var ReactComponent = { * @public */ replaceProps: function(props, callback) { - invariant( - this.isMounted(), - 'replaceProps(...): Can only update a mounted component.' - ); invariant( this._mountDepth === 0, 'replaceProps(...): You called `setProps` or `replaceProps` on a ' + @@ -225,15 +190,6 @@ var ReactComponent = { * @internal */ construct: function(element) { - // Record the component responsible for creating this component. - // This is accessible through the element but we maintain an extra - // field for compatibility with devtools and as a way to make an - // incremental update. TODO: Consider deprecating this field. - this._owner = element._owner; - - // All components start unmounted. - this._lifeCycleState = ComponentLifeCycle.UNMOUNTED; - // See ReactUpdates. this._pendingCallbacks = null; @@ -258,20 +214,12 @@ var ReactComponent = { * @internal */ mountComponent: function(rootID, transaction, mountDepth) { - invariant( - !this.isMounted(), - 'mountComponent(%s, ...): Can only mount an unmounted component. ' + - 'Make sure to avoid storing components between renders or reusing a ' + - 'single component instance in multiple places.', - rootID - ); var ref = this._currentElement.ref; if (ref != null) { var owner = this._currentElement._owner; attachRef(ref, this, owner); } this._rootNodeID = rootID; - this._lifeCycleState = ComponentLifeCycle.MOUNTED; this._mountDepth = mountDepth; // Effectively: return ''; }, @@ -287,17 +235,15 @@ var ReactComponent = { * @internal */ unmountComponent: function() { - invariant( - this.isMounted(), - 'unmountComponent(): Can only unmount a mounted component.' - ); var ref = this._currentElement.ref; if (ref != null) { - detachRef(ref, this, this._owner); + detachRef(ref, this, this._currentElement._owner); } unmountIDFromEnvironment(this._rootNodeID); + // Reset all fields this._rootNodeID = null; - this._lifeCycleState = ComponentLifeCycle.UNMOUNTED; + this._pendingCallbacks = null; + this._pendingElement = null; }, /** @@ -312,10 +258,6 @@ var ReactComponent = { * @internal */ receiveComponent: function(nextElement, transaction) { - invariant( - this.isMounted(), - 'receiveComponent(...): Can only update a mounted component.' - ); this._pendingElement = nextElement; this.performUpdateIfNecessary(transaction); }, @@ -333,7 +275,6 @@ var ReactComponent = { var prevElement = this._currentElement; var nextElement = this._pendingElement; this._currentElement = nextElement; - this._owner = nextElement._owner; this._pendingElement = null; this.updateComponent(transaction, prevElement, nextElement); }, diff --git a/src/core/ReactCompositeComponent.js b/src/core/ReactCompositeComponent.js index 58bc4475b4..b0284cc7ae 100644 --- a/src/core/ReactCompositeComponent.js +++ b/src/core/ReactCompositeComponent.js @@ -28,7 +28,7 @@ var shouldUpdateReactComponent = require('shouldUpdateReactComponent'); var warning = require('warning'); function getDeclarationErrorAddendum(component) { - var owner = component._owner || null; + var owner = component._currentElement._owner || null; if (owner) { var constructor = owner._instance.constructor; if (constructor && constructor.displayName) { @@ -56,8 +56,8 @@ function validateLifeCycleOnReplaceState(instance) { * `ReactCompositeComponent` maintains an auxiliary life cycle state in * `this._compositeLifeCycleState` (which can be null). * - * This is different from the life cycle state maintained by `ReactComponent` in - * `this._lifeCycleState`. The following diagram shows how the states overlap in + * This is different from the life cycle state maintained by `ReactComponent`. + * The following diagram shows how the states overlap in * time. There are times when the CompositeLifeCycle is null - at those times it * is only meaningful to look at ComponentLifeCycle alone. * @@ -129,8 +129,7 @@ var ReactCompositeComponentMixin = assign({}, * @final */ isMounted: function() { - return ReactComponent.Mixin.isMounted.call(this) && - this._compositeLifeCycleState !== CompositeLifeCycle.MOUNTING; + return this._compositeLifeCycleState !== CompositeLifeCycle.MOUNTING; }, /** @@ -232,6 +231,9 @@ var ReactCompositeComponentMixin = assign({}, this._renderedComponent.unmountComponent(); this._renderedComponent = null; + // Reset pending fields + this._pendingState = null; + this._pendingForceUpdate = false; ReactComponent.Mixin.unmountComponent.call(this); // Delete the reference from the instance to this internal representation @@ -554,11 +556,6 @@ var ReactCompositeComponentMixin = assign({}, inst.props = nextProps; inst.state = nextState; inst.context = nextContext; - - // Owner cannot change because shouldUpdateReactComponent doesn't allow - // it. TODO: Remove this._owner completely. - this._owner = nextParentElement._owner; - return; } @@ -606,10 +603,6 @@ var ReactCompositeComponentMixin = assign({}, inst.state = nextState; inst.context = nextContext; - // Owner cannot change because shouldUpdateReactComponent doesn't allow - // it. TODO: Remove this._owner completely. - this._owner = nextElement._owner; - this._updateRenderedComponent(transaction); if (inst.componentDidUpdate) { diff --git a/src/core/ReactUpdates.js b/src/core/ReactUpdates.js index 7486e3ac67..d9d6f8cc45 100644 --- a/src/core/ReactUpdates.js +++ b/src/core/ReactUpdates.js @@ -136,24 +136,23 @@ function runBatchedUpdates(transaction) { dirtyComponents.sort(mountDepthComparator); for (var i = 0; i < len; i++) { - // If a component is unmounted before pending changes apply, ignore them - // TODO: Queue unmounts in the same list to avoid this happening at all + // If a component is unmounted before pending changes apply, it will still + // be here, but we assume that it has cleared its _pendingCallbacks and + // that performUpdateIfNecessary is a noop. var component = dirtyComponents[i]; - if (component.isMounted()) { - // If performUpdateIfNecessary happens to enqueue any new updates, we - // shouldn't execute the callbacks until the next render happens, so - // stash the callbacks first - var callbacks = component._pendingCallbacks; - component._pendingCallbacks = null; - component.performUpdateIfNecessary(transaction.reconcileTransaction); + // If performUpdateIfNecessary happens to enqueue any new updates, we + // shouldn't execute the callbacks until the next render happens, so + // stash the callbacks first + var callbacks = component._pendingCallbacks; + component._pendingCallbacks = null; + component.performUpdateIfNecessary(transaction.reconcileTransaction); - if (callbacks) { - for (var j = 0; j < callbacks.length; j++) { - transaction.callbackQueue.enqueue( - callbacks[j], - component - ); - } + if (callbacks) { + for (var j = 0; j < callbacks.length; j++) { + transaction.callbackQueue.enqueue( + callbacks[j], + component + ); } } } diff --git a/src/core/__tests__/ReactComponentLifeCycle-test.js b/src/core/__tests__/ReactComponentLifeCycle-test.js index 214b18d017..8f145924fe 100644 --- a/src/core/__tests__/ReactComponentLifeCycle-test.js +++ b/src/core/__tests__/ReactComponentLifeCycle-test.js @@ -11,12 +11,13 @@ "use strict"; +var keyMirror = require('keyMirror'); + var React; var ReactTestUtils; var ReactComponent; var ReactCompositeComponent; var ReactInstanceMap; -var ComponentLifeCycle; var CompositeComponentLifeCycle; var getCompositeLifeCycle; @@ -69,6 +70,21 @@ var POST_WILL_UNMOUNT_STATE = { hasWillUnmountCompleted: true }; +/** + * Every React component is in one of these life cycles. + */ +var ComponentLifeCycle = keyMirror({ + /** + * Mounted components have a DOM node representation and are capable of + * receiving new props. + */ + MOUNTED: null, + /** + * Unmounted components are inactive and cannot receive new props. + */ + UNMOUNTED: null +}); + /** * TODO: We should make any setState calls fail in * `getInitialState` and `componentWillMount`. They will usually fail @@ -83,7 +99,6 @@ describe('ReactComponentLifeCycle', function() { ReactTestUtils = require('ReactTestUtils'); ReactComponent = require('ReactComponent'); ReactCompositeComponent = require('ReactCompositeComponent'); - ComponentLifeCycle = ReactComponent.LifeCycle; CompositeComponentLifeCycle = ReactCompositeComponent.LifeCycle; ReactInstanceMap = require('ReactInstanceMap'); @@ -94,13 +109,11 @@ describe('ReactComponentLifeCycle', function() { getLifeCycleState = function(instance) { var internalInstance = ReactInstanceMap.get(instance); - if (!internalInstance) { - // Once a component is fully unmounted, we cannot actually get to the - // internal instance. It's already dereferenced and possibly GC:ed. - // So the unmounted life cycle hook doesn't exist anymore. - return ComponentLifeCycle.UNMOUNTED; - } - return internalInstance._lifeCycleState; + // Once a component gets mounted, it has an internal instance, once it + // gets unmounted, it loses that internal instance. + return internalInstance ? + ComponentLifeCycle.MOUNTED : + ComponentLifeCycle.UNMOUNTED; }; }); diff --git a/src/core/__tests__/ReactUpdates-test.js b/src/core/__tests__/ReactUpdates-test.js index 5774560007..816ec2e053 100644 --- a/src/core/__tests__/ReactUpdates-test.js +++ b/src/core/__tests__/ReactUpdates-test.js @@ -815,4 +815,46 @@ describe('ReactUpdates', function() { 'asap-1.2' ]); }); + + it('does not call render after a component as been deleted', function() { + + var renderCount = 0; + var componentB = null; + + var B = React.createClass({ + getInitialState: function() { + return { updates: 0 }; + }, + componentDidMount: function() { + componentB = this; + }, + render: function() { + renderCount++; + return
; + } + }); + + var A = React.createClass({ + getInitialState: function() { + return { showB: true }; + }, + render: function() { + return this.state.showB ? :
; + } + }); + + var container = document.createElement('div'); + + var component = React.render(, container); + + ReactUpdates.batchedUpdates(function() { + // B will have scheduled an update but the batching should ensure that its + // update never fires. + componentB.setState({ updates: 1 }); + component.setState({ showB: false }); + }); + + expect(renderCount).toBe(1); + }); + }); diff --git a/src/test/ReactDefaultPerf.js b/src/test/ReactDefaultPerf.js index ce10bc6a52..d91a61d70e 100644 --- a/src/test/ReactDefaultPerf.js +++ b/src/test/ReactDefaultPerf.js @@ -244,7 +244,8 @@ var ReactDefaultPerf = { entry.displayNames[rootNodeID] = { current: this.constructor.displayName, - owner: this._owner ? this._owner.constructor.displayName : '' + owner: this._currentElement._owner ? + this._currentElement._owner.constructor.displayName : '' }; return rv; From 63644d5e71356d422603c0faef6b664c1bdb1f42 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 16 Nov 2014 16:59:47 -0800 Subject: [PATCH 5/5] Add test for warning in ReactElementValidator I previously had a mistake here, so I'm adding a unit test to ensure that I don't make the same mistake again. --- src/core/ReactElementValidator.js | 2 +- src/core/__tests__/ReactElement-test.js | 30 +++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/core/ReactElementValidator.js b/src/core/ReactElementValidator.js index e93dae340f..51866856dd 100644 --- a/src/core/ReactElementValidator.js +++ b/src/core/ReactElementValidator.js @@ -160,7 +160,7 @@ function monitorUseOfObjectMap() { } /** - * Ensure that every component either is passed in a static location, in an + * Ensure that every element either is passed in a static location, in an * array with an explicit keys property defined, or in an object literal * with valid key property. * diff --git a/src/core/__tests__/ReactElement-test.js b/src/core/__tests__/ReactElement-test.js index 38772a90e9..1a8f530838 100644 --- a/src/core/__tests__/ReactElement-test.js +++ b/src/core/__tests__/ReactElement-test.js @@ -173,6 +173,36 @@ describe('ReactElement', function() { ); }); + it('warns for keys for arrays of elements in rest args', function() { + spyOn(console, 'warn'); + var Component = React.createFactory(ComponentClass); + + var InnerClass = React.createClass({ + displayName: 'InnerClass', + render: function() { + return Component(null, this.props.childSet); + } + }); + + var InnerComponent = React.createFactory(InnerClass); + + var ComponentWrapper = React.createClass({ + displayName: 'ComponentWrapper', + render: function() { + return InnerComponent({ childSet: [ Component(), Component() ] }); + } + }); + + ReactTestUtils.renderIntoDocument(); + + expect(console.warn.argsForCall.length).toBe(1); + expect(console.warn.argsForCall[0][0]).toContain( + 'Each child in an array or iterator should have a unique "key" prop. ' + + 'Check the render method of InnerClass. ' + + 'It was passed a child from ComponentWrapper. ' + ); + }); + it('warns for keys for iterables of elements in rest args', function() { spyOn(console, 'warn'); var Component = React.createFactory(ComponentClass);