Skip to content

[Fiber] adds childContextTypes warnings for functional components #9043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
trueadm merged 4 commits into from
Feb 23, 2017
Merged

[Fiber] adds childContextTypes warnings for functional components #9043

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a9325e2
adds childContextTypes warnings for functional components
trueadm Feb 22, 2017
e7d7809
reverted the getChildContext warning from showing on functional compo…
trueadm Feb 23, 2017
925743b
reverted ReactDOMFeatureFlags
trueadm Feb 23, 2017
bde5df1
addressed code in feedback
trueadm Feb 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions scripts/fiber/tests-passing-except-dev.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions scripts/fiber/tests-passing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 15 additions & 0 deletions src/renderers/shared/fiber/ReactFiberBeginWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ var invariant = require('invariant');
if (__DEV__) {
var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');
var warning = require('warning');
var {
warnAboutMissingGetChildContext,
} = require('ReactFiberContext');

var warnedAboutStatelessRefs = {};
}
Expand Down Expand Up @@ -479,6 +482,18 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
// 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);
Copy link
Collaborator

@sebmarkbage sebmarkbage Feb 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test is wrong. It doesn't make sense to both warn for childContextTypes being defined and also warn for that you should provide a getChildContext which you cannot do. It's not actionable.

IMO we should either fix this in Stack and remove it or gate the test on ReactDOMFeatureFlags.useFiber and change the assertion for the Fiber case.

Copy link
Contributor Author

@trueadm trueadm Feb 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, completely. I was unsure if we wanted 1:1 parity with Stack in regards to these strange warning messages. I don't know why anyone would ever need the second warning message when using functional components, given they can't ever use getChildContext on the components. Was there intentions at some point to allow functional components to use getChildContext?

}
}
if (workInProgress.ref !== null) {
let info = '';
const ownerName = ReactDebugCurrentFiber.getCurrentFiberOwnerName();
Expand Down
37 changes: 22 additions & 15 deletions src/renderers/shared/fiber/ReactFiberContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}
Expand Down