diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index ae1b00f2df5a3..36f8480baf36f 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -66,7 +66,6 @@ src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js * should warn when stateless component returns array -* should throw on string refs in pure functions * should warn when using non-React functions in JSX src/renderers/shared/shared/__tests__/ReactUpdates-test.js diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index df708821eb6f7..decc542712916 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -116,4 +116,3 @@ src/renderers/shared/shared/__tests__/ReactMultiChild-test.js src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js * should warn for childContextTypes on a functional component -* should warn when given a ref diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 761b7f8b65720..e8d3b4ae7551e 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1535,6 +1535,9 @@ src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js * should update stateless component * should unmount stateless component * should pass context thru stateless component +* should throw on string refs in pure functions +* should warn when given a string ref +* should warn when given a function ref * should provide a null ref * should use correct name in key warning * should support default props and prop types diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 028490ee83cd6..9302c2ed809be 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -78,14 +78,16 @@ function coerceRef(current: ?Fiber, element: ReactElement) { let mixedRef = element.ref; if (mixedRef != null && typeof mixedRef !== 'function') { if (element._owner) { - const ownerFiber : ?(Fiber | ReactInstance) = (element._owner : any); + const owner : ?(Fiber | ReactInstance) = (element._owner : any); let inst; - if (ownerFiber) { - if ((ownerFiber : any).tag === ClassComponent) { - inst = (ownerFiber : any).stateNode; + if (owner) { + if (typeof owner.tag === 'number') { + const ownerFiber = ((owner : any) : Fiber); + invariant(ownerFiber.tag === ClassComponent, 'Stateless function components cannot have refs.'); + inst = ownerFiber.stateNode; } else { // Stack - inst = (ownerFiber : any).getPublicInstance(); + inst = (owner : any).getPublicInstance(); } } invariant(inst, 'Missing owner for string ref %s', mixedRef); diff --git a/src/renderers/shared/fiber/ReactDebugCurrentFiber.js b/src/renderers/shared/fiber/ReactDebugCurrentFiber.js index 881795cd1db73..c1ce29ea1c472 100644 --- a/src/renderers/shared/fiber/ReactDebugCurrentFiber.js +++ b/src/renderers/shared/fiber/ReactDebugCurrentFiber.js @@ -15,12 +15,6 @@ import type { Fiber } from 'ReactFiber'; if (__DEV__) { - var { - IndeterminateComponent, - FunctionalComponent, - ClassComponent, - HostComponent, - } = require('ReactTypeOfWork'); var getComponentName = require('getComponentName'); var { getStackAddendumByWorkInProgressFiber } = require('ReactComponentTreeHook'); } @@ -31,18 +25,8 @@ function getCurrentFiberOwnerName() : string | null { if (fiber == null) { return null; } - switch (fiber.tag) { - case IndeterminateComponent: - case FunctionalComponent: - case ClassComponent: - return getComponentName(fiber); - case HostComponent: - if (fiber._debugOwner != null) { - return getComponentName(fiber._debugOwner); - } - return null; - default: - return null; + if (fiber._debugOwner != null) { + return getComponentName(fiber._debugOwner); } } return null; diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index ea0b88bb60e06..4a0f333d4af21 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/src/renderers/shared/fiber/ReactFiber.js @@ -50,7 +50,7 @@ var { var invariant = require('invariant'); if (__DEV__) { - var { getCurrentFiberOwnerName } = require('ReactDebugCurrentFiber'); + var getComponentName = require('getComponentName'); } // A Fiber is work on a Component that needs to be done or was done. There can @@ -300,7 +300,12 @@ exports.createHostRootFiber = function() : Fiber { }; exports.createFiberFromElement = function(element : ReactElement, priorityLevel : PriorityLevel) : Fiber { - const fiber = createFiberFromElementType(element.type, element.key); + let owner = null; + if (__DEV__) { + owner = element._owner; + } + + const fiber = createFiberFromElementType(element.type, element.key, owner); fiber.pendingProps = element.props; fiber.pendingWorkPriority = priorityLevel; @@ -328,7 +333,7 @@ exports.createFiberFromText = function(content : string, priorityLevel : Priorit return fiber; }; -function createFiberFromElementType(type : mixed, key : null | string) : Fiber { +function createFiberFromElementType(type : mixed, key : null | string, debugOwner : null | Fiber | ReactInstance) : Fiber { let fiber; if (typeof type === 'function') { fiber = shouldConstruct(type) ? @@ -363,7 +368,7 @@ function createFiberFromElementType(type : mixed, key : null | string) : Fiber { ' You likely forgot to export your component from the file ' + 'it\'s defined in.'; } - const ownerName = getCurrentFiberOwnerName(); + const ownerName = debugOwner ? getComponentName(debugOwner) : null; if (ownerName) { info += ' Check the render method of `' + ownerName + '`.'; } diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index 2b438657e4c43..db863f1783a70 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -63,6 +63,7 @@ var { } = require('ReactTypeOfSideEffect'); var ReactCurrentOwner = require('ReactCurrentOwner'); var ReactFiberClassComponent = require('ReactFiberClassComponent'); +var warning = require('warning'); if (__DEV__) { var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); @@ -455,6 +456,23 @@ module.exports = function( } else { // Proceed under the assumption that this is a functional component workInProgress.tag = FunctionalComponent; + if (__DEV__) { + if (workInProgress.ref != null) { + let info = ''; + const ownerName = ReactDebugCurrentFiber.getCurrentFiberOwnerName(); + if (ownerName) { + info += ' Check the render method of `' + ownerName + '`.'; + } + + warning( + false, + 'Stateless function components cannot be given refs. ' + + 'Attempts to access this ref will fail.%s%s', + info, + ReactDebugCurrentFiber.getCurrentFiberStackAddendum() + ); + } + } reconcileChildren(current, workInProgress, value); return workInProgress.child; } diff --git a/src/renderers/shared/fiber/ReactReifiedYield.js b/src/renderers/shared/fiber/ReactReifiedYield.js index 4fec70e180ce8..1cb899c15ec59 100644 --- a/src/renderers/shared/fiber/ReactReifiedYield.js +++ b/src/renderers/shared/fiber/ReactReifiedYield.js @@ -22,7 +22,8 @@ export type ReifiedYield = { continuation: Fiber, props: Object }; exports.createReifiedYield = function(yieldNode : ReactYield) : ReifiedYield { var fiber = createFiberFromElementType( yieldNode.continuation, - yieldNode.key + yieldNode.key, + null // debugOwner ); return { continuation: fiber, @@ -35,7 +36,8 @@ exports.createUpdatedReifiedYield = function(previousYield : ReifiedYield, yield if (fiber.type !== yieldNode.continuation) { fiber = createFiberFromElementType( yieldNode.continuation, - yieldNode.key + yieldNode.key, + null // debugOwner ); } return { diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js index c22d644fddd7f..bee26adef8f98 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js @@ -739,9 +739,8 @@ describe('ReactIncrementalErrorHandling', () => { } } const InvalidType = undefined; - const brokenElement = ; function BrokenRender(props) { - return brokenElement; + return ; } ReactNoop.render( @@ -776,9 +775,8 @@ describe('ReactIncrementalErrorHandling', () => { } const InvalidType = undefined; - const brokenElement = ; function BrokenRender(props) { - return props.fail ? brokenElement : ; + return props.fail ? : ; } ReactNoop.render( diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js index 2666eecca7e50..15f3d6f5df593 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js @@ -21,6 +21,10 @@ describe('ReactIncrementalSideEffects', () => { ReactNoop = require('ReactNoop'); }); + function normalizeCodeLocInfo(str) { + return str && str.replace(/\(at .+?:\d+\)/g, '(at **)'); + } + function div(...children) { children = children.map(c => typeof c === 'string' ? { text: c } : c); return { type: 'div', children, prop: undefined }; @@ -1069,7 +1073,7 @@ describe('ReactIncrementalSideEffects', () => { }); it('invokes ref callbacks after insertion/update/unmount', () => { - + spyOn(console, 'error'); var classInstance = null; var ops = []; @@ -1129,6 +1133,14 @@ describe('ReactIncrementalSideEffects', () => { null, ]); + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( + 'Warning: Stateless function components cannot be given refs. ' + + 'Attempts to access this ref will fail. Check the render method ' + + 'of `Foo`.\n' + + ' in FunctionalComponent (at **)\n' + + ' in div (at **)\n' + + ' in Foo (at **)' + ); }); // TODO: Test that mounts, updates, refs, unmounts and deletions happen in the diff --git a/src/renderers/shared/shared/__tests__/ReactComponent-test.js b/src/renderers/shared/shared/__tests__/ReactComponent-test.js index 1195070857744..5c2ba86a41d5e 100644 --- a/src/renderers/shared/shared/__tests__/ReactComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactComponent-test.js @@ -352,16 +352,25 @@ describe('ReactComponent', () => { it('includes owner name in the error about badly-typed elements', () => { spyOn(console, 'error'); + var X = undefined; + + function Indirection(props) { + return
{props.children}
; + } + + function Bar() { + return ; + } + function Foo() { - var X = undefined; - return ; + return ; } expect(() => ReactTestUtils.renderIntoDocument()).toThrowError( 'Element type is invalid: expected a string (for built-in components) ' + 'or a class/function (for composite components) but got: undefined. ' + 'You likely forgot to export your component from the file it\'s ' + - 'defined in. Check the render method of `Foo`.' + 'defined in. Check the render method of `Bar`.' ); // One warning for each element creation diff --git a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js index 8577ca180e688..ec63bbd70b4d9 100644 --- a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js @@ -20,6 +20,9 @@ function StatelessComponent(props) { } describe('ReactStatelessComponent', () => { + function normalizeCodeLocInfo(str) { + return str && str.replace(/\(at .+?:\d+\)/g, '(at **)'); + } beforeEach(() => { React = require('React'); @@ -146,24 +149,60 @@ describe('ReactStatelessComponent', () => { ); }); - it('should warn when given a ref', () => { + it('should warn when given a string ref', () => { spyOn(console, 'error'); + function Indirection(props) { + return
{props.children}
; + } + class Parent extends React.Component { - static displayName = 'Parent'; + render() { + return ; + } + } + ReactTestUtils.renderIntoDocument(); + + expectDev(console.error.calls.count()).toBe(1); + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( + 'Warning: Stateless function components cannot be given refs. ' + + 'Attempts to access this ref will fail. Check the render method ' + + 'of `Parent`.\n' + + ' in StatelessComponent (at **)\n' + + ' in div (at **)\n' + + ' in Indirection (at **)\n' + + ' in Parent (at **)' + ); + }); + + it('should warn when given a function ref', () => { + spyOn(console, 'error'); + var ref = jasmine.createSpy().and.callFake((arg) => { + expect(arg).toBe(null); + }); + + function Indirection(props) { + return
{props.children}
; + } + + class Parent extends React.Component { render() { - return ; + return ; } } ReactTestUtils.renderIntoDocument(); expectDev(console.error.calls.count()).toBe(1); - expectDev(console.error.calls.argsFor(0)[0]).toContain( - 'Stateless function components cannot be given refs ' + - '(See ref "stateless" in StatelessComponent created by Parent). ' + - 'Attempts to access this ref will fail.' + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( + 'Warning: Stateless function components cannot be given refs. ' + + 'Attempts to access this ref will fail. Check the render method ' + + 'of `Parent`.\n' + + ' in StatelessComponent (at **)\n' + + ' in div (at **)\n' + + ' in Indirection (at **)\n' + + ' in Parent (at **)' ); }); diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index 79baf74489a9c..1abe88d62246a 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -13,6 +13,7 @@ var React = require('React'); var ReactComponentEnvironment = require('ReactComponentEnvironment'); +var ReactCompositeComponentTypes = require('ReactCompositeComponentTypes'); var ReactCurrentOwner = require('ReactCurrentOwner'); var ReactErrorUtils = require('ReactErrorUtils'); var ReactFeatureFlags = require('ReactFeatureFlags'); @@ -33,12 +34,6 @@ var warning = require('warning'); import type { ReactPropTypeLocations } from 'ReactPropTypeLocations'; -var CompositeTypes = { - ImpureClass: 0, - PureClass: 1, - StatelessFunctional: 2, -}; - function StatelessComponent(Component) { } StatelessComponent.prototype.render = function() { @@ -220,12 +215,12 @@ var ReactCompositeComponent = { Component.displayName || Component.name || 'Component' ); inst = new StatelessComponent(Component); - this._compositeType = CompositeTypes.StatelessFunctional; + this._compositeType = ReactCompositeComponentTypes.StatelessFunctional; } else { if (isPureComponent(Component)) { - this._compositeType = CompositeTypes.PureClass; + this._compositeType = ReactCompositeComponentTypes.PureClass; } else { - this._compositeType = CompositeTypes.ImpureClass; + this._compositeType = ReactCompositeComponentTypes.ImpureClass; } } @@ -881,7 +876,7 @@ var ReactCompositeComponent = { shouldUpdate = inst.shouldComponentUpdate(nextProps, nextState, nextContext); } } else { - if (this._compositeType === CompositeTypes.PureClass) { + if (this._compositeType === ReactCompositeComponentTypes.PureClass) { shouldUpdate = !shallowEqual(prevProps, nextProps) || !shallowEqual(inst.state, nextState); @@ -1215,7 +1210,7 @@ var ReactCompositeComponent = { */ _renderValidatedComponent: function() { var renderedElement; - if (__DEV__ || this._compositeType !== CompositeTypes.StatelessFunctional) { + if (__DEV__ || this._compositeType !== ReactCompositeComponentTypes.StatelessFunctional) { ReactCurrentOwner.current = this; try { renderedElement = @@ -1251,20 +1246,6 @@ var ReactCompositeComponent = { var inst = this.getPublicInstance(); invariant(inst != null, 'Stateless function components cannot have refs.'); var publicComponentInstance = component.getPublicInstance(); - if (__DEV__) { - var componentName = component && component.getName ? - component.getName() : 'a component'; - warning( - publicComponentInstance != null || - component._compositeType !== CompositeTypes.StatelessFunctional, - 'Stateless function components cannot be given refs ' + - '(See ref "%s" in %s created by %s). ' + - 'Attempts to access this ref will fail.', - ref, - componentName, - this.getName() - ); - } var refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs; refs[ref] = publicComponentInstance; }, @@ -1307,7 +1288,7 @@ var ReactCompositeComponent = { */ getPublicInstance: function() { var inst = this._instance; - if (this._compositeType === CompositeTypes.StatelessFunctional) { + if (this._compositeType === ReactCompositeComponentTypes.StatelessFunctional) { return null; } return inst; diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponentTypes.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponentTypes.js new file mode 100644 index 0000000000000..e6f0825028898 --- /dev/null +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponentTypes.js @@ -0,0 +1,19 @@ +/** + * Copyright 2013-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + * @providesModule ReactCompositeComponentTypes + * @flow + */ + +export type CompositeComponentTypes = 0 | 1 | 2; + +module.exports = { + ImpureClass: 0, + PureClass: 1, + StatelessFunctional: 2, +}; diff --git a/src/renderers/shared/stack/reconciler/ReactInstanceType.js b/src/renderers/shared/stack/reconciler/ReactInstanceType.js index e22ff632f54f0..5b23ae4690601 100644 --- a/src/renderers/shared/stack/reconciler/ReactInstanceType.js +++ b/src/renderers/shared/stack/reconciler/ReactInstanceType.js @@ -13,6 +13,7 @@ 'use strict'; import type {ReactElement} from 'ReactElementType'; +import type {CompositeComponentTypes} from 'ReactCompositeComponentTypes'; export type DebugID = number; @@ -34,6 +35,7 @@ export type ReactInstance = { attachRef: (ref: string, component: ReactInstance) => void, detachRef: (ref: string) => void, _rootNodeID: number, + _compositeType: CompositeComponentTypes, // ReactDOMComponent _tag: string, diff --git a/src/renderers/shared/stack/reconciler/ReactRef.js b/src/renderers/shared/stack/reconciler/ReactRef.js index a7350cc6a8a73..762e29685cd19 100644 --- a/src/renderers/shared/stack/reconciler/ReactRef.js +++ b/src/renderers/shared/stack/reconciler/ReactRef.js @@ -19,7 +19,34 @@ import type { ReactElement } from 'ReactElementType'; var ReactRef = {}; +if (__DEV__) { + var ReactCompositeComponentTypes = require('ReactCompositeComponentTypes'); + var ReactComponentTreeHook = require('ReactComponentTreeHook'); + var warning = require('warning'); +} + function attachRef(ref, component, owner) { + if (__DEV__) { + let info = ''; + if (owner) { + let ownerName; + if (typeof owner.getName === 'function') { + ownerName = owner.getName(); + } + if (ownerName) { + info += ' Check the render method of `' + ownerName + '`.'; + } + } + + warning( + component._compositeType !== ReactCompositeComponentTypes.StatelessFunctional, + 'Stateless function components cannot be given refs. ' + + 'Attempts to access this ref will fail.%s%s', + info, + ReactComponentTreeHook.getStackAddendumByID(component._debugID) + ); + } + if (typeof ref === 'function') { ref(component.getPublicInstance()); } else { diff --git a/src/renderers/testing/__tests__/ReactTestRenderer-test.js b/src/renderers/testing/__tests__/ReactTestRenderer-test.js index 8135d164da272..31a63cb9e20a1 100644 --- a/src/renderers/testing/__tests__/ReactTestRenderer-test.js +++ b/src/renderers/testing/__tests__/ReactTestRenderer-test.js @@ -15,6 +15,9 @@ var React = require('React'); var ReactTestRenderer = require('ReactTestRenderer'); describe('ReactTestRenderer', () => { + function normalizeCodeLocInfo(str) { + return str && str.replace(/\(at .+?:\d+\)/g, '(at **)'); + } it('renders a simple component', () => { function Link() { @@ -231,10 +234,11 @@ describe('ReactTestRenderer', () => { ReactTestRenderer.create(); ReactTestRenderer.create(); expectDev(console.error.calls.count()).toBe(1); - expectDev(console.error.calls.argsFor(0)[0]).toContain( - 'Stateless function components cannot be given refs ' + - '(See ref "foo" in Bar created by Foo). ' + - 'Attempts to access this ref will fail.' + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( + 'Warning: Stateless function components cannot be given refs. Attempts ' + + 'to access this ref will fail. Check the render method of `Foo`.\n' + + ' in Bar (at **)\n' + + ' in Foo (at **)' ); });