Warn about missing getChildContext method#8742
Conversation
| // It has only been added in Fiber to match the (unintentional) behavior in Stack. | ||
| if (typeof instance.getChildContext !== 'function') { | ||
| warning( | ||
| false, |
There was a problem hiding this comment.
We should deduplicate this by component name. Otherwise it can get very noisy.
There was a problem hiding this comment.
Here is an example of how we do it #8739.
This PR would be much simpler though. Since warning is specific to a class, we can use component name as the key in the warnedAboutMissingChildContext cache.
There was a problem hiding this comment.
Great suggestion Dan. Thanks!
There was a problem hiding this comment.
Deduped and a new test added
bvaughn
force-pushed
the
stack-fiber-gcc-warning
branch
2 times, most recently
from
edf41b5 to
e972b89
Compare
bvaughn
changed the title
gaearon
left a comment
gaearon
left a comment
There was a problem hiding this comment.
Accepting with nitpicks.
|
|
||
| if (__DEV__) { | ||
| var checkReactTypeSpec = require('checkReactTypeSpec'); | ||
| var warningAboutMissingGetChildContext = {}; |
There was a problem hiding this comment.
Nit: can we name this warnedAboutMissingGetChildContext?
| warningAboutMissingGetChildContext[componentName] = true; | ||
| warning( | ||
| false, | ||
| 'getChildContext() is not defined for %s', |
There was a problem hiding this comment.
Maybe let’s make it a bit more descriptive? We can make it similar in style to messages in ReactFiberClassComponent (and Stack equivalents in ReactCompositeComponent). For example:
'%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.',
| it('should pass parent context if getChildContext method is missing', () => { | ||
| spyOn(console, 'error'); | ||
|
|
||
| var ParentContextProvider = React.createClass({ |
There was a problem hiding this comment.
Any special reason for using createClass here? Ideally we'd like to use ES6 for testing anything other than createClass itself specifically.
There was a problem hiding this comment.
Was just mirroring what's in the rest of the class. I'll replace it with ES6 syntax though. I prefer that anyway.
Wrote this test first, mirrored the rest of the class, wrote the other test afterward and used ES6. Not sure why. 😅
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.
bvaughn
force-pushed
the
stack-fiber-gcc-warning
branch
from
e3d06ed to
7a2e35b
Compare
4 participants
Previous (probably unintentional) behavior of Stack was to allow components to define
childContextTypeswithout also supplying agetChildContextproperty. This PR updates Fiber to temporarily mimic that behavior. It also adds warning messages to both Fiber and Stack along with a some tests.For the time being Fiber components with a missing
getChildContextmethod will return the parentcontextas-is, after warning.