From a9325e2ec40d2f16f2f07c0a24f72683c12f79c3 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 22 Feb 2017 14:04:35 +0000 Subject: [PATCH 1/4] adds childContextTypes warnings for functional components ReactStatelessComponent-test.js fails due to warnings not showing up when functional components mount or update. This PR ports the existing warnings from stack and re-applies them to fiber functional components and attempts to re-use some logic in ReactFiberContext that currently exists for showing warnings when dealing with class components. --- scripts/fiber/tests-passing-except-dev.txt | 3 -- scripts/fiber/tests-passing.txt | 1 + .../shared/fiber/ReactFiberBeginWork.js | 15 ++++++++ .../shared/fiber/ReactFiberContext.js | 37 +++++++++++-------- 4 files changed, 38 insertions(+), 18 deletions(-) diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index d082f2dc195..24ea49d188e 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -139,9 +139,6 @@ src/renderers/__tests__/ReactHostOperationHistoryHook-test.js * gets reported when a child is inserted * gets reported when a child is removed -src/renderers/__tests__/ReactStatelessComponent-test.js -* should warn for childContextTypes on a functional component - src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should not warn when server-side rendering `onScroll` * should warn about incorrect casing on properties (ssr) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 072fe059889..683ff48ff2e 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -777,6 +777,7 @@ src/renderers/__tests__/ReactStatelessComponent-test.js * should update stateless component * should unmount stateless component * should pass context thru stateless component +* should warn for childContextTypes on a functional component * should throw when stateless component returns undefined * should throw on string refs in pure functions * should warn when given a string ref diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index ffe68983d4d..a8020aeb503 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -67,6 +67,9 @@ var invariant = require('invariant'); if (__DEV__) { var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); var warning = require('warning'); + var { + warnAboutMissingGetChildContext, + } = require('ReactFiberContext'); var warnedAboutStatelessRefs = {}; } @@ -479,6 +482,18 @@ module.exports = function( // Proceed under the assumption that this is a functional component workInProgress.tag = FunctionalComponent; if (__DEV__) { + const Component = workInProgress.type; + + if (Component) { + warning( + !Component.childContextTypes, + '%s(...): childContextTypes cannot be defined on a functional component.', + Component.displayName || Component.name || 'Component' + ); + if (Component.childContextTypes) { + warnAboutMissingGetChildContext(workInProgress); + } + } if (workInProgress.ref !== null) { let info = ''; const ownerName = ReactDebugCurrentFiber.getCurrentFiberOwnerName(); diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index 8b7383458b2..c819410764e 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -32,9 +32,30 @@ const { push, } = require('ReactFiberStack'); +var warnAboutMissingGetChildContext; + if (__DEV__) { var checkReactTypeSpec = require('checkReactTypeSpec'); var warnedAboutMissingGetChildContext = {}; + + // TODO (bvaughn) Replace this behavior with an invariant() in the future. + // It has only been added in Fiber to match the (unintentional) behavior in Stack. + warnAboutMissingGetChildContext = (fiber: Fiber) => { + const componentName = getComponentName(fiber); + + if (!warnedAboutMissingGetChildContext[componentName]) { + warnedAboutMissingGetChildContext[componentName] = true; + warning( + false, + '%s.childContextTypes is specified but there is no getChildContext() method ' + + 'on the instance. You can either define getChildContext() on %s or remove ' + + 'childContextTypes from it.', + componentName, + componentName, + ); + } + }; + module.exports.warnAboutMissingGetChildContext = warnAboutMissingGetChildContext; } // A cursor to the current merged context object on the stack. @@ -144,23 +165,9 @@ function processChildContext(fiber : Fiber, parentContext : Object, isReconcilin const instance = fiber.stateNode; const childContextTypes = fiber.type.childContextTypes; - // TODO (bvaughn) Replace this behavior with an invariant() in the future. - // It has only been added in Fiber to match the (unintentional) behavior in Stack. if (typeof instance.getChildContext !== 'function') { if (__DEV__) { - const componentName = getComponentName(fiber); - - if (!warnedAboutMissingGetChildContext[componentName]) { - warnedAboutMissingGetChildContext[componentName] = true; - warning( - false, - '%s.childContextTypes is specified but there is no getChildContext() method ' + - 'on the instance. You can either define getChildContext() on %s or remove ' + - 'childContextTypes from it.', - componentName, - componentName, - ); - } + warnAboutMissingGetChildContext(fiber); } return parentContext; } From e7d7809eb198616b1765179c654b1f53a8d0486b Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 23 Feb 2017 13:39:08 +0000 Subject: [PATCH 2/4] reverted the getChildContext warning from showing on functional components in Fiber --- .../__tests__/ReactStatelessComponent-test.js | 31 ++++++++++------ .../dom/shared/ReactDOMFeatureFlags.js | 2 +- .../shared/fiber/ReactFiberBeginWork.js | 7 ---- .../shared/fiber/ReactFiberContext.js | 37 ++++++++----------- 4 files changed, 36 insertions(+), 41 deletions(-) diff --git a/src/renderers/__tests__/ReactStatelessComponent-test.js b/src/renderers/__tests__/ReactStatelessComponent-test.js index 223cdc47ec2..beab244cf99 100644 --- a/src/renderers/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/__tests__/ReactStatelessComponent-test.js @@ -118,17 +118,26 @@ describe('ReactStatelessComponent', () => { ReactDOM.render(, container); - expectDev(console.error.calls.count()).toBe(2); - expectDev(console.error.calls.argsFor(0)[0]).toContain( - 'StatelessComponentWithChildContext(...): childContextTypes cannot ' + - 'be defined on a functional component.' - ); - expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(1)[0])).toBe( - 'Warning: StatelessComponentWithChildContext.childContextTypes is specified ' + - 'but there is no getChildContext() method on the instance. You can either ' + - 'define getChildContext() on StatelessComponentWithChildContext or remove ' + - 'childContextTypes from it.' - ); + // Stack and Fiber differ in terms of they show warnings + if (!ReactDOMFeatureFlags.useFiber) { + expectDev(console.error.calls.count()).toBe(2); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'StatelessComponentWithChildContext(...): childContextTypes cannot ' + + 'be defined on a functional component.' + ); + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(1)[0])).toBe( + 'Warning: StatelessComponentWithChildContext.childContextTypes is specified ' + + 'but there is no getChildContext() method on the instance. You can either ' + + 'define getChildContext() on StatelessComponentWithChildContext or remove ' + + 'childContextTypes from it.' + ); + } else { + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'StatelessComponentWithChildContext(...): childContextTypes cannot ' + + 'be defined on a functional component.' + ); + } }); if (!ReactDOMFeatureFlags.useFiber) { diff --git a/src/renderers/dom/shared/ReactDOMFeatureFlags.js b/src/renderers/dom/shared/ReactDOMFeatureFlags.js index 5e9d93267cf..17bae0a58af 100644 --- a/src/renderers/dom/shared/ReactDOMFeatureFlags.js +++ b/src/renderers/dom/shared/ReactDOMFeatureFlags.js @@ -13,7 +13,7 @@ var ReactDOMFeatureFlags = { useCreateElement: true, - useFiber: false, + useFiber: true, }; module.exports = ReactDOMFeatureFlags; diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index a8020aeb503..73c4870a300 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -67,10 +67,6 @@ var invariant = require('invariant'); if (__DEV__) { var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); var warning = require('warning'); - var { - warnAboutMissingGetChildContext, - } = require('ReactFiberContext'); - var warnedAboutStatelessRefs = {}; } @@ -490,9 +486,6 @@ module.exports = function( '%s(...): childContextTypes cannot be defined on a functional component.', Component.displayName || Component.name || 'Component' ); - if (Component.childContextTypes) { - warnAboutMissingGetChildContext(workInProgress); - } } if (workInProgress.ref !== null) { let info = ''; diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index c819410764e..8b7383458b2 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -32,30 +32,9 @@ const { push, } = require('ReactFiberStack'); -var warnAboutMissingGetChildContext; - if (__DEV__) { var checkReactTypeSpec = require('checkReactTypeSpec'); var warnedAboutMissingGetChildContext = {}; - - // TODO (bvaughn) Replace this behavior with an invariant() in the future. - // It has only been added in Fiber to match the (unintentional) behavior in Stack. - warnAboutMissingGetChildContext = (fiber: Fiber) => { - const componentName = getComponentName(fiber); - - if (!warnedAboutMissingGetChildContext[componentName]) { - warnedAboutMissingGetChildContext[componentName] = true; - warning( - false, - '%s.childContextTypes is specified but there is no getChildContext() method ' + - 'on the instance. You can either define getChildContext() on %s or remove ' + - 'childContextTypes from it.', - componentName, - componentName, - ); - } - }; - module.exports.warnAboutMissingGetChildContext = warnAboutMissingGetChildContext; } // A cursor to the current merged context object on the stack. @@ -165,9 +144,23 @@ function processChildContext(fiber : Fiber, parentContext : Object, isReconcilin const instance = fiber.stateNode; const childContextTypes = fiber.type.childContextTypes; + // TODO (bvaughn) Replace this behavior with an invariant() in the future. + // It has only been added in Fiber to match the (unintentional) behavior in Stack. if (typeof instance.getChildContext !== 'function') { if (__DEV__) { - warnAboutMissingGetChildContext(fiber); + const componentName = getComponentName(fiber); + + if (!warnedAboutMissingGetChildContext[componentName]) { + warnedAboutMissingGetChildContext[componentName] = true; + warning( + false, + '%s.childContextTypes is specified but there is no getChildContext() method ' + + 'on the instance. You can either define getChildContext() on %s or remove ' + + 'childContextTypes from it.', + componentName, + componentName, + ); + } } return parentContext; } From 925743bfc9a89204e0fd095d4efaf126f782790f Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 23 Feb 2017 13:40:16 +0000 Subject: [PATCH 3/4] reverted ReactDOMFeatureFlags --- src/renderers/dom/shared/ReactDOMFeatureFlags.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderers/dom/shared/ReactDOMFeatureFlags.js b/src/renderers/dom/shared/ReactDOMFeatureFlags.js index 17bae0a58af..5e9d93267cf 100644 --- a/src/renderers/dom/shared/ReactDOMFeatureFlags.js +++ b/src/renderers/dom/shared/ReactDOMFeatureFlags.js @@ -13,7 +13,7 @@ var ReactDOMFeatureFlags = { useCreateElement: true, - useFiber: true, + useFiber: false, }; module.exports = ReactDOMFeatureFlags; From bde5df1d7636e9ea1a5b487226bd5f303c8f0cfa Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 23 Feb 2017 15:37:19 +0000 Subject: [PATCH 4/4] addressed code in feedback --- .../__tests__/ReactStatelessComponent-test.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/renderers/__tests__/ReactStatelessComponent-test.js b/src/renderers/__tests__/ReactStatelessComponent-test.js index beab244cf99..249b17acd33 100644 --- a/src/renderers/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/__tests__/ReactStatelessComponent-test.js @@ -119,7 +119,13 @@ describe('ReactStatelessComponent', () => { ReactDOM.render(, container); // Stack and Fiber differ in terms of they show warnings - if (!ReactDOMFeatureFlags.useFiber) { + if (ReactDOMFeatureFlags.useFiber) { + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'StatelessComponentWithChildContext(...): childContextTypes cannot ' + + 'be defined on a functional component.' + ); + } else { expectDev(console.error.calls.count()).toBe(2); expectDev(console.error.calls.argsFor(0)[0]).toContain( 'StatelessComponentWithChildContext(...): childContextTypes cannot ' + @@ -131,12 +137,6 @@ describe('ReactStatelessComponent', () => { 'define getChildContext() on StatelessComponentWithChildContext or remove ' + 'childContextTypes from it.' ); - } else { - expectDev(console.error.calls.count()).toBe(1); - expectDev(console.error.calls.argsFor(0)[0]).toContain( - 'StatelessComponentWithChildContext(...): childContextTypes cannot ' + - 'be defined on a functional component.' - ); } });