From c7f1abade518c70a6386c15279054c2ed1abf2e8 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 10 Jan 2017 13:44:19 -0800 Subject: [PATCH 1/4] Stack and Fiber warn about missing getChildContext method Previous (probably unintentional) behavior of Stack was to allow components to define childContextTypes without also supplying a getChildContext property. This PR updates Fiber to (temporarily) mimic that behavior. It also adds warning messages to both Fiber and Stack (along with a test). For the time being, Fiber components with a missing getChildContext method will return the parent context as-is, after warning. --- scripts/fiber/tests-passing.txt | 2 + .../__tests__/ReactContextValidator-test.js | 67 +++++++++++++++++++ .../shared/fiber/ReactFiberContext.js | 13 ++++ .../__tests__/ReactStatelessComponent-test.js | 6 +- .../reconciler/ReactCompositeComponent.js | 8 +++ 5 files changed, 95 insertions(+), 1 deletion(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 0097217ec2..b2952dfe3d 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -157,6 +157,8 @@ src/isomorphic/classic/__tests__/ReactContextValidator-test.js * should pass next context to lifecycles * should check context types * should check child context types +* should warn (but not error) if getChildContext method is missing +* should pass parent context if getChildContext method is missing src/isomorphic/classic/class/__tests__/ReactBind-test.js * Holds reference to instance diff --git a/src/isomorphic/classic/__tests__/ReactContextValidator-test.js b/src/isomorphic/classic/__tests__/ReactContextValidator-test.js index 7caedcd761..8e9cad90ca 100644 --- a/src/isomorphic/classic/__tests__/ReactContextValidator-test.js +++ b/src/isomorphic/classic/__tests__/ReactContextValidator-test.js @@ -289,4 +289,71 @@ describe('ReactContextValidator', () => { expectDev(console.error.calls.count()).toBe(2); }); + // TODO (bvaughn) Remove this test and the associated behavior in the future. + // It has only been added in Fiber to match the (unintentional) behavior in Stack. + it('should warn (but not error) if getChildContext method is missing', () => { + spyOn(console, 'error'); + + var MyComponent = React.createClass({ + childContextTypes: { + foo: React.PropTypes.string.isRequired, + }, + + render: function() { + return
; + }, + }); + + ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(1); + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( + 'Warning: getChildContext() is not defined for MyComponent' + ); + }); + + // TODO (bvaughn) Remove this test and the associated behavior in the future. + // It has only been added in Fiber to match the (unintentional) behavior in Stack. + it('should pass parent context if getChildContext method is missing', () => { + spyOn(console, 'error'); + + var ParentContextProvider = React.createClass({ + childContextTypes: { + foo: React.PropTypes.number, + }, + getChildContext: function() { + return { + foo: 'FOO', + }; + }, + render: function() { + return ; + }, + }); + + var MiddleMissingContext = React.createClass({ + childContextTypes: { + bar: React.PropTypes.string.isRequired, + }, + render: function() { + return ; + }, + }); + + var childContext; + var ChildContextConsumer = React.createClass({ + contextTypes: { + bar: React.PropTypes.string.isRequired, + foo: React.PropTypes.string.isRequired, + }, + render: function() { + childContext = this.context; + return
; + }, + }); + + ReactTestUtils.renderIntoDocument(); + expect(childContext.bar).toBeUndefined(); + expect(childContext.foo).toBe('FOO'); + }); + }); diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index 1746a6e818..7bd1debf7f 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -17,6 +17,7 @@ import type { StackCursor } from 'ReactFiberStack'; var emptyObject = require('emptyObject'); var invariant = require('invariant'); +var warning = require('warning'); var { getComponentName, isFiberMounted, @@ -141,6 +142,18 @@ exports.pushTopLevelContextObject = function(fiber : Fiber, context : Object, di function processChildContext(fiber : Fiber, parentContext : Object, isReconciling : boolean): Object { 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') { + warning( + false, + 'getChildContext() is not defined for %s', + getComponentName(fiber), + ); + return parentContext; + } + const childContext = instance.getChildContext(); for (let contextKey in childContext) { invariant( diff --git a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js index 876f268554..94291c816d 100644 --- a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js @@ -118,11 +118,15 @@ describe('ReactStatelessComponent', () => { ReactDOM.render(, container); - expectDev(console.error.calls.count()).toBe(1); + 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: getChildContext() is not defined for ' + + 'StatelessComponentWithChildContext' + ); }); if (!ReactDOMFeatureFlags.useFiber) { diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index ecdccd0982..9469e3f2a4 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -698,6 +698,14 @@ var ReactCompositeComponent = { ); } return Object.assign({}, currentContext, childContext); + } else { + if (__DEV__) { + warning( + !Component.childContextTypes, + 'getChildContext() is not defined for %s', + this.getName(), + ); + } } return currentContext; }, From 4f08884b5d132ed7275261769e403ce44517dcc7 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 10 Jan 2017 14:00:56 -0800 Subject: [PATCH 2/4] Dedupe missing getChildContext warning by component name --- scripts/fiber/tests-passing.txt | 1 + .../__tests__/ReactContextValidator-test.js | 34 +++++++++++++++++++ .../shared/fiber/ReactFiberContext.js | 18 +++++++--- .../reconciler/ReactCompositeComponent.js | 16 ++++++--- 4 files changed, 59 insertions(+), 10 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index b2952dfe3d..5eb43be0ba 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -159,6 +159,7 @@ src/isomorphic/classic/__tests__/ReactContextValidator-test.js * should check child context types * should warn (but not error) if getChildContext method is missing * should pass parent context if getChildContext method is missing +* should only warn about missing getChildContext once per component type src/isomorphic/classic/class/__tests__/ReactBind-test.js * Holds reference to instance diff --git a/src/isomorphic/classic/__tests__/ReactContextValidator-test.js b/src/isomorphic/classic/__tests__/ReactContextValidator-test.js index 8e9cad90ca..cd844e9f52 100644 --- a/src/isomorphic/classic/__tests__/ReactContextValidator-test.js +++ b/src/isomorphic/classic/__tests__/ReactContextValidator-test.js @@ -356,4 +356,38 @@ describe('ReactContextValidator', () => { expect(childContext.foo).toBe('FOO'); }); + it('should only warn about missing getChildContext once per component type', () => { + spyOn(console, 'error'); + + class ComponentA extends React.Component { + static childContextTypes = { + foo: React.PropTypes.string.isRequired, + }; + render() { + return
; + } + } + class ComponentB extends React.Component { + static childContextTypes = { + foo: React.PropTypes.string.isRequired, + }; + render() { + return
; + } + } + + ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(1); + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( + 'Warning: getChildContext() is not defined for ComponentA' + ); + ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(1); + ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(2); + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(1)[0])).toBe( + 'Warning: getChildContext() is not defined for ComponentB' + ); + }); + }); diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index 7bd1debf7f..b3a4f61e53 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -34,6 +34,7 @@ const { if (__DEV__) { var checkReactTypeSpec = require('checkReactTypeSpec'); + var warningAboutMissingGetChildContext = {}; } // A cursor to the current merged context object on the stack. @@ -146,11 +147,18 @@ function processChildContext(fiber : Fiber, parentContext : Object, isReconcilin // 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') { - warning( - false, - 'getChildContext() is not defined for %s', - getComponentName(fiber), - ); + if (__DEV__) { + const componentName = getComponentName(fiber); + + if (!warningAboutMissingGetChildContext[componentName]) { + warningAboutMissingGetChildContext[componentName] = true; + warning( + false, + 'getChildContext() is not defined for %s', + componentName, + ); + } + } return parentContext; } diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index 9469e3f2a4..d93135e51a 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -24,6 +24,7 @@ var ReactReconciler = require('ReactReconciler'); if (__DEV__) { var checkReactTypeSpec = require('checkReactTypeSpec'); + var warningAboutMissingGetChildContext = {}; } var emptyObject = require('emptyObject'); @@ -700,11 +701,16 @@ var ReactCompositeComponent = { return Object.assign({}, currentContext, childContext); } else { if (__DEV__) { - warning( - !Component.childContextTypes, - 'getChildContext() is not defined for %s', - this.getName(), - ); + const componentName = this.getName(); + + if (!warningAboutMissingGetChildContext[componentName]) { + warningAboutMissingGetChildContext[componentName] = true; + warning( + !Component.childContextTypes, + 'getChildContext() is not defined for %s', + componentName, + ); + } } } return currentContext; From e17cc98a898f0e92e1f8a709820b9c82ea8a1965 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 10 Jan 2017 14:03:23 -0800 Subject: [PATCH 3/4] Removed redundant gCC test --- scripts/fiber/tests-passing.txt | 1 - .../__tests__/ReactContextValidator-test.js | 68 +++++++------------ 2 files changed, 25 insertions(+), 44 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 5eb43be0ba..b2952dfe3d 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -159,7 +159,6 @@ src/isomorphic/classic/__tests__/ReactContextValidator-test.js * should check child context types * should warn (but not error) if getChildContext method is missing * should pass parent context if getChildContext method is missing -* should only warn about missing getChildContext once per component type src/isomorphic/classic/class/__tests__/ReactBind-test.js * Holds reference to instance diff --git a/src/isomorphic/classic/__tests__/ReactContextValidator-test.js b/src/isomorphic/classic/__tests__/ReactContextValidator-test.js index cd844e9f52..883764e067 100644 --- a/src/isomorphic/classic/__tests__/ReactContextValidator-test.js +++ b/src/isomorphic/classic/__tests__/ReactContextValidator-test.js @@ -294,20 +294,36 @@ describe('ReactContextValidator', () => { it('should warn (but not error) if getChildContext method is missing', () => { spyOn(console, 'error'); - var MyComponent = React.createClass({ - childContextTypes: { + class ComponentA extends React.Component { + static childContextTypes = { foo: React.PropTypes.string.isRequired, - }, - - render: function() { + }; + render() { return
; - }, - }); + } + } + class ComponentB extends React.Component { + static childContextTypes = { + foo: React.PropTypes.string.isRequired, + }; + render() { + return
; + } + } - ReactTestUtils.renderIntoDocument(); + ReactTestUtils.renderIntoDocument(); expectDev(console.error.calls.count()).toBe(1); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: getChildContext() is not defined for MyComponent' + 'Warning: getChildContext() is not defined for ComponentA' + ); + + // Warnings should be deduped by component type + ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(1); + ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(2); + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(1)[0])).toBe( + 'Warning: getChildContext() is not defined for ComponentB' ); }); @@ -356,38 +372,4 @@ describe('ReactContextValidator', () => { expect(childContext.foo).toBe('FOO'); }); - it('should only warn about missing getChildContext once per component type', () => { - spyOn(console, 'error'); - - class ComponentA extends React.Component { - static childContextTypes = { - foo: React.PropTypes.string.isRequired, - }; - render() { - return
; - } - } - class ComponentB extends React.Component { - static childContextTypes = { - foo: React.PropTypes.string.isRequired, - }; - render() { - return
; - } - } - - ReactTestUtils.renderIntoDocument(); - expectDev(console.error.calls.count()).toBe(1); - expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: getChildContext() is not defined for ComponentA' - ); - ReactTestUtils.renderIntoDocument(); - expectDev(console.error.calls.count()).toBe(1); - ReactTestUtils.renderIntoDocument(); - expectDev(console.error.calls.count()).toBe(2); - expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(1)[0])).toBe( - 'Warning: getChildContext() is not defined for ComponentB' - ); - }); - }); From 7a2e35b93b20f702c274dcfcddf4a184e82a7cc3 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 12 Jan 2017 17:11:53 -0800 Subject: [PATCH 4/4] Improved error message wording for missing getChildContext() method --- .../__tests__/ReactContextValidator-test.js | 36 ++++++++++--------- .../shared/fiber/ReactFiberContext.js | 11 +++--- .../__tests__/ReactStatelessComponent-test.js | 6 ++-- .../reconciler/ReactCompositeComponent.js | 5 ++- 4 files changed, 35 insertions(+), 23 deletions(-) diff --git a/src/isomorphic/classic/__tests__/ReactContextValidator-test.js b/src/isomorphic/classic/__tests__/ReactContextValidator-test.js index 883764e067..08582a200d 100644 --- a/src/isomorphic/classic/__tests__/ReactContextValidator-test.js +++ b/src/isomorphic/classic/__tests__/ReactContextValidator-test.js @@ -314,7 +314,9 @@ describe('ReactContextValidator', () => { ReactTestUtils.renderIntoDocument(); expectDev(console.error.calls.count()).toBe(1); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: getChildContext() is not defined for ComponentA' + 'Warning: ComponentA.childContextTypes is specified but there is no ' + + 'getChildContext() method on the instance. You can either define ' + + 'getChildContext() on ComponentA or remove childContextTypes from it.' ); // Warnings should be deduped by component type @@ -323,7 +325,9 @@ describe('ReactContextValidator', () => { ReactTestUtils.renderIntoDocument(); expectDev(console.error.calls.count()).toBe(2); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(1)[0])).toBe( - 'Warning: getChildContext() is not defined for ComponentB' + 'Warning: ComponentB.childContextTypes is specified but there is no ' + + 'getChildContext() method on the instance. You can either define ' + + 'getChildContext() on ComponentB or remove childContextTypes from it.' ); }); @@ -332,28 +336,28 @@ describe('ReactContextValidator', () => { it('should pass parent context if getChildContext method is missing', () => { spyOn(console, 'error'); - var ParentContextProvider = React.createClass({ - childContextTypes: { + class ParentContextProvider extends React.Component { + static childContextTypes = { foo: React.PropTypes.number, - }, - getChildContext: function() { + }; + getChildContext() { return { foo: 'FOO', }; - }, - render: function() { + } + render() { return ; - }, - }); + } + } - var MiddleMissingContext = React.createClass({ - childContextTypes: { + class MiddleMissingContext extends React.Component { + static childContextTypes = { bar: React.PropTypes.string.isRequired, - }, - render: function() { + }; + render() { return ; - }, - }); + } + } var childContext; var ChildContextConsumer = React.createClass({ diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index b3a4f61e53..322affe797 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -34,7 +34,7 @@ const { if (__DEV__) { var checkReactTypeSpec = require('checkReactTypeSpec'); - var warningAboutMissingGetChildContext = {}; + var warnedAboutMissingGetChildContext = {}; } // A cursor to the current merged context object on the stack. @@ -150,11 +150,14 @@ function processChildContext(fiber : Fiber, parentContext : Object, isReconcilin if (__DEV__) { const componentName = getComponentName(fiber); - if (!warningAboutMissingGetChildContext[componentName]) { - warningAboutMissingGetChildContext[componentName] = true; + if (!warnedAboutMissingGetChildContext[componentName]) { + warnedAboutMissingGetChildContext[componentName] = true; warning( false, - 'getChildContext() is not defined for %s', + '%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, ); } diff --git a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js index 94291c816d..b31706c66b 100644 --- a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js @@ -124,8 +124,10 @@ describe('ReactStatelessComponent', () => { 'be defined on a functional component.' ); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(1)[0])).toBe( - 'Warning: getChildContext() is not defined for ' + - 'StatelessComponentWithChildContext' + '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.' ); }); diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index d93135e51a..e7aa14ef93 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -707,7 +707,10 @@ var ReactCompositeComponent = { warningAboutMissingGetChildContext[componentName] = true; warning( !Component.childContextTypes, - 'getChildContext() is not defined for %s', + '%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, ); }