From 7bc8f36407d949eddf4b67f05262885aa29d12d0 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 14 Dec 2016 00:03:22 +0000 Subject: [PATCH 01/14] Implement component stack for some warnings in Fiber --- scripts/fiber/tests-failing.txt | 4 --- scripts/fiber/tests-passing-except-dev.txt | 16 --------- scripts/fiber/tests-passing.txt | 14 ++++++++ .../hooks/ReactComponentTreeHook.js | 35 +++++++++++++++++-- src/renderers/shared/fiber/ReactChildFiber.js | 7 ++-- src/renderers/shared/fiber/ReactFiber.js | 31 +++++++++++++--- .../shared/fiber/ReactFiberContext.js | 6 ++-- src/shared/types/checkReactTypeSpec.js | 14 +++++--- 8 files changed, 89 insertions(+), 38 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 254abf87b27a..d02b83955316 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -12,9 +12,6 @@ src/isomorphic/classic/__tests__/ReactContextValidator-test.js src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js * includes the owner name when passing null, undefined, boolean, or number -src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js -* should give context for PropType errors in nested components. - src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production @@ -64,7 +61,6 @@ src/renderers/shared/__tests__/ReactPerf-test.js * should work when measurement starts during reconciliation src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js -* gets created * can be retrieved by ID src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index 0b604fcf6f56..892c5248ab5a 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -1,21 +1,12 @@ src/addons/transitions/__tests__/ReactTransitionGroup-test.js * should warn for duplicated keys with component stack info -src/isomorphic/classic/__tests__/ReactContextValidator-test.js -* should check context types -* should check child context types - src/isomorphic/classic/element/__tests__/ReactElementClone-test.js * should check declared prop types after clone src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js -* warns for keys for arrays of elements with owner info -* warns for keys with component stack info * should give context for PropType errors in nested components. -src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js -* warns for keys for arrays of elements with owner info - src/renderers/dom/shared/__tests__/CSSPropertyOperations-test.js * should warn when using hyphenated style names * should warn when updating hyphenated style names @@ -133,15 +124,9 @@ src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js * gets recorded for composite roots * gets recorded when a native is mounted deeply instead of null * gets recorded during mount -* gets recorded during an update -* gets ignored if the styles are shallowly equal * gets recorded during mount -* gets recorded during an update -* gets recorded as a removal during an update * gets recorded during mount -* gets recorded during an update * gets recorded during mount -* gets recorded during an update * gets recorded during an update from text content * gets recorded during an update from html * gets recorded during an update from children @@ -176,7 +161,6 @@ 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 -* should use correct name in key warning src/renderers/shared/shared/__tests__/refs-test.js * attaches, detaches from fiber component with stack layer diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 2c127f082de0..7d8241e46b75 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -153,6 +153,8 @@ src/isomorphic/children/__tests__/sliceChildren-test.js src/isomorphic/classic/__tests__/ReactContextValidator-test.js * should filter out context not in contextTypes * should pass next context to lifecycles +* should check context types +* should check child context types src/isomorphic/classic/class/__tests__/ReactBind-test.js * Holds reference to instance @@ -272,8 +274,10 @@ src/isomorphic/classic/element/__tests__/ReactElementClone-test.js src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js * warns for keys for arrays of elements in rest args +* warns for keys for arrays of elements with owner info * warns for keys for arrays with no owner or parent info * warns for keys for arrays of elements with no owner info +* warns for keys with component stack info * does not warn for keys when passing children down * warns for keys for iterables of elements in rest args * does not warns for arrays of elements with keys @@ -471,12 +475,14 @@ src/isomorphic/modern/element/__tests__/ReactJSXElement-test.js src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js * warns for keys for arrays of elements in children position +* warns for keys for arrays of elements with owner info * warns for keys for iterables of elements in rest args * does not warns for arrays of elements with keys * does not warns for iterable elements with keys * does not warn for numeric keys in entry iterable as a child * does not warn when the element is directly as children * does not warn when the child array contains non-elements +* should give context for PropType errors in nested components. * gives a helpful error when passing null, undefined, or boolean * should check default prop values * should not check the default for explicit null @@ -1204,6 +1210,7 @@ src/renderers/shared/fiber/__tests__/ReactTopLevelText-test.js * should render a component returning numbers directly from render src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js +* gets created * is created during mounting * is created when calling renderToString during render @@ -1258,6 +1265,12 @@ src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.native.js src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js * gets ignored for composite roots that return null +* gets recorded during an update +* gets ignored if the styles are shallowly equal +* gets recorded during an update +* gets recorded as a removal during an update +* gets recorded during an update +* gets recorded during an update * gets ignored if new text is equal * gets ignored if new text is equal * gets ignored if the type has not changed @@ -1455,6 +1468,7 @@ src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js * should unmount stateless component * should pass context thru stateless component * should provide a null ref +* should use correct name in key warning * should support default props and prop types * should receive context * should work with arrow functions diff --git a/src/isomorphic/hooks/ReactComponentTreeHook.js b/src/isomorphic/hooks/ReactComponentTreeHook.js index a5fdd85a58cb..27f40f01b48c 100644 --- a/src/isomorphic/hooks/ReactComponentTreeHook.js +++ b/src/isomorphic/hooks/ReactComponentTreeHook.js @@ -20,6 +20,7 @@ var warning = require('warning'); import type { ReactElement, Source } from 'ReactElementType'; import type { DebugID } from 'ReactInstanceType'; +import type { Fiber } from 'ReactFiber'; function isNative(fn) { // Based on isNative() from Lodash @@ -191,6 +192,20 @@ function describeID(id: DebugID): string { return describeComponentFrame(name, element && element._source, ownerName); } +function describeFiber(fiber : Fiber) : string { + var owner = fiber._debugOwner; + var source = fiber._debugSource; + if (owner == null && source == null) { + return ''; + } + var name = getComponentName(fiber); + var ownerName = null; + if (owner) { + ownerName = getComponentName(owner); + } + return describeComponentFrame(name, source, ownerName); +} + var ReactComponentTreeHook = { onSetChildren(id: DebugID, nextChildIDs: Array): void { var item = getItem(id); @@ -324,13 +339,27 @@ var ReactComponentTreeHook = { } var currentOwner = ReactCurrentOwner.current; - if (currentOwner && typeof currentOwner._debugID === 'number') { - var id = currentOwner && currentOwner._debugID; - info += ReactComponentTreeHook.getStackAddendumByID(id); + if (currentOwner) { + if (typeof currentOwner.tag === 'number') { + const fiber = ((currentOwner : any) : Fiber); + info += ReactComponentTreeHook.getStackAddendumByFiber(fiber); + } else if (typeof currentOwner._debugID === 'number') { + info += ReactComponentTreeHook.getStackAddendumByID(currentOwner._debugID); + } } return info; }, + getStackAddendumByFiber(fiber : Fiber) : string { + var info = ''; + var node = fiber; + do { + info += describeFiber(node); + node = node.return; + } while (node); + return info; + }, + getStackAddendumByID(id: ?DebugID): string { var info = ''; while (id) { diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index e7ee06f88c41..c6bb22545b3b 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -12,6 +12,7 @@ 'use strict'; +import type { ReactElement } from 'ReactElementType'; import type { ReactCoroutine, ReactYield } from 'ReactCoroutine'; import type { ReactPortal } from 'ReactPortal'; import type { Fiber } from 'ReactFiber'; @@ -68,7 +69,7 @@ const { Deletion, } = ReactTypeOfSideEffect; -function coerceRef(current: ?Fiber, element: ReactElement) { +function coerceRef(current: ?Fiber, element: ReactElement) { let mixedRef = element.ref; if (mixedRef != null && typeof mixedRef !== 'function') { if (element._owner) { @@ -256,7 +257,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { function updateElement( returnFiber : Fiber, current : ?Fiber, - element : ReactElement, + element : ReactElement, priority : PriorityLevel ) : Fiber { if (current == null || current.type !== element.type) { @@ -854,7 +855,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { function reconcileSingleElement( returnFiber : Fiber, currentFirstChild : ?Fiber, - element : ReactElement, + element : ReactElement, priority : PriorityLevel ) : Fiber { const key = element.key; diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index 7d3097ffaf4f..7f0d444aa751 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/src/renderers/shared/fiber/ReactFiber.js @@ -12,6 +12,8 @@ 'use strict'; +import type { ReactElement, Source } from 'ReactElementType'; +import type { ReactInstance, DebugID } from 'ReactInstanceType'; import type { ReactFragment } from 'ReactTypes'; import type { ReactCoroutine, ReactYield } from 'ReactCoroutine'; import type { ReactPortal } from 'ReactPortal'; @@ -46,6 +48,11 @@ var invariant = require('invariant'); // A Fiber is work on a Component that needs to be done or was done. There can // be more than one per component. export type Fiber = { + // __DEV__ only + _debugID ?: DebugID, + _debugSource ?: Source, + _debugOwner ?: Fiber | ReactInstance | null, // Stack compatible + // These first fields are conceptually members of an Instance. This used to // be split into a separate type and intersected with the other Fiber fields, // but until Flow fixes its intersection bugs, we've merged them into a @@ -145,7 +152,7 @@ export type Fiber = { }; if (__DEV__) { - var debugCounter = 0; + var debugCounter = 1; } // This is a constructor of a POJO instead of a constructor function for a few @@ -204,10 +211,14 @@ var createFiber = function(tag : TypeOfWork, key : null | string) : Fiber { alternate: null, }; + if (__DEV__) { (fiber : any)._debugID = debugCounter++; + (fiber : any)._debugSource = null; + (fiber : any)._debugOwner = null; } - return fiber; + + return (fiber : any); // Don't set dev-only fields in production }; function shouldConstruct(Component) { @@ -266,6 +277,13 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi alt.memoizedProps = fiber.memoizedProps; alt.memoizedState = fiber.memoizedState; + if (__DEV__) { + alt._debugID = fiber._debugID; + // TODO: reassign this when an element is updated. + alt._debugSource = fiber._debugSource; + alt._debugOwner = fiber._debugOwner; + } + return alt; }; @@ -274,11 +292,16 @@ exports.createHostRootFiber = function() : Fiber { return fiber; }; -exports.createFiberFromElement = function(element : ReactElement<*>, priorityLevel : PriorityLevel) : Fiber { -// $FlowFixMe: ReactElement.key is currently defined as ?string but should be defined as null | string in Flow. +exports.createFiberFromElement = function(element : ReactElement, priorityLevel : PriorityLevel) : Fiber { const fiber = createFiberFromElementType(element.type, element.key); fiber.pendingProps = element.props; fiber.pendingWorkPriority = priorityLevel; + + if (__DEV__) { + fiber._debugSource = element._source; + fiber._debugOwner = element._owner; + } + return fiber; }; diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index 4fdb8e2ec31d..83964f01b3e1 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -55,8 +55,7 @@ exports.getMaskedContext = function(fiber : Fiber) { if (__DEV__) { const name = getComponentName(fiber); - const debugID = 0; // TODO: pass a real ID - checkReactTypeSpec(contextTypes, context, 'context', name, null, debugID); + checkReactTypeSpec(contextTypes, context, 'context', name, null, fiber); } return context; @@ -105,8 +104,7 @@ function processChildContext(fiber : Fiber, parentContext : Object): Object { } if (__DEV__) { const name = getComponentName(fiber); - const debugID = 0; // TODO: pass a real ID - checkReactTypeSpec(childContextTypes, childContext, 'childContext', name, null, debugID); + checkReactTypeSpec(childContextTypes, childContext, 'childContext', name, null, fiber); } return {...parentContext, ...childContext}; } diff --git a/src/shared/types/checkReactTypeSpec.js b/src/shared/types/checkReactTypeSpec.js index c9315a349cdf..bd51d3faf392 100644 --- a/src/shared/types/checkReactTypeSpec.js +++ b/src/shared/types/checkReactTypeSpec.js @@ -44,7 +44,7 @@ var loggedTypeFailures = {}; * @param {string} location e.g. "prop", "context", "child context" * @param {string} componentName Name of the component for error messages. * @param {?object} element The React element that is being type-checked - * @param {?number} debugID The React component instance that is being type-checked + * @param {?number} fiberOrDebugID The React component instance that is being type-checked * @private */ function checkReactTypeSpec( @@ -53,7 +53,7 @@ function checkReactTypeSpec( location: ReactPropTypeLocations, componentName, element, - debugID, + fiberOrDebugID, ) { for (var typeSpecName in typeSpecs) { if (typeSpecs.hasOwnProperty(typeSpecName)) { @@ -99,8 +99,14 @@ function checkReactTypeSpec( if (!ReactComponentTreeHook) { ReactComponentTreeHook = require('ReactComponentTreeHook'); } - if (debugID !== null) { - componentStackInfo = ReactComponentTreeHook.getStackAddendumByID(debugID); + if (fiberOrDebugID != null) { + if (typeof fiberOrDebugID === 'number') { + const debugID = fiberOrDebugID; + componentStackInfo = ReactComponentTreeHook.getStackAddendumByID(debugID); + } else if (typeof fiberOrDebugID.tag === 'number') { + const fiber = fiberOrDebugID; + componentStackInfo = ReactComponentTreeHook.getStackAddendumByFiber(fiber); + } } else if (element !== null) { componentStackInfo = ReactComponentTreeHook.getCurrentStackAddendum(element); } From c456f7de181f387493eb2b60f0f8ac0ac44a0314 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 14 Dec 2016 01:14:09 +0000 Subject: [PATCH 02/14] Keep Fiber debug source up to date When an element changes, we should copy the source and owner again. Otherwise they can get stale since we're not reading them from the element. --- scripts/fiber/tests-passing.txt | 1 + .../ReactJSXElementValidator-test.js | 41 +++++++++++++++++++ src/renderers/shared/fiber/ReactChildFiber.js | 8 ++++ src/renderers/shared/fiber/ReactFiber.js | 1 - 4 files changed, 50 insertions(+), 1 deletion(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 7d8241e46b75..37c1f2a71be3 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -483,6 +483,7 @@ src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js * does not warn when the element is directly as children * does not warn when the child array contains non-elements * should give context for PropType errors in nested components. +* should update component stack after receiving next element * gives a helpful error when passing null, undefined, or boolean * should check default prop values * should not check the default for explicit null diff --git a/src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js b/src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js index 47563ff67b21..f573a6f66242 100644 --- a/src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js +++ b/src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js @@ -15,6 +15,7 @@ // of dynamic errors when using JSX with Flow. var React; +var ReactDOM; var ReactTestUtils; describe('ReactJSXElementValidator', () => { @@ -25,6 +26,7 @@ describe('ReactJSXElementValidator', () => { jest.resetModuleRegistry(); React = require('React'); + ReactDOM = require('ReactDOM'); ReactTestUtils = require('ReactTestUtils'); Component = class extends React.Component { @@ -206,6 +208,45 @@ describe('ReactJSXElementValidator', () => { ); }); + it('should update component stack after receiving next element', () => { + spyOn(console, 'error'); + function MyComp() { + return null; + } + MyComp.propTypes = { + color: React.PropTypes.string, + }; + function MiddleComp(props) { + return ; + } + function ParentComp(props) { + if (props.warn) { + // This element has a source thanks to JSX. + return ; + } + // This element has no source. + return React.createElement(MiddleComp, {color: 'blue'}); + } + + var container = document.createElement('div'); + ReactDOM.render(, container); + ReactDOM.render(, container); + + expect(console.error.calls.count()).toBe(1); + // The warning should have the full stack with line numbers. + // If it doesn't, it means we're using information from the old element. + expect( + console.error.calls.argsFor(0)[0].replace(/\(at .+?:\d+\)/g, '(at **)') + ).toBe( + 'Warning: Failed prop type: ' + + 'Invalid prop `color` of type `number` supplied to `MyComp`, ' + + 'expected `string`.\n' + + ' in MyComp (at **)\n' + + ' in MiddleComp (at **)\n' + + ' in ParentComp (at **)' + ); + }); + it('gives a helpful error when passing null, undefined, or boolean', () => { var Undefined = undefined; var Null = null; diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index c6bb22545b3b..d82e3cd3b6bc 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -272,6 +272,10 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { existing.ref = coerceRef(current, element); existing.pendingProps = element.props; existing.return = returnFiber; + if (__DEV__) { + existing._debugSource = element._source; + existing._debugOwner = element._owner; + } return existing; } } @@ -870,6 +874,10 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { existing.ref = coerceRef(child, element); existing.pendingProps = element.props; existing.return = returnFiber; + if (__DEV__) { + existing._debugSource = element._source; + existing._debugOwner = element._owner; + } return existing; } else { deleteRemainingChildren(returnFiber, child); diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index 7f0d444aa751..885c4ef4041a 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/src/renderers/shared/fiber/ReactFiber.js @@ -279,7 +279,6 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi if (__DEV__) { alt._debugID = fiber._debugID; - // TODO: reassign this when an element is updated. alt._debugSource = fiber._debugSource; alt._debugOwner = fiber._debugOwner; } From 18808a148cf979bcffc14b62828bf2532da6e50c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 14 Dec 2016 01:51:11 +0000 Subject: [PATCH 03/14] Remove outdated TODOs from tests --- .../classic/element/__tests__/ReactElementValidator-test.js | 3 --- .../modern/element/__tests__/ReactJSXElementValidator-test.js | 3 --- 2 files changed, 6 deletions(-) diff --git a/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js b/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js index 11dfb266b9bc..6783f762697f 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js @@ -258,9 +258,6 @@ describe('ReactElementValidator', () => { expectDev(console.error.calls.count()).toBe(0); }); - // TODO: These warnings currently come from the composite component, but - // they should be moved into the ReactElementValidator. - it('should give context for PropType errors in nested components.', () => { // In this test, we're making sure that if a proptype error is found in a // component, we give a small hint as to which parent instantiated that diff --git a/src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js b/src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js index f573a6f66242..21d9999ebb55 100644 --- a/src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js +++ b/src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js @@ -175,9 +175,6 @@ describe('ReactJSXElementValidator', () => { expectDev(console.error.calls.count()).toBe(0); }); - // TODO: These warnings currently come from the composite component, but - // they should be moved into the ReactElementValidator. - it('should give context for PropType errors in nested components.', () => { // In this test, we're making sure that if a proptype error is found in a // component, we give a small hint as to which parent instantiated that From d5016762fb1208bb459a16b000cc37dc8da15235 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 14 Dec 2016 01:51:51 +0000 Subject: [PATCH 04/14] Explicitly specify Fiber types to include in the stack Fixes an accidental omission when both source and owner are null but displayName exists. --- scripts/fiber/tests-passing-except-dev.txt | 6 ---- scripts/fiber/tests-passing.txt | 2 ++ .../hooks/ReactComponentTreeHook.js | 32 +++++++++++++------ 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index 892c5248ab5a..a6646b378a3e 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -1,12 +1,6 @@ src/addons/transitions/__tests__/ReactTransitionGroup-test.js * should warn for duplicated keys with component stack info -src/isomorphic/classic/element/__tests__/ReactElementClone-test.js -* should check declared prop types after clone - -src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js -* should give context for PropType errors in nested components. - src/renderers/dom/shared/__tests__/CSSPropertyOperations-test.js * should warn when using hyphenated style names * should warn when updating hyphenated style names diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 37c1f2a71be3..0baf2bc65e37 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -268,6 +268,7 @@ src/isomorphic/classic/element/__tests__/ReactElementClone-test.js * does not warns for arrays of elements with keys * does not warn when the element is directly in rest args * does not warn when the array contains a non-element +* should check declared prop types after clone * should ignore key and ref warning getters * should ignore undefined key and ref * should extract null key and ref @@ -284,6 +285,7 @@ src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js * does not warns for iterable elements with keys * does not warn when the element is directly in rest args * does not warn when the array contains a non-element +* should give context for PropType errors in nested components. * gives a helpful error when passing null, undefined, boolean, or number * should check default prop values * should not check the default for explicit null diff --git a/src/isomorphic/hooks/ReactComponentTreeHook.js b/src/isomorphic/hooks/ReactComponentTreeHook.js index 27f40f01b48c..0ef8a6d4a181 100644 --- a/src/isomorphic/hooks/ReactComponentTreeHook.js +++ b/src/isomorphic/hooks/ReactComponentTreeHook.js @@ -13,6 +13,13 @@ 'use strict'; var ReactCurrentOwner = require('ReactCurrentOwner'); +var ReactTypeOfWork = require('ReactTypeOfWork'); +var { + IndeterminateComponent, + FunctionalComponent, + ClassComponent, + HostComponent, +} = ReactTypeOfWork; var getComponentName = require('getComponentName'); var invariant = require('invariant'); @@ -193,17 +200,22 @@ function describeID(id: DebugID): string { } function describeFiber(fiber : Fiber) : string { - var owner = fiber._debugOwner; - var source = fiber._debugSource; - if (owner == null && source == null) { - return ''; - } - var name = getComponentName(fiber); - var ownerName = null; - if (owner) { - ownerName = getComponentName(owner); + switch (fiber.tag) { + case IndeterminateComponent: + case FunctionalComponent: + case ClassComponent: + case HostComponent: + var owner = fiber._debugOwner; + var source = fiber._debugSource; + var name = getComponentName(fiber); + var ownerName = null; + if (owner) { + ownerName = getComponentName(owner); + } + return describeComponentFrame(name, source, ownerName); + default: + return ''; } - return describeComponentFrame(name, source, ownerName); } var ReactComponentTreeHook = { From ddae7dba9a9beb7c5b5d7fee84df260fcf40e825 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 14 Dec 2016 02:10:44 +0000 Subject: [PATCH 05/14] Fix mised Stack+Fiber test to not expect extra warnings When we're in Fiber mode we don't actually expect that warning being printed. --- scripts/fiber/tests-passing-except-dev.txt | 4 --- scripts/fiber/tests-passing.txt | 2 ++ .../shared/shared/__tests__/refs-test.js | 29 +++++++++++-------- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index a6646b378a3e..2fd5a6792a9e 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -155,7 +155,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 - -src/renderers/shared/shared/__tests__/refs-test.js -* attaches, detaches from fiber component with stack layer -* attaches, detaches from stack component with fiber layer diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 0baf2bc65e37..17251177b87a 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1526,6 +1526,8 @@ src/renderers/shared/shared/__tests__/refs-test.js * ref called correctly for stateless component when __DEV__ = false * ref called correctly for stateless component when __DEV__ = true * coerces numbers to strings +* attaches, detaches from fiber component with stack layer +* attaches, detaches from stack component with fiber layer src/renderers/shared/shared/event/__tests__/EventPluginHub-test.js * should prevent non-function listeners, at dispatch diff --git a/src/renderers/shared/shared/__tests__/refs-test.js b/src/renderers/shared/shared/__tests__/refs-test.js index f18b5870e161..a0ba0027061d 100644 --- a/src/renderers/shared/shared/__tests__/refs-test.js +++ b/src/renderers/shared/shared/__tests__/refs-test.js @@ -12,6 +12,7 @@ 'use strict'; var React = require('React'); +var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); var ReactTestUtils = require('ReactTestUtils'); /** @@ -334,12 +335,14 @@ describe('string refs between fiber and stack', () => { ReactDOMFiber.unmountComponentAtNode(container); expect(a.refs.span).toBe(undefined); expect(layerMounted).toBe(true); - expectDev(console.error.calls.count()).toBe(1); - expectDev(console.error.calls.argsFor(0)[0]).toBe( - 'Warning: You are using React DOM Fiber which is an experimental ' + - 'renderer. It is likely to have bugs, breaking changes and is ' + - 'unsupported.' - ); + if (!ReactDOMFeatureFlags.useFiber) { + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toBe( + 'Warning: You are using React DOM Fiber which is an experimental ' + + 'renderer. It is likely to have bugs, breaking changes and is ' + + 'unsupported.' + ); + } }); it('attaches, detaches from stack component with fiber layer', () => { @@ -379,11 +382,13 @@ describe('string refs between fiber and stack', () => { ReactDOM.unmountComponentAtNode(container); expect(a.refs.span).toBe(undefined); expect(layerMounted).toBe(true); - expectDev(console.error.calls.count()).toBe(1); - expectDev(console.error.calls.argsFor(0)[0]).toBe( - 'Warning: You are using React DOM Fiber which is an experimental ' + - 'renderer. It is likely to have bugs, breaking changes and is ' + - 'unsupported.' - ); + if (!ReactDOMFeatureFlags.useFiber) { + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toBe( + 'Warning: You are using React DOM Fiber which is an experimental ' + + 'renderer. It is likely to have bugs, breaking changes and is ' + + 'unsupported.' + ); + } }); }); From aa92f7489a25c1b7490e21cade7db4be8dd8a8a1 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 14 Dec 2016 02:21:03 +0000 Subject: [PATCH 06/14] Warn on passing different props to super() --- scripts/fiber/tests-passing-except-dev.txt | 1 - scripts/fiber/tests-passing.txt | 1 + src/renderers/shared/fiber/ReactFiberClassComponent.js | 8 ++++++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index 2fd5a6792a9e..3ca34eebd493 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -146,7 +146,6 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js * should warn about `forceUpdate` on unmounted components * should warn about `setState` on unmounted components * should disallow nested render calls -* should warn when mutated props are passed src/renderers/shared/shared/__tests__/ReactMultiChild-test.js * should warn for duplicated keys with component stack info diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 17251177b87a..b68d9c260232 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1333,6 +1333,7 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js * should replace state * should support objects with prototypes as state * should not warn about unmounting during unmounting +* should warn when mutated props are passed * should only call componentWillUnmount once src/renderers/shared/shared/__tests__/ReactCompositeComponentDOMMinimalism-test.js diff --git a/src/renderers/shared/fiber/ReactFiberClassComponent.js b/src/renderers/shared/fiber/ReactFiberClassComponent.js index 79a6a9ebc9ef..79595dd152d7 100644 --- a/src/renderers/shared/fiber/ReactFiberClassComponent.js +++ b/src/renderers/shared/fiber/ReactFiberClassComponent.js @@ -180,6 +180,14 @@ module.exports = function(scheduleUpdate : (fiber: Fiber) => void) { 'componentWillRecieveProps(). Did you mean componentWillReceiveProps()?', name ); + const hasMutatedProps = instance.props !== workInProgress.pendingProps; + warning( + instance.props === undefined || !hasMutatedProps, + '%s(...): When calling super() in `%s`, make sure to pass ' + + 'up the same props that your component\'s constructor was passed.', + name, + name + ); } const state = instance.state; From dbb62a9dda1f2f9655544de3ddc28d00761438d2 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 14 Dec 2016 03:55:23 +0000 Subject: [PATCH 07/14] Implement duplicate key warnings We keep known keys in a set in development. There is an annoying special case where we know we'll check it again because we break out of the loop early. One test in the tree hook regresses to the failing state because it checks that the tree hook works without a Set available, but we started using Set in this code. It is not essential and we can clean this up later when we decide how to deal with polyfills. --- scripts/fiber/tests-passing-except-dev.txt | 5 - scripts/fiber/tests-passing.txt | 8 ++ src/renderers/shared/fiber/ReactChildFiber.js | 94 +++++++++++++++++-- src/renderers/shared/fiber/ReactFiber.js | 12 +-- .../__tests__/ReactChildReconciler-test.js | 77 ++++++++++++++- .../shared/__tests__/ReactMultiChild-test.js | 68 +++++++++++++- 6 files changed, 238 insertions(+), 26 deletions(-) diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index 3ca34eebd493..07a2dab9c724 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -133,10 +133,6 @@ src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js * gets reported when a child is inserted * gets reported when a child is removed -src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js -* warns for duplicated keys -* warns for duplicated keys with component stack info - src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js * should correctly determine if a component is mounted * should correctly determine if a null component is mounted @@ -148,7 +144,6 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js * should disallow nested render calls src/renderers/shared/shared/__tests__/ReactMultiChild-test.js -* should warn for duplicated keys with component stack info * should warn for using maps as children with owner info src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index b68d9c260232..1a98eff257bf 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1279,6 +1279,12 @@ src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js * gets ignored if the type has not changed * gets ignored if new html is equal +src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js +* warns for duplicated array keys +* warns for duplicated array keys with component stack info +* warns for duplicated iterable keys +* warns for duplicated iterable keys with component stack info + src/renderers/shared/shared/__tests__/ReactComponent-test.js * should throw when supplying a ref outside of render method * should warn when children are mutated during render @@ -1421,6 +1427,8 @@ src/renderers/shared/shared/__tests__/ReactMultiChild-test.js * should replace children with different constructors * should NOT replace children with different owners * should replace children with different keys +* should warn for duplicated array keys with component stack info +* should warn for duplicated iterable keys with component stack info * should reorder bailed-out children src/renderers/shared/shared/__tests__/ReactMultiChildReconcile-test.js diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index d82e3cd3b6bc..33c19b362317 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -37,6 +37,12 @@ var emptyObject = require('emptyObject'); var getIteratorFn = require('getIteratorFn'); var invariant = require('invariant'); +if (__DEV__) { + var ReactComponentTreeHook = require('ReactComponentTreeHook'); + var { getStackAddendumByFiber } = ReactComponentTreeHook; + var warning = require('warning'); +} + const { cloneFiber, createFiberFromElement, @@ -541,6 +547,49 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { return null; } + function warnOnDuplicateKey( + child : mixed, + returnFiber : Fiber, + knownKeys : Set | null + ) : Set | null { + if (__DEV__) { + if (typeof child !== 'object' || child == null) { + return knownKeys; + } + switch (child.$$typeof) { + case REACT_ELEMENT_TYPE: + case REACT_COROUTINE_TYPE: + case REACT_YIELD_TYPE: + case REACT_PORTAL_TYPE: + const key = child.key; + if (typeof key !== 'string') { + break; + } + if (knownKeys == null) { + knownKeys = new Set(); + knownKeys.add(key); + break; + } + if (!knownKeys.has(key)) { + knownKeys.add(key); + break; + } + warning( + false, + 'Encountered two children with the same key, ' + + '`%s`. Child keys must be unique; when two children share a key, ' + + 'only the first child will be used.%s', + key, + getStackAddendumByFiber(returnFiber) + ); + break; + default: + break; + } + } + return knownKeys; + } + function reconcileChildrenArray( returnFiber : Fiber, currentFirstChild : ?Fiber, @@ -566,6 +615,15 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { // If you change this code, also update reconcileChildrenIterator() which // uses the same algorithm. + if (__DEV__) { + // First, validate keys. + let knownKeys = null; + for (let i = 0; i < newChildren.length; i++) { + const child = newChildren[i]; + knownKeys = warnOnDuplicateKey(child, returnFiber, knownKeys); + } + } + let resultingFirstChild : ?Fiber = null; let previousNewFiber : ?Fiber = null; @@ -696,12 +754,37 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { function reconcileChildrenIterator( returnFiber : Fiber, currentFirstChild : ?Fiber, - newChildren : Iterator<*>, + newChildrenIterable : Iterable<*>, priority : PriorityLevel) : ?Fiber { // This is the same implementation as reconcileChildrenArray(), // but using the iterator instead. + const iteratorFn = getIteratorFn(newChildrenIterable); + if (typeof iteratorFn !== 'function') { + throw new Error('An object is not an iterable.'); + } + + if (__DEV__) { + // First, validate keys. + // We'll get a different iterator later for the main pass. + const newChildren = iteratorFn.call(newChildrenIterable); + if (newChildren == null) { + throw new Error('An iterable object provided no iterator.'); + } + let knownKeys = null; + let step = newChildren.next(); + for (; !step.done; step = newChildren.next()) { + const child = step.value; + knownKeys = warnOnDuplicateKey(child, returnFiber, knownKeys); + } + } + + const newChildren = iteratorFn.call(newChildrenIterable); + if (newChildren == null) { + throw new Error('An iterable object provided no iterator.'); + } + let resultingFirstChild : ?Fiber = null; let previousNewFiber : ?Fiber = null; @@ -1070,16 +1153,11 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { ); } - const iteratorFn = getIteratorFn(newChild); - if (iteratorFn) { - const iterator = iteratorFn.call(newChild); - if (iterator == null) { - throw new Error('An iterable object provided no iterator.'); - } + if (getIteratorFn(newChild)) { return reconcileChildrenIterator( returnFiber, currentFirstChild, - iterator, + newChild, priority ); } diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index 885c4ef4041a..8873e2b46c22 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/src/renderers/shared/fiber/ReactFiber.js @@ -50,7 +50,7 @@ var invariant = require('invariant'); export type Fiber = { // __DEV__ only _debugID ?: DebugID, - _debugSource ?: Source, + _debugSource ?: Source | null, _debugOwner ?: Fiber | ReactInstance | null, // Stack compatible // These first fields are conceptually members of an Instance. This used to @@ -169,7 +169,7 @@ if (__DEV__) { // 5) It should be easy to port this to a C struct and keep a C implementation // compatible. var createFiber = function(tag : TypeOfWork, key : null | string) : Fiber { - var fiber = { + var fiber : Fiber = { // Instance @@ -213,12 +213,12 @@ var createFiber = function(tag : TypeOfWork, key : null | string) : Fiber { }; if (__DEV__) { - (fiber : any)._debugID = debugCounter++; - (fiber : any)._debugSource = null; - (fiber : any)._debugOwner = null; + fiber._debugID = debugCounter++; + fiber._debugSource = null; + fiber._debugOwner = null; } - return (fiber : any); // Don't set dev-only fields in production + return fiber; }; function shouldConstruct(Component) { diff --git a/src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js b/src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js index 5b1dca744809..1e0c0adce631 100644 --- a/src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js +++ b/src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js @@ -29,7 +29,25 @@ describe('ReactChildReconciler', () => { ReactTestUtils = require('ReactTestUtils'); }); - it('warns for duplicated keys', () => { + function createIterable(array) { + return { + '@@iterator': function() { + var i = 0; + return { + next() { + const next = { + value: i < array.length ? array[i] : undefined, + done: i === array.length, + }; + i++; + return next; + }, + }; + }, + }; + } + + it('warns for duplicated array keys', () => { spyOn(console, 'error'); class Component extends React.Component { @@ -46,7 +64,7 @@ describe('ReactChildReconciler', () => { ); }); - it('warns for duplicated keys with component stack info', () => { + it('warns for duplicated array keys with component stack info', () => { spyOn(console, 'error'); class Component extends React.Component { @@ -70,8 +88,59 @@ describe('ReactChildReconciler', () => { ReactTestUtils.renderIntoDocument(); expectDev(console.error.calls.count()).toBe(1); - expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: flattenChildren(...): ' + + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toContain( + 'Encountered two children with the same key, `1`. ' + + 'Child keys must be unique; when two children share a key, ' + + 'only the first child will be used.\n' + + ' in div (at **)\n' + + ' in Component (at **)\n' + + ' in Parent (at **)\n' + + ' in GrandParent (at **)' + ); + }); + + it('warns for duplicated iterable keys', () => { + spyOn(console, 'error'); + + class Component extends React.Component { + render() { + return
{createIterable([
,
])}
; + } + } + + ReactTestUtils.renderIntoDocument(); + + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Child keys must be unique; when two children share a key, only the first child will be used.' + ); + }); + + it('warns for duplicated iterable keys with component stack info', () => { + spyOn(console, 'error'); + + class Component extends React.Component { + render() { + return
{createIterable([
,
])}
; + } + } + + class Parent extends React.Component { + render() { + return React.cloneElement(this.props.child); + } + } + + class GrandParent extends React.Component { + render() { + return } />; + } + } + + ReactTestUtils.renderIntoDocument(); + + expectDev(console.error.calls.count()).toBe(1); + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toContain( 'Encountered two children with the same key, `1`. ' + 'Child keys must be unique; when two children share a key, ' + 'only the first child will be used.\n' + diff --git a/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js b/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js index e2d14a7cc3c9..3e005fd1afca 100644 --- a/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js +++ b/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js @@ -152,7 +152,7 @@ describe('ReactMultiChild', () => { expect(mockUnmount.mock.calls.length).toBe(1); }); - it('should warn for duplicated keys with component stack info', () => { + it('should warn for duplicated array keys with component stack info', () => { spyOn(console, 'error'); var container = document.createElement('div'); @@ -186,8 +186,70 @@ describe('ReactMultiChild', () => { ); expectDev(console.error.calls.count()).toBe(1); - expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: flattenChildren(...): ' + + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toContain( + 'Encountered two children with the same key, `1`. ' + + 'Child keys must be unique; when two children share a key, ' + + 'only the first child will be used.\n' + + ' in div (at **)\n' + + ' in WrapperComponent (at **)\n' + + ' in div (at **)\n' + + ' in Parent (at **)' + ); + }); + + it('should warn for duplicated iterable keys with component stack info', () => { + spyOn(console, 'error'); + + var container = document.createElement('div'); + + class WrapperComponent extends React.Component { + render() { + return
{this.props.children}
; + } + } + + class Parent extends React.Component { + render() { + return ( +
+ + {this.props.children} + +
+ ); + } + } + + function createIterable(array) { + return { + '@@iterator': function() { + var i = 0; + return { + next() { + const next = { + value: i < array.length ? array[i] : undefined, + done: i === array.length, + }; + i++; + return next; + }, + }; + }, + }; + } + + ReactDOM.render( + {createIterable([
])}, + container + ); + + ReactDOM.render( + {createIterable([
,
])}, + container + ); + + expectDev(console.error.calls.count()).toBe(1); + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toContain( 'Encountered two children with the same key, `1`. ' + 'Child keys must be unique; when two children share a key, ' + 'only the first child will be used.\n' + From 8f544a7cc61c5e8e129931a3990ba5258152e608 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 14 Dec 2016 13:57:38 +0000 Subject: [PATCH 08/14] Move ReactTypeOfWork to src/shared It needs to be available both to Fiber and Isomorphic because the tree hook lives in Isomorphic but pretty-prints Fiber stack. --- src/{renderers/shared/fiber => shared}/ReactTypeOfWork.js | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/{renderers/shared/fiber => shared}/ReactTypeOfWork.js (100%) diff --git a/src/renderers/shared/fiber/ReactTypeOfWork.js b/src/shared/ReactTypeOfWork.js similarity index 100% rename from src/renderers/shared/fiber/ReactTypeOfWork.js rename to src/shared/ReactTypeOfWork.js From 6796336f3cf99b617e978862275387657d9f7aa2 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 14 Dec 2016 22:12:35 +0000 Subject: [PATCH 09/14] Add dev-only ReactDebugCurrentFiber for warnings The goal is to use ReactCurrentOwner less and rely on ReactDebugCurrentFiber for warning owner name and stack. --- scripts/fiber/tests-failing.txt | 1 - scripts/fiber/tests-passing-except-dev.txt | 1 - scripts/fiber/tests-passing.txt | 3 + .../hooks/ReactComponentTreeHook.js | 30 ++++++---- .../dom/fiber/ReactDOMFiberComponent.js | 8 +-- .../dom/fiber/wrappers/ReactDOMFiberInput.js | 12 ++-- .../dom/fiber/wrappers/ReactDOMFiberSelect.js | 7 +-- .../fiber/wrappers/ReactDOMFiberTextarea.js | 4 +- .../__tests__/ReactDOMComponent-test.js | 27 +++++++++ src/renderers/shared/fiber/ReactChildFiber.js | 10 ++-- .../shared/fiber/ReactDebugCurrentFiber.js | 55 +++++++++++++++++++ .../shared/fiber/ReactFiberBeginWork.js | 8 +++ .../shared/fiber/ReactFiberCompleteWork.js | 8 +++ .../shared/fiber/ReactFiberContext.js | 24 +++++--- .../shared/fiber/ReactFiberReconciler.js | 2 +- .../shared/fiber/ReactFiberScheduler.js | 10 ++++ .../shared/fiber/getCurrentOwnerName.js | 24 -------- src/shared/types/checkReactTypeSpec.js | 22 +++++--- 18 files changed, 178 insertions(+), 78 deletions(-) create mode 100644 src/renderers/shared/fiber/ReactDebugCurrentFiber.js delete mode 100644 src/renderers/shared/fiber/getCurrentOwnerName.js diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index d02b83955316..3ae03c52e5d7 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -21,7 +21,6 @@ src/renderers/dom/shared/__tests__/ReactDOM-test.js src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should warn for children on void elements -* should report component containing invalid styles * should clean up input value tracking * should clean up input textarea tracking * gives source code refs for unknown prop warning diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index 07a2dab9c724..6b32b2d69f01 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -12,7 +12,6 @@ src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should warn for unknown prop * should group multiple unknown prop warnings together * should warn for onDblClick prop -* should emit a warning once for a named custom component using shady DOM * should not warn when server-side rendering `onScroll` * warns on invalid nesting * warns on invalid nesting at root diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 1a98eff257bf..5cccab5d7b2c 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -653,6 +653,8 @@ src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should warn on upper case HTML tags, not SVG nor custom tags * should warn against children for void elements * should warn against dangerouslySetInnerHTML for void elements +* should include owner rather than parent in warnings +* should emit a warning once for a named custom component using shady DOM * should emit a warning once for an unnamed custom component using shady DOM * should treat menuitem as a void element but still create the closing tag * should validate against multiple children props @@ -671,6 +673,7 @@ src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should validate against multiple children props * should warn about contentEditable and children * should validate against invalid styles +* should report component containing invalid styles * should properly escape text content and attributes values * unmounts children before unsetting DOM node info * should warn about the `onScroll` issue when unsupported (IE8) diff --git a/src/isomorphic/hooks/ReactComponentTreeHook.js b/src/isomorphic/hooks/ReactComponentTreeHook.js index 0ef8a6d4a181..b7d253df86ea 100644 --- a/src/isomorphic/hooks/ReactComponentTreeHook.js +++ b/src/isomorphic/hooks/ReactComponentTreeHook.js @@ -353,8 +353,10 @@ var ReactComponentTreeHook = { var currentOwner = ReactCurrentOwner.current; if (currentOwner) { if (typeof currentOwner.tag === 'number') { - const fiber = ((currentOwner : any) : Fiber); - info += ReactComponentTreeHook.getStackAddendumByFiber(fiber); + const workInProgress = ((currentOwner : any) : Fiber); + // Safe because if current owner exists, we are reconciling, + // and it is guaranteed to be the work-in-progress version. + info += ReactComponentTreeHook.getStackAddendumByWorkInProgressFiber(workInProgress); } else if (typeof currentOwner._debugID === 'number') { info += ReactComponentTreeHook.getStackAddendumByID(currentOwner._debugID); } @@ -362,16 +364,6 @@ var ReactComponentTreeHook = { return info; }, - getStackAddendumByFiber(fiber : Fiber) : string { - var info = ''; - var node = fiber; - do { - info += describeFiber(node); - node = node.return; - } while (node); - return info; - }, - getStackAddendumByID(id: ?DebugID): string { var info = ''; while (id) { @@ -381,6 +373,20 @@ var ReactComponentTreeHook = { return info; }, + // This function can only be called with a work-in-progress fiber and + // only during begin or complete phase. Do not call it under any other + // circumstances. + getStackAddendumByWorkInProgressFiber(workInProgress : Fiber) : string { + var info = ''; + var node = workInProgress; + do { + info += describeFiber(node); + // Otherwise this return pointer might point to the wrong tree: + node = node.return; + } while (node); + return info; + }, + getChildIDs(id: DebugID): Array { var item = getItem(id); return item ? item.childIDs : []; diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 44afff611e87..2705fef7b59a 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -25,10 +25,10 @@ var ReactDOMFiberInput = require('ReactDOMFiberInput'); var ReactDOMFiberOption = require('ReactDOMFiberOption'); var ReactDOMFiberSelect = require('ReactDOMFiberSelect'); var ReactDOMFiberTextarea = require('ReactDOMFiberTextarea'); +var { getCurrentFiberOwnerName } = require('ReactDebugCurrentFiber'); var emptyFunction = require('emptyFunction'); var focusNode = require('focusNode'); -var getCurrentOwnerName = require('getCurrentOwnerName'); var invariant = require('invariant'); var isEventSupported = require('isEventSupported'); var setInnerHTML = require('setInnerHTML'); @@ -56,7 +56,7 @@ var DOC_FRAGMENT_TYPE = 11; function getDeclarationErrorAddendum() { - var ownerName = getCurrentOwnerName(); + var ownerName = getCurrentFiberOwnerName(); if (ownerName) { return ' This DOM node was rendered by `' + ownerName + '`.'; } @@ -436,7 +436,7 @@ function updateDOMProperties( CSSPropertyOperations.setValueForStyles( domElement, styleUpdates, - componentPlaceholder // TODO: Change CSSPropertyOperations to use getCurrentOwnerName. + componentPlaceholder // TODO: Change CSSPropertyOperations to use getCurrentFiberOwnerName. ); } } @@ -529,7 +529,7 @@ var ReactDOMFiberComponent = { false, '%s is using shady DOM. Using shady DOM with React can ' + 'cause things to break subtly.', - getCurrentOwnerName() || 'A component' + getCurrentFiberOwnerName() || 'A component' ); didWarnShadyDOM = true; } diff --git a/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js b/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js index 8b5b4121ac6b..0c34e13744a0 100644 --- a/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js +++ b/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js @@ -23,8 +23,8 @@ type InputWithWrapperState = HTMLInputElement & { var DOMPropertyOperations = require('DOMPropertyOperations'); var ReactControlledValuePropTypes = require('ReactControlledValuePropTypes'); var ReactDOMComponentTree = require('ReactDOMComponentTree'); +var { getCurrentFiberOwnerName } = require('ReactDebugCurrentFiber'); -var getCurrentOwnerName = require('getCurrentOwnerName'); var invariant = require('invariant'); var warning = require('warning'); @@ -86,7 +86,7 @@ var ReactDOMInput = { ReactControlledValuePropTypes.checkPropTypes( 'input', props, - getCurrentOwnerName() + getCurrentFiberOwnerName() ); if ( @@ -102,7 +102,7 @@ var ReactDOMInput = { 'both). Decide between using a controlled or uncontrolled input ' + 'element and remove one of these props. More info: ' + 'https://fb.me/react-controlled-components', - getCurrentOwnerName() || 'A component', + getCurrentFiberOwnerName() || 'A component', props.type ); didWarnCheckedDefaultChecked = true; @@ -120,7 +120,7 @@ var ReactDOMInput = { 'both). Decide between using a controlled or uncontrolled input ' + 'element and remove one of these props. More info: ' + 'https://fb.me/react-controlled-components', - getCurrentOwnerName() || 'A component', + getCurrentFiberOwnerName() || 'A component', props.type ); didWarnValueDefaultValue = true; @@ -151,7 +151,7 @@ var ReactDOMInput = { 'Input elements should not switch from uncontrolled to controlled (or vice versa). ' + 'Decide between using a controlled or uncontrolled input ' + 'element for the lifetime of the component. More info: https://fb.me/react-controlled-components', - getCurrentOwnerName() || 'A component', + getCurrentFiberOwnerName() || 'A component', props.type ); didWarnUncontrolledToControlled = true; @@ -163,7 +163,7 @@ var ReactDOMInput = { 'Input elements should not switch from controlled to uncontrolled (or vice versa). ' + 'Decide between using a controlled or uncontrolled input ' + 'element for the lifetime of the component. More info: https://fb.me/react-controlled-components', - getCurrentOwnerName() || 'A component', + getCurrentFiberOwnerName() || 'A component', props.type ); didWarnControlledToUncontrolled = true; diff --git a/src/renderers/dom/fiber/wrappers/ReactDOMFiberSelect.js b/src/renderers/dom/fiber/wrappers/ReactDOMFiberSelect.js index 9631fddcd920..195a0e610592 100644 --- a/src/renderers/dom/fiber/wrappers/ReactDOMFiberSelect.js +++ b/src/renderers/dom/fiber/wrappers/ReactDOMFiberSelect.js @@ -20,14 +20,13 @@ type SelectWithWrapperState = HTMLSelectElement & { }; var ReactControlledValuePropTypes = require('ReactControlledValuePropTypes'); - -var getCurrentOwnerName = require('getCurrentOwnerName'); +var { getCurrentFiberOwnerName } = require('ReactDebugCurrentFiber'); var warning = require('warning'); var didWarnValueDefaultValue = false; function getDeclarationErrorAddendum() { - var ownerName = getCurrentOwnerName(); + var ownerName = getCurrentFiberOwnerName(); if (ownerName) { return ' Check the render method of `' + ownerName + '`.'; } @@ -43,7 +42,7 @@ function checkSelectPropTypes(props) { ReactControlledValuePropTypes.checkPropTypes( 'select', props, - getCurrentOwnerName() + getCurrentFiberOwnerName() ); for (var i = 0; i < valuePropNames.length; i++) { diff --git a/src/renderers/dom/fiber/wrappers/ReactDOMFiberTextarea.js b/src/renderers/dom/fiber/wrappers/ReactDOMFiberTextarea.js index 34ec317fd106..49433ff5a17a 100644 --- a/src/renderers/dom/fiber/wrappers/ReactDOMFiberTextarea.js +++ b/src/renderers/dom/fiber/wrappers/ReactDOMFiberTextarea.js @@ -19,8 +19,8 @@ type TextAreaWithWrapperState = HTMLTextAreaElement & { }; var ReactControlledValuePropTypes = require('ReactControlledValuePropTypes'); +var { getCurrentFiberOwnerName } = require('ReactDebugCurrentFiber'); -var getCurrentOwnerName = require('getCurrentOwnerName'); var invariant = require('invariant'); var warning = require('warning'); @@ -69,7 +69,7 @@ var ReactDOMTextarea = { ReactControlledValuePropTypes.checkPropTypes( 'textarea', props, - getCurrentOwnerName() + getCurrentFiberOwnerName() ); if ( props.value !== undefined && diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 6c5661e6663d..4edced12c548 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -848,6 +848,33 @@ describe('ReactDOMComponent', () => { ); }); + it('should include owner rather than parent in warnings', () => { + var container = document.createElement('div'); + + function Parent(props) { + return props.children; + } + function Owner() { + // We're using the input dangerouslySetInnerHTML invariant but the + // exact error doesn't matter as long as we have a way to verify + // that warnings and invariants contain owner rather than parent name. + return ( + + + + ); + } + + expect(function() { + ReactDOM.render( + , + container + ); + }).toThrowError( + 'This DOM node was rendered by `Owner`.' + ); + }); + it('should emit a warning once for a named custom component using shady DOM', () => { if (ReactDOMFeatureFlags.useCreateElement) { spyOn(console, 'error'); diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 33c19b362317..028490ee83cd 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -38,8 +38,7 @@ var getIteratorFn = require('getIteratorFn'); var invariant = require('invariant'); if (__DEV__) { - var ReactComponentTreeHook = require('ReactComponentTreeHook'); - var { getStackAddendumByFiber } = ReactComponentTreeHook; + var { getCurrentFiberStackAddendum } = require('ReactDebugCurrentFiber'); var warning = require('warning'); } @@ -549,7 +548,6 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { function warnOnDuplicateKey( child : mixed, - returnFiber : Fiber, knownKeys : Set | null ) : Set | null { if (__DEV__) { @@ -580,7 +578,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { '`%s`. Child keys must be unique; when two children share a key, ' + 'only the first child will be used.%s', key, - getStackAddendumByFiber(returnFiber) + getCurrentFiberStackAddendum() ); break; default: @@ -620,7 +618,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { let knownKeys = null; for (let i = 0; i < newChildren.length; i++) { const child = newChildren[i]; - knownKeys = warnOnDuplicateKey(child, returnFiber, knownKeys); + knownKeys = warnOnDuplicateKey(child, knownKeys); } } @@ -776,7 +774,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { let step = newChildren.next(); for (; !step.done; step = newChildren.next()) { const child = step.value; - knownKeys = warnOnDuplicateKey(child, returnFiber, knownKeys); + knownKeys = warnOnDuplicateKey(child, knownKeys); } } diff --git a/src/renderers/shared/fiber/ReactDebugCurrentFiber.js b/src/renderers/shared/fiber/ReactDebugCurrentFiber.js new file mode 100644 index 000000000000..6b25a05ce032 --- /dev/null +++ b/src/renderers/shared/fiber/ReactDebugCurrentFiber.js @@ -0,0 +1,55 @@ +/** + * 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 ReactDebugCurrentFiber + * @flow + */ + +'use strict'; + +import type { Fiber } from 'ReactFiber'; + +if (__DEV__) { + var getComponentName = require('getComponentName'); + var { getStackAddendumByWorkInProgressFiber } = require('ReactComponentTreeHook'); +} + +function getCurrentFiberOwnerName() : string | null { + if (__DEV__) { + const fiber = ReactDebugCurrentFiber.current; + if (fiber == null) { + return null; + } + if (fiber._debugOwner == null) { + return null; + } + return getComponentName(fiber._debugOwner); + } + return null; +} + +function getCurrentFiberStackAddendum() : string | null { + if (__DEV__) { + const fiber = ReactDebugCurrentFiber.current; + if (fiber == null) { + return null; + } + // Safe because if current fiber exists, we are reconciling, + // and it is guaranteed to be the work-in-progress version. + return getStackAddendumByWorkInProgressFiber(fiber); + } + return null; +} + +var ReactDebugCurrentFiber = { + current: (null : Fiber | null), + getCurrentFiberOwnerName, + getCurrentFiberStackAddendum, +}; + +module.exports = ReactDebugCurrentFiber; diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index ca68c0d01bab..bc98949e6fce 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -60,6 +60,10 @@ var { var ReactCurrentOwner = require('ReactCurrentOwner'); var ReactFiberClassComponent = require('ReactFiberClassComponent'); +if (__DEV__) { + var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); +} + module.exports = function( config : HostConfig, hostContext : HostContext, @@ -452,6 +456,10 @@ module.exports = function( return bailoutOnLowPriority(current, workInProgress); } + if (__DEV__) { + ReactDebugCurrentFiber.current = workInProgress; + } + // If we don't bail out, we're going be recomputing our children so we need // to drop our effect list. workInProgress.firstEffect = null; diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index b8ffe3132f0f..d0a6ab217d75 100644 --- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js +++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js @@ -44,6 +44,10 @@ var { Callback, } = ReactTypeOfSideEffect; +if (__DEV__) { + var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); +} + module.exports = function( config : HostConfig, hostContext : HostContext, @@ -167,6 +171,10 @@ module.exports = function( } function completeWork(current : ?Fiber, workInProgress : Fiber) : ?Fiber { + if (__DEV__) { + ReactDebugCurrentFiber.current = workInProgress; + } + switch (workInProgress.tag) { case FunctionalComponent: workInProgress.memoizedProps = workInProgress.pendingProps; diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index 83964f01b3e1..76e8ed568f62 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -40,8 +40,8 @@ function getUnmaskedContext() { return contextStack[index]; } -exports.getMaskedContext = function(fiber : Fiber) { - const type = fiber.type; +exports.getMaskedContext = function(workInProgress : Fiber) { + const type = workInProgress.type; const contextTypes = type.contextTypes; if (!contextTypes) { return emptyObject; @@ -54,8 +54,8 @@ exports.getMaskedContext = function(fiber : Fiber) { } if (__DEV__) { - const name = getComponentName(fiber); - checkReactTypeSpec(contextTypes, context, 'context', name, null, fiber); + const name = getComponentName(workInProgress); + checkReactTypeSpec(contextTypes, context, 'context', name, null, workInProgress); } return context; @@ -90,7 +90,7 @@ exports.pushTopLevelContextObject = function(context : Object, didChange : boole didPerformWorkStack[index] = didChange; }; -function processChildContext(fiber : Fiber, parentContext : Object): Object { +function processChildContext(fiber : Fiber, parentContext : Object, isReconciling : boolean): Object { const instance = fiber.stateNode; const childContextTypes = fiber.type.childContextTypes; const childContext = instance.getChildContext(); @@ -104,14 +104,20 @@ function processChildContext(fiber : Fiber, parentContext : Object): Object { } if (__DEV__) { const name = getComponentName(fiber); - checkReactTypeSpec(childContextTypes, childContext, 'childContext', name, null, fiber); + // We can only provide accurate element stacks if we pass work-in-progress tree + // during the begin or complete phase. However currently this function is also + // called from unstable_renderSubtree legacy implementation. In this case it unsafe to + // assume anything about the given fiber. We won't pass it down if we aren't sure. + // TODO: remove this hack when we delete unstable_renderSubtree in Fiber. + const workInProgress = isReconciling ? fiber : null; + checkReactTypeSpec(childContextTypes, childContext, 'childContext', name, null, workInProgress); } return {...parentContext, ...childContext}; } exports.processChildContext = processChildContext; -exports.pushContextProvider = function(fiber : Fiber, didPerformWork : boolean) : void { - const instance = fiber.stateNode; +exports.pushContextProvider = function(workInProgress : Fiber, didPerformWork : boolean) : void { + const instance = workInProgress.stateNode; const memoizedMergedChildContext = instance.__reactInternalMemoizedMergedChildContext; const canReuseMergedChildContext = !didPerformWork && memoizedMergedChildContext != null; @@ -119,7 +125,7 @@ exports.pushContextProvider = function(fiber : Fiber, didPerformWork : boolean) if (canReuseMergedChildContext) { mergedContext = memoizedMergedChildContext; } else { - mergedContext = processChildContext(fiber, getUnmaskedContext()); + mergedContext = processChildContext(workInProgress, getUnmaskedContext(), true); instance.__reactInternalMemoizedMergedChildContext = mergedContext; } diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index 571d9aa921d1..582d7946e322 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -91,7 +91,7 @@ export type Reconciler = { getContextForSubtree._injectFiber(function(fiber : Fiber) { const parentContext = findCurrentUnmaskedContext(fiber); return isContextProvider(fiber) ? - processChildContext(fiber, parentContext) : + processChildContext(fiber, parentContext, false) : parentContext; }); diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 4d50ac0a41f1..0dfcf048b03a 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -59,6 +59,7 @@ var { if (__DEV__) { var ReactFiberInstrumentation = require('ReactFiberInstrumentation'); + var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); } var timeHeuristicForUnitOfWork = 1; @@ -478,6 +479,9 @@ module.exports = function(config : HostConfig(config : HostConfig(config : HostConfig Date: Wed, 14 Dec 2016 17:00:04 +0000 Subject: [PATCH 10/14] Make Stack invariant messages more consistent Fiber used a helper so two tests had the same phrasing. Stack also used a helper for most invariants but hardcoded a different phrase in one place. I changed that invariant message to use a helper which made it consistent with what it prints in Fiber. --- scripts/fiber/tests-failing.txt | 1 - scripts/fiber/tests-passing.txt | 1 + src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js | 2 +- src/renderers/dom/stack/client/ReactDOMComponent.js | 5 +---- 4 files changed, 3 insertions(+), 6 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 3ae03c52e5d7..0fdada5f4448 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -20,7 +20,6 @@ src/renderers/dom/shared/__tests__/ReactDOM-test.js * throws in render() if the update callback is not a function src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js -* should warn for children on void elements * should clean up input value tracking * should clean up input textarea tracking * gives source code refs for unknown prop warning diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 5cccab5d7b2c..2ead19a35b44 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -667,6 +667,7 @@ src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should validate against invalid styles * should track input values * should track textarea values +* should warn for children on void elements * should support custom elements which extend native elements * should warn against children for void elements * should warn against dangerouslySetInnerHTML for void elements diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 4edced12c548..af0b439be0fb 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1048,7 +1048,7 @@ describe('ReactDOMComponent', () => { ReactDOM.render(, container); }).toThrowError( 'input is a void element tag and must neither have `children` ' + - 'nor use `dangerouslySetInnerHTML`. Check the render method of X.' + 'nor use `dangerouslySetInnerHTML`. This DOM node was rendered by `X`.' ); }); diff --git a/src/renderers/dom/stack/client/ReactDOMComponent.js b/src/renderers/dom/stack/client/ReactDOMComponent.js index b23a736b083c..826a95360ec8 100644 --- a/src/renderers/dom/stack/client/ReactDOMComponent.js +++ b/src/renderers/dom/stack/client/ReactDOMComponent.js @@ -88,10 +88,7 @@ function assertValidProps(component, props) { '%s is a void element tag and must neither have `children` nor ' + 'use `dangerouslySetInnerHTML`.%s', component._tag, - component._currentElement._owner ? - ' Check the render method of ' + - component._currentElement._owner.getName() + '.' : - '' + getDeclarationErrorAddendum(component) ); } if (props.dangerouslySetInnerHTML != null) { From 7566a14b5911e6627bcbf4bdc3844807732a620c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 15 Dec 2016 15:40:48 +0000 Subject: [PATCH 11/14] Make CSSPropertyOperations use getCurrentFiberOwnerName() This gets mount-time CSS warnings to be printed. However update-time warnings are currently ignored because current fiber is not yet available during the commit phase. We also regress on HostOperation hook tests but this doesn't matter because it's only used by ReactPerf and it doesn't work with Fiber yet anyway. We'll have to think more about it later. --- scripts/fiber/tests-passing-except-dev.txt | 6 ++-- scripts/fiber/tests-passing.txt | 6 ++-- .../dom/fiber/ReactDOMFiberComponent.js | 13 ++------ .../dom/shared/CSSPropertyOperations.js | 30 ++++++++++--------- .../dom/stack/client/ReactDOMComponent.js | 7 +++++ 5 files changed, 31 insertions(+), 31 deletions(-) diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index 6b32b2d69f01..e016ef670fc7 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -2,11 +2,7 @@ src/addons/transitions/__tests__/ReactTransitionGroup-test.js * should warn for duplicated keys with component stack info src/renderers/dom/shared/__tests__/CSSPropertyOperations-test.js -* should warn when using hyphenated style names * should warn when updating hyphenated style names -* warns when miscapitalizing vendored style names -* should warn about style having a trailing semicolon -* should warn about style containing a NaN value src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should warn for unknown prop @@ -117,6 +113,8 @@ src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js * gets recorded for composite roots * gets recorded when a native is mounted deeply instead of null * gets recorded during mount +* gets recorded during an update +* gets ignored if the styles are shallowly equal * gets recorded during mount * gets recorded during mount * gets recorded during mount diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 2ead19a35b44..a2d5956313ef 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -552,6 +552,10 @@ src/renderers/dom/shared/__tests__/CSSPropertyOperations-test.js * should create vendor-prefixed markup correctly * should set style attribute when styles exist * should not set style attribute when no styles exist +* should warn when using hyphenated style names +* warns when miscapitalizing vendored style names +* should warn about style having a trailing semicolon +* should warn about style containing a NaN value src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js * should create markup for simple properties @@ -1273,8 +1277,6 @@ src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.native.js src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js * gets ignored for composite roots that return null * gets recorded during an update -* gets ignored if the styles are shallowly equal -* gets recorded during an update * gets recorded as a removal during an update * gets recorded during an update * gets recorded during an update diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 2705fef7b59a..aed02f2b84a2 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -20,7 +20,6 @@ var DOMProperty = require('DOMProperty'); var DOMPropertyOperations = require('DOMPropertyOperations'); var EventPluginRegistry = require('EventPluginRegistry'); var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter'); -var ReactDOMComponentTree = require('ReactDOMComponentTree'); var ReactDOMFiberInput = require('ReactDOMFiberInput'); var ReactDOMFiberOption = require('ReactDOMFiberOption'); var ReactDOMFiberSelect = require('ReactDOMFiberSelect'); @@ -58,6 +57,7 @@ var DOC_FRAGMENT_TYPE = 11; function getDeclarationErrorAddendum() { var ownerName = getCurrentFiberOwnerName(); if (ownerName) { + // TODO: also report the stack. return ' This DOM node was rendered by `' + ownerName + '`.'; } return ''; @@ -424,19 +424,10 @@ function updateDOMProperties( } } if (styleUpdates) { - var componentPlaceholder = null; - if (__DEV__) { - // HACK - var internalInstance = ReactDOMComponentTree.getInstanceFromNode(domElement); - componentPlaceholder = { - _currentElement: { type: internalInstance.type, props: internalInstance.memoizedProps }, - _debugID: internalInstance._debugID, - }; - } + // TODO: call ReactInstrumentation.debugTool.onHostOperation in DEV. CSSPropertyOperations.setValueForStyles( domElement, styleUpdates, - componentPlaceholder // TODO: Change CSSPropertyOperations to use getCurrentFiberOwnerName. ); } } diff --git a/src/renderers/dom/shared/CSSPropertyOperations.js b/src/renderers/dom/shared/CSSPropertyOperations.js index 4843ed0dac59..c970007bf7a6 100644 --- a/src/renderers/dom/shared/CSSPropertyOperations.js +++ b/src/renderers/dom/shared/CSSPropertyOperations.js @@ -13,7 +13,6 @@ var CSSProperty = require('CSSProperty'); var ExecutionEnvironment = require('ExecutionEnvironment'); -var ReactInstrumentation = require('ReactInstrumentation'); var camelizeStyleName = require('camelizeStyleName'); var dangerousStyleValue = require('dangerousStyleValue'); @@ -22,6 +21,10 @@ var hyphenateStyleName = require('hyphenateStyleName'); var memoizeStringOnly = require('memoizeStringOnly'); var warning = require('warning'); +if (__DEV__) { + var { getCurrentFiberOwnerName } = require('ReactDebugCurrentFiber'); +} + var processStyleName = memoizeStringOnly(function(styleName) { return hyphenateStyleName(styleName); }); @@ -114,11 +117,18 @@ if (__DEV__) { }; var checkRenderMessage = function(owner) { - if (owner) { - var name = getComponentName(owner); - if (name) { - return ' Check the render method of `' + name + '`.'; - } + var ownerName; + if (owner != null) { + // Stack passes the owner manually all the way to CSSPropertyOperations. + ownerName = getComponentName(owner); + } else { + // Fiber doesn't pass it but uses ReactDebugCurrentFiber to track it. + // It is only enabled in development and tracks host components too. + ownerName = getCurrentFiberOwnerName(); + // TODO: also report the stack. + } + if (ownerName) { + return ' Check the render method of `' + ownerName + '`.'; } return ''; }; @@ -193,14 +203,6 @@ var CSSPropertyOperations = { * @param {ReactDOMComponent} component */ setValueForStyles: function(node, styles, component) { - if (__DEV__) { - ReactInstrumentation.debugTool.onHostOperation({ - instanceID: component._debugID, - type: 'update styles', - payload: styles, - }); - } - var style = node.style; for (var styleName in styles) { if (!styles.hasOwnProperty(styleName)) { diff --git a/src/renderers/dom/stack/client/ReactDOMComponent.js b/src/renderers/dom/stack/client/ReactDOMComponent.js index 826a95360ec8..fbfd8ccf0ad8 100644 --- a/src/renderers/dom/stack/client/ReactDOMComponent.js +++ b/src/renderers/dom/stack/client/ReactDOMComponent.js @@ -1026,6 +1026,13 @@ ReactDOMComponent.Mixin = { } } if (styleUpdates) { + if (__DEV__) { + ReactInstrumentation.debugTool.onHostOperation({ + instanceID: this._debugID, + type: 'update styles', + payload: styleUpdates, + }); + } CSSPropertyOperations.setValueForStyles( getNode(this), styleUpdates, From 080330ab9c70c222246e5a5e256ec6b833feebdf Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 15 Dec 2016 16:03:43 +0000 Subject: [PATCH 12/14] Set ReactDebugCurrentFiber during the commit phase This makes it available during updates, fixing the last failing test in CSSPropertyOperations. --- scripts/fiber/tests-passing-except-dev.txt | 3 --- scripts/fiber/tests-passing.txt | 1 + src/renderers/shared/fiber/ReactFiberScheduler.js | 8 ++++++++ 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index e016ef670fc7..bef269b327aa 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -1,9 +1,6 @@ src/addons/transitions/__tests__/ReactTransitionGroup-test.js * should warn for duplicated keys with component stack info -src/renderers/dom/shared/__tests__/CSSPropertyOperations-test.js -* should warn when updating hyphenated style names - src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should warn for unknown prop * should group multiple unknown prop warnings together diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index a2d5956313ef..3893dd78dc03 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -553,6 +553,7 @@ src/renderers/dom/shared/__tests__/CSSPropertyOperations-test.js * should set style attribute when styles exist * should not set style attribute when no styles exist * should warn when using hyphenated style names +* should warn when updating hyphenated style names * warns when miscapitalizing vendored style names * should warn about style having a trailing semicolon * should warn about style containing a NaN value diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 0dfcf048b03a..7bf2f7d7aa56 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -187,6 +187,10 @@ module.exports = function(config : HostConfig(config : HostConfig Date: Wed, 14 Dec 2016 23:17:37 +0000 Subject: [PATCH 13/14] Add DOM warnings by calling hooks directly It is not clear if the old hook system is worth it in its generic incarnation. For now I am just hooking it up to the DOMFiber renderer directly. --- scripts/fiber/tests-passing-except-dev.txt | 22 --------- scripts/fiber/tests-passing.txt | 14 ++++++ .../dom/fiber/ReactDOMFiberComponent.js | 23 +++++++++ .../shared/hooks/ReactDOMInvalidARIAHook.js | 48 +++++++++++------- .../hooks/ReactDOMNullInputValuePropHook.js | 33 +++++++++---- .../hooks/ReactDOMUnknownPropertyHook.js | 49 ++++++++++++------- 6 files changed, 122 insertions(+), 67 deletions(-) diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index bef269b327aa..2bc48ccba95c 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -2,9 +2,6 @@ src/addons/transitions/__tests__/ReactTransitionGroup-test.js * should warn for duplicated keys with component stack info src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js -* should warn for unknown prop -* should group multiple unknown prop warnings together -* should warn for onDblClick prop * should not warn when server-side rendering `onScroll` * warns on invalid nesting * warns on invalid nesting at root @@ -12,15 +9,7 @@ src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * gives useful context in warnings * should warn about incorrect casing on properties (ssr) * should warn about incorrect casing on event handlers (ssr) -* should warn about incorrect casing on properties -* should warn about incorrect casing on event handlers * should warn about class -* should suggest property name if available - -src/renderers/dom/shared/__tests__/ReactDOMInvalidARIAHook-test.js -* should warn for one invalid aria-* prop -* should warn for many invalid aria-* props -* should warn for an improperly cased aria-* prop src/renderers/dom/shared/__tests__/ReactMount-test.js * should warn if mounting into dirty rendered markup @@ -36,17 +25,6 @@ src/renderers/dom/shared/__tests__/ReactMountDestruction-test.js src/renderers/dom/shared/__tests__/ReactServerRendering-test.js * should have the correct mounting behavior -src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js -* should warn if value is null -* should warn if controlled input switches to uncontrolled (value is null) -* should warn if uncontrolled input (value is null) switches to controlled - -src/renderers/dom/shared/wrappers/__tests__/ReactDOMSelect-test.js -* should warn if value is null - -src/renderers/dom/shared/wrappers/__tests__/ReactDOMTextarea-test.js -* should warn if value is null - src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js * uses displayName or Unknown for classic components * uses displayName, name, or ReactComponent for modern components diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 3893dd78dc03..a5a4c0f2a1a3 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -621,6 +621,9 @@ src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should gracefully handle various style value types * should not update styles when mutating a proxy style object * should throw when mutating style objectsd +* should warn for unknown prop +* should group multiple unknown prop warnings together +* should warn for onDblClick prop * should not warn for "0" as a unitless style value * should warn nicely about NaN in style * should update styles if initially null @@ -687,7 +690,10 @@ src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should throw when an attack vector is used server-side * should throw when an invalid tag name is used * should throw when an attack vector is used +* should warn about incorrect casing on properties +* should warn about incorrect casing on event handlers * should warn about props that are no longer supported +* should suggest property name if available src/renderers/dom/shared/__tests__/ReactDOMComponentTree-test.js * finds nodes for instances @@ -698,6 +704,9 @@ src/renderers/dom/shared/__tests__/ReactDOMIDOperations-test.js src/renderers/dom/shared/__tests__/ReactDOMInvalidARIAHook-test.js * should allow valid aria-* props +* should warn for one invalid aria-* prop +* should warn for many invalid aria-* props +* should warn for an improperly cased aria-* prop src/renderers/dom/shared/__tests__/ReactDOMSVG-test.js * creates initial namespaced markup @@ -951,11 +960,14 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js * should have a this value of undefined if bind is not used * should warn with checked and no onChange handler with readOnly specified * should update defaultValue to empty string +* should warn if value is null * should warn if checked and defaultChecked props are specified * should warn if value and defaultValue props are specified * should warn if controlled input switches to uncontrolled (value is undefined) +* should warn if controlled input switches to uncontrolled (value is null) * should warn if controlled input switches to uncontrolled with defaultValue * should warn if uncontrolled input (value is undefined) switches to controlled +* should warn if uncontrolled input (value is null) switches to controlled * should warn if controlled checkbox switches to uncontrolled (checked is undefined) * should warn if controlled checkbox switches to uncontrolled (checked is null) * should warn if controlled checkbox switches to uncontrolled with defaultChecked @@ -1001,6 +1013,7 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMSelect-test.js * should support server-side rendering with defaultValue * should support server-side rendering with multiple * should not control defaultValue if readding options +* should warn if value is null * should refresh state on change * should warn if value and defaultValue props are specified * should be able to safely remove select onChange @@ -1033,6 +1046,7 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMTextarea-test.js * should allow objects as children * should throw with multiple or invalid children * should unmount +* should warn if value is null * should warn if value and defaultValue are specified src/renderers/native/__tests__/ReactNativeAttributePayload-test.js diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index aed02f2b84a2..497a3f648d50 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -34,6 +34,16 @@ var setInnerHTML = require('setInnerHTML'); var setTextContent = require('setTextContent'); var inputValueTracking = require('inputValueTracking'); var warning = require('warning'); + +if (__DEV__) { + var ReactDOMInvalidARIAHook = require('ReactDOMInvalidARIAHook'); + var ReactDOMNullInputValuePropHook = require('ReactDOMNullInputValuePropHook'); + var ReactDOMUnknownPropertyHook = require('ReactDOMUnknownPropertyHook'); + var { validateProperties: validateARIAProperties } = ReactDOMInvalidARIAHook; + var { validateProperties: validateInputPropertes } = ReactDOMNullInputValuePropHook; + var { validateProperties: validateUnknownPropertes } = ReactDOMUnknownPropertyHook; +} + var didWarnShadyDOM = false; var listenTo = ReactBrowserEventEmitter.listenTo; @@ -122,6 +132,14 @@ function assertValidProps(tag : string, props : ?Object) { ); } +if (__DEV__) { + var validatePropertiesInDevelopment = function(type, props) { + validateARIAProperties(type, props); + validateInputPropertes(type, props); + validateUnknownPropertes(type, props); + }; +} + function ensureListeningTo(rootContainerElement, registrationName) { if (__DEV__) { // IE8 has no API for event capturing and the `onScroll` event doesn't @@ -515,6 +533,7 @@ var ReactDOMFiberComponent = { var isCustomComponentTag = isCustomComponent(tag, rawProps); if (__DEV__) { + validatePropertiesInDevelopment(tag, rawProps); if (isCustomComponentTag && !didWarnShadyDOM && domElement.shadyRoot) { warning( false, @@ -632,6 +651,10 @@ var ReactDOMFiberComponent = { nextRawProps : Object, rootContainerElement : Element ) : void { + if (__DEV__) { + validatePropertiesInDevelopment(tag, nextRawProps); + } + var lastProps : Object; var nextProps : Object; switch (tag) { diff --git a/src/renderers/dom/shared/hooks/ReactDOMInvalidARIAHook.js b/src/renderers/dom/shared/hooks/ReactDOMInvalidARIAHook.js index 6e2fdcfa363e..a470e9afec24 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMInvalidARIAHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMInvalidARIAHook.js @@ -13,12 +13,23 @@ var DOMProperty = require('DOMProperty'); var ReactComponentTreeHook = require('ReactComponentTreeHook'); +var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); var warning = require('warning'); var warnedProperties = {}; var rARIA = new RegExp('^(aria)-[' + DOMProperty.ATTRIBUTE_NAME_CHAR + ']*$'); +function getStackAddendum(debugID) { + if (debugID != null) { + // This can only happen on Stack + return ReactComponentTreeHook.getStackAddendumByID(debugID); + } else { + // This can only happen on Fiber + return ReactDebugCurrentFiber.getCurrentFiberStackAddendum(); + } +} + function validateProperty(tagName, name, debugID) { if ( warnedProperties.hasOwnProperty(name) @@ -47,7 +58,7 @@ function validateProperty(tagName, name, debugID) { 'Unknown ARIA attribute %s. Did you mean %s?%s', name, standardName, - ReactComponentTreeHook.getStackAddendumByID(debugID) + getStackAddendum(debugID) ); warnedProperties[name] = true; return true; @@ -57,11 +68,11 @@ function validateProperty(tagName, name, debugID) { return true; } -function warnInvalidARIAProps(debugID, element) { +function warnInvalidARIAProps(type, props, debugID) { const invalidProps = []; - for (var key in element.props) { - var isValid = validateProperty(element.type, key, debugID); + for (var key in props) { + var isValid = validateProperty(type, key, debugID); if (!isValid) { invalidProps.push(key); } @@ -77,8 +88,8 @@ function warnInvalidARIAProps(debugID, element) { 'Invalid aria prop %s on <%s> tag. ' + 'For details, see https://fb.me/invalid-aria-prop%s', unknownPropString, - element.type, - ReactComponentTreeHook.getStackAddendumByID(debugID) + type, + getStackAddendum(debugID) ); } else if (invalidProps.length > 1) { warning( @@ -86,32 +97,31 @@ function warnInvalidARIAProps(debugID, element) { 'Invalid aria props %s on <%s> tag. ' + 'For details, see https://fb.me/invalid-aria-prop%s', unknownPropString, - element.type, - ReactComponentTreeHook.getStackAddendumByID(debugID) + type, + getStackAddendum(debugID) ); } } -function handleElement(debugID, element) { - if (element == null || typeof element.type !== 'string') { +function validateProperties(type, props, debugID /* Stack only */) { + if (type.indexOf('-') >= 0 || props.is) { return; } - if (element.type.indexOf('-') >= 0 || element.props.is) { - return; - } - - warnInvalidARIAProps(debugID, element); + warnInvalidARIAProps(type, props, debugID); } var ReactDOMInvalidARIAHook = { + // Fiber + validateProperties, + // Stack onBeforeMountComponent(debugID, element) { - if (__DEV__) { - handleElement(debugID, element); + if (__DEV__ && element != null && typeof element.type === 'string') { + validateProperties(element.type, element.props, debugID); } }, onBeforeUpdateComponent(debugID, element) { - if (__DEV__) { - handleElement(debugID, element); + if (__DEV__ && element != null && typeof element.type === 'string') { + validateProperties(element.type, element.props, debugID); } }, }; diff --git a/src/renderers/dom/shared/hooks/ReactDOMNullInputValuePropHook.js b/src/renderers/dom/shared/hooks/ReactDOMNullInputValuePropHook.js index 959db6bf26cc..564f6e1abd7e 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMNullInputValuePropHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMNullInputValuePropHook.js @@ -12,26 +12,34 @@ 'use strict'; var ReactComponentTreeHook = require('ReactComponentTreeHook'); +var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); var warning = require('warning'); var didWarnValueNull = false; -function handleElement(debugID, element) { - if (element == null) { - return; +function getStackAddendum(debugID) { + if (debugID != null) { + // This can only happen on Stack + return ReactComponentTreeHook.getStackAddendumByID(debugID); + } else { + // This can only happen on Fiber + return ReactDebugCurrentFiber.getCurrentFiberStackAddendum(); } - if (element.type !== 'input' && element.type !== 'textarea' && element.type !== 'select') { +} + +function validateProperties(type, props, debugID /* Stack only */) { + if (type !== 'input' && type !== 'textarea' && type !== 'select') { return; } - if (element.props != null && element.props.value === null && !didWarnValueNull) { + if (props != null && props.value === null && !didWarnValueNull) { warning( false, '`value` prop on `%s` should not be null. ' + 'Consider using the empty string to clear the component or `undefined` ' + 'for uncontrolled components.%s', - element.type, - ReactComponentTreeHook.getStackAddendumByID(debugID) + type, + getStackAddendum(debugID) ); didWarnValueNull = true; @@ -39,11 +47,18 @@ function handleElement(debugID, element) { } var ReactDOMNullInputValuePropHook = { + // Fiber + validateProperties, + // Stack onBeforeMountComponent(debugID, element) { - handleElement(debugID, element); + if (__DEV__ && element != null && typeof element.type === 'string') { + validateProperties(element.type, element.props, debugID); + } }, onBeforeUpdateComponent(debugID, element) { - handleElement(debugID, element); + if (__DEV__ && element != null && typeof element.type === 'string') { + validateProperties(element.type, element.props, debugID); + } }, }; diff --git a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js index 5f409cddb376..6462875f6591 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js @@ -14,9 +14,20 @@ var DOMProperty = require('DOMProperty'); var EventPluginRegistry = require('EventPluginRegistry'); var ReactComponentTreeHook = require('ReactComponentTreeHook'); +var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); var warning = require('warning'); +function getStackAddendum(debugID) { + if (debugID != null) { + // This can only happen on Stack + return ReactComponentTreeHook.getStackAddendumByID(debugID); + } else { + // This can only happen on Fiber + return ReactDebugCurrentFiber.getCurrentFiberStackAddendum(); + } +} + if (__DEV__) { var reactProps = { children: true, @@ -71,7 +82,7 @@ if (__DEV__) { 'Unknown DOM property %s. Did you mean %s?%s', name, standardName, - ReactComponentTreeHook.getStackAddendumByID(debugID) + getStackAddendum(debugID) ); return true; } else if (registrationName != null) { @@ -80,7 +91,7 @@ if (__DEV__) { 'Unknown event handler property %s. Did you mean `%s`?%s', name, registrationName, - ReactComponentTreeHook.getStackAddendumByID(debugID) + getStackAddendum(debugID) ); return true; } else { @@ -93,10 +104,10 @@ if (__DEV__) { }; } -var warnUnknownProperties = function(debugID, element) { +var warnUnknownProperties = function(type, props, debugID) { var unknownProps = []; - for (var key in element.props) { - var isValid = validateProperty(element.type, key, debugID); + for (var key in props) { + var isValid = validateProperty(type, key, debugID); if (!isValid) { unknownProps.push(key); } @@ -112,8 +123,8 @@ var warnUnknownProperties = function(debugID, element) { 'Unknown prop %s on <%s> tag. Remove this prop from the element. ' + 'For details, see https://fb.me/react-unknown-prop%s', unknownPropString, - element.type, - ReactComponentTreeHook.getStackAddendumByID(debugID) + type, + getStackAddendum(debugID) ); } else if (unknownProps.length > 1) { warning( @@ -121,28 +132,32 @@ var warnUnknownProperties = function(debugID, element) { 'Unknown props %s on <%s> tag. Remove these props from the element. ' + 'For details, see https://fb.me/react-unknown-prop%s', unknownPropString, - element.type, - ReactComponentTreeHook.getStackAddendumByID(debugID) + type, + getStackAddendum(debugID) ); } }; -function handleElement(debugID, element) { - if (element == null || typeof element.type !== 'string') { +function validateProperties(type, props, debugID /* Stack only */) { + if (type.indexOf('-') >= 0 || props.is) { return; } - if (element.type.indexOf('-') >= 0 || element.props.is) { - return; - } - warnUnknownProperties(debugID, element); + warnUnknownProperties(type, props, debugID); } var ReactDOMUnknownPropertyHook = { + // Fiber + validateProperties, + // Stack onBeforeMountComponent(debugID, element) { - handleElement(debugID, element); + if (__DEV__ && element != null && typeof element.type === 'string') { + validateProperties(element.type, element.props, debugID); + } }, onBeforeUpdateComponent(debugID, element) { - handleElement(debugID, element); + if (__DEV__ && element != null && typeof element.type === 'string') { + validateProperties(element.type, element.props, debugID); + } }, }; From 837e6832fe4707a8f47ab1eaae545ee1db77d03f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 15 Dec 2016 16:26:37 +0000 Subject: [PATCH 14/14] Add client-side counterparts for some warning tests This helps us track which warnings are really failing in Fiber, and which ones depend on SSR. --- scripts/fiber/tests-failing.txt | 7 +- scripts/fiber/tests-passing-except-dev.txt | 3 +- scripts/fiber/tests-passing.txt | 6 + .../__tests__/ReactDOMComponent-test.js | 140 +++++++++++++++++- 4 files changed, 147 insertions(+), 9 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 0fdada5f4448..034f765d97a4 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -22,10 +22,9 @@ src/renderers/dom/shared/__tests__/ReactDOM-test.js src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should clean up input value tracking * should clean up input textarea tracking -* gives source code refs for unknown prop warning -* gives source code refs for unknown prop warning for update render -* gives source code refs for unknown prop warning for exact elements -* gives source code refs for unknown prop warning for exact elements in composition +* gives source code refs for unknown prop warning (ssr) +* gives source code refs for unknown prop warning for exact elements (ssr) +* gives source code refs for unknown prop warning for exact elements in composition (ssr) src/renderers/dom/shared/__tests__/ReactDOMTextComponent-test.js * can reconcile text merged by Node.normalize() alongside other elements diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index 2bc48ccba95c..6464c7d7ee54 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -9,7 +9,8 @@ src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * gives useful context in warnings * should warn about incorrect casing on properties (ssr) * should warn about incorrect casing on event handlers (ssr) -* should warn about class +* should warn about class (ssr) +* should suggest property name if available (ssr) src/renderers/dom/shared/__tests__/ReactMount-test.js * should warn if mounting into dirty rendered markup diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index a5a4c0f2a1a3..e4e9a496b724 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -692,7 +692,13 @@ src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should throw when an attack vector is used * should warn about incorrect casing on properties * should warn about incorrect casing on event handlers +* should warn about class * should warn about props that are no longer supported +* should warn about props that are no longer supported (ssr) +* gives source code refs for unknown prop warning +* gives source code refs for unknown prop warning for update render +* gives source code refs for unknown prop warning for exact elements +* gives source code refs for unknown prop warning for exact elements in composition * should suggest property name if available src/renderers/dom/shared/__tests__/ReactDOMComponentTree-test.js diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index af0b439be0fb..5ef4e545a5eb 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1435,6 +1435,13 @@ describe('ReactDOMComponent', () => { }); it('should warn about class', () => { + spyOn(console, 'error'); + ReactTestUtils.renderIntoDocument(React.createElement('div', {class: 'muffins'})); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain('className'); + }); + + it('should warn about class (ssr)', () => { spyOn(console, 'error'); ReactDOMServer.renderToString(React.createElement('div', {class: 'muffins'})); expectDev(console.error.calls.count()).toBe(1); @@ -1453,7 +1460,37 @@ describe('ReactDOMComponent', () => { expectDev(console.error.calls.count()).toBe(2); }); + it('should warn about props that are no longer supported (ssr)', () => { + spyOn(console, 'error'); + ReactDOMServer.renderToString(
); + expectDev(console.error.calls.count()).toBe(0); + + ReactDOMServer.renderToString(
{}} />); + expectDev(console.error.calls.count()).toBe(1); + + ReactDOMServer.renderToString(
{}} />); + expectDev(console.error.calls.count()).toBe(2); + }); + it('gives source code refs for unknown prop warning', () => { + spyOn(console, 'error'); + ReactTestUtils.renderIntoDocument(
); + ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(2); + expect( + normalizeCodeLocInfo(console.error.calls.argsFor(0)[0]) + ).toBe( + 'Warning: Unknown DOM property class. Did you mean className?\n in div (at **)' + ); + expect( + normalizeCodeLocInfo(console.error.calls.argsFor(1)[0]) + ).toBe( + 'Warning: Unknown event handler property onclick. Did you mean ' + + '`onClick`?\n in input (at **)' + ); + }); + + it('gives source code refs for unknown prop warning (ssr)', () => { spyOn(console, 'error'); ReactDOMServer.renderToString(
); ReactDOMServer.renderToString(); @@ -1475,20 +1512,47 @@ describe('ReactDOMComponent', () => { spyOn(console, 'error'); var container = document.createElement('div'); - ReactDOMServer.renderToString(
, container); + ReactTestUtils.renderIntoDocument(
, container); expectDev(console.error.calls.count()).toBe(0); - ReactDOMServer.renderToString(
, container); + ReactTestUtils.renderIntoDocument(
, container); expectDev(console.error.calls.count()).toBe(1); expect( normalizeCodeLocInfo(console.error.calls.argsFor(0)[0]) ).toBe( 'Warning: Unknown DOM property class. Did you mean className?\n in div (at **)' ); + }); + it('gives source code refs for unknown prop warning for exact elements', () => { + spyOn(console, 'error'); + + ReactTestUtils.renderIntoDocument( +
+
+
+
+
+
+
+ ); + + expectDev(console.error.calls.count()).toBe(2); + + expectDev(console.error.calls.argsFor(0)[0]).toContain('className'); + var matches = console.error.calls.argsFor(0)[0].match(/.*\(.*:(\d+)\).*/); + var previousLine = matches[1]; + + expectDev(console.error.calls.argsFor(1)[0]).toContain('onClick'); + matches = console.error.calls.argsFor(1)[0].match(/.*\(.*:(\d+)\).*/); + var currentLine = matches[1]; + + //verify line number has a proper relative difference, + //since hard coding the line number would make test too brittle + expect(parseInt(previousLine, 10) + 2).toBe(parseInt(currentLine, 10)); }); - it('gives source code refs for unknown prop warning for exact elements ', () => { + it('gives source code refs for unknown prop warning for exact elements (ssr)', () => { spyOn(console, 'error'); ReactDOMServer.renderToString( @@ -1516,7 +1580,58 @@ describe('ReactDOMComponent', () => { expect(parseInt(previousLine, 10) + 2).toBe(parseInt(currentLine, 10)); }); - it('gives source code refs for unknown prop warning for exact elements in composition ', () => { + it('gives source code refs for unknown prop warning for exact elements in composition', () => { + spyOn(console, 'error'); + var container = document.createElement('div'); + + class Parent extends React.Component { + render() { + return
; + } + } + + class Child1 extends React.Component { + render() { + return
Child1
; + } + } + + class Child2 extends React.Component { + render() { + return
Child2
; + } + } + + class Child3 extends React.Component { + render() { + return
Child3
; + } + } + + class Child4 extends React.Component { + render() { + return
Child4
; + } + } + + ReactTestUtils.renderIntoDocument(, container); + + expectDev(console.error.calls.count()).toBe(2); + + expectDev(console.error.calls.argsFor(0)[0]).toContain('className'); + var matches = console.error.calls.argsFor(0)[0].match(/.*\(.*:(\d+)\).*/); + var previousLine = matches[1]; + + expectDev(console.error.calls.argsFor(1)[0]).toContain('onClick'); + matches = console.error.calls.argsFor(1)[0].match(/.*\(.*:(\d+)\).*/); + var currentLine = matches[1]; + + //verify line number has a proper relative difference, + //since hard coding the line number would make test too brittle + expect(parseInt(previousLine, 10) + 12).toBe(parseInt(currentLine, 10)); + }); + + it('gives source code refs for unknown prop warning for exact elements in composition (ssr)', () => { spyOn(console, 'error'); var container = document.createElement('div'); @@ -1583,5 +1698,22 @@ describe('ReactDOMComponent', () => { 'Warning: Unknown DOM property autofocus. Did you mean autoFocus?\n in input' ); }); + + it('should suggest property name if available (ssr)', () => { + spyOn(console, 'error'); + + ReactDOMServer.renderToString(React.createElement('label', {for: 'test'})); + ReactDOMServer.renderToString(React.createElement('input', {type: 'text', autofocus: true})); + + expectDev(console.error.calls.count()).toBe(2); + + expectDev(console.error.calls.argsFor(0)[0]).toBe( + 'Warning: Unknown DOM property for. Did you mean htmlFor?\n in label' + ); + + expectDev(console.error.calls.argsFor(1)[0]).toBe( + 'Warning: Unknown DOM property autofocus. Did you mean autoFocus?\n in input' + ); + }); }); });