skip null ref warning for ReactTestRenderer #7658
Conversation
ghost
added
| @@ -1107,7 +1107,7 @@ var ReactCompositeComponent = { | |||
| if (__DEV__) { | |||
| var componentName = component && component.getName ? | |||
| component.getName() : 'a component'; | |||
| warning(publicComponentInstance != null, | |||
There was a problem hiding this comment.
Can we check this._compositeType instead, and only warn for stateless functional components? It seems like this would solve the issue by making the warning more specific without hardcoding information about test renderer. Another upside is that the warning would still fire if you try to use refs with stateless components incorrectly while using test renderer for example.
There was a problem hiding this comment.
Oooo, I didn't know about _compositeType, thanks! 🎉
aweary
force-pushed
the
fix-react-test-renderer-ref-warning
branch
from
b89824f
to
11315dc
Compare
|
Looks great. Thanks. |
| warning(publicComponentInstance != null, | ||
| warning( | ||
| publicComponentInstance != null || | ||
| component._compositeType !== CompositeTypes.StatelessFunctional, |
There was a problem hiding this comment.
Note: this is a little sketchy because component might not be a ReactCompositeComponent (and thus wouldn't have _compositeType), but I suppose it is fine.
There was a problem hiding this comment.
It should still suppress the warning correctly though, right? Since it wouldn't be a SFC if there wasn't a _compositeType anyways. I can add an additional check to make it safer if you'd like
There was a problem hiding this comment.
Right, it would. I avoid polymorphic accesses like this whenever possible because they're slower but I'm not sure there's a better option here.
There was a problem hiding this comment.
Agree, sorry I missed this. Should we add a check for typeof component._currentElement.type === 'function'?
There was a problem hiding this comment.
component._currentElement is still polymorphic. We'd probably need to pass the component type along with the component instance if we wanted to avoid this.
There was a problem hiding this comment.
I think this is fine as-is, just wanted to note it.
There was a problem hiding this comment.
Oh, I understand this now. Different hidden class? Seems like we do this in many places anyway but thanks for explaining.
(cherry picked from commit 06ea71d)
Resolves #7645 as far as the warning goes--assuming that
toJSONisn't going to be implemented in other renderers.