Unbreak custom animated components#10827
Conversation
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
@leeight could you please take a look at this? |
There was a problem hiding this comment.
We should ensure viewConfig exists before call setNativeProps
if (this._component.viewConfig != null) {
this._component.setNativeProps(...);
}
There was a problem hiding this comment.
So we can change throw new Erorr to console.warn??
There was a problem hiding this comment.
@leeight The problem is that custom components can proxy the setNativeProps call to the wrapped component which will throw/warn even though it's valid use. Isn't it better to do this check in the setNativeProps function instead?
There was a problem hiding this comment.
|
@leeight Updated PR |
There was a problem hiding this comment.
I think it should be this.constructor.displayName
There was a problem hiding this comment.
var ctor = this.constructor;
var componentName = ctor.displayName || ctor.name || '<Unknown Component
facebook-github-bot
added
GH Review: review-needed
and removed
GH Review: needs-revision
labels
|
LGTM. CC @javache |
| if (!this.viewConfig) { | ||
| var ctor = this.constructor; | ||
| var componentName = ctor.displayName || ctor.name || '<Unknown Component>'; | ||
| throw new Error(componentName + ' "viewConfig" is not defined.'); |
There was a problem hiding this comment.
Can you use invariant here? Because you already have checked for the error, please use invariant(false, ...
There was a problem hiding this comment.
I was really just moving this this section from the other function, but now fixed. What are the benefits to using invariant here if we already have done the check in the if statement?
|
Thanks! @facebook-github-bot shipit |
facebook-github-bot
added
GH Review: accepted
Import Started
|
@ericvicenti has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: Commit c996081 broke `Animated.createAnimatedComponent()` for most components outside of the standard ones. This is because it will throw an exception for any component without a `viewConfig` defined, which basically no composition style component has AFAIK. Related issues: #10825, oblador/react-native-vector-icons#337, oblador/react-native-animatable#70 This PR removes the check which is more treating the symptoms really – but shouldn't `setNativeProps` duck typing be enough? Closes #10827 Differential Revision: D4178183 Pulled By: ericvicenti fbshipit-source-id: b85cc78ed6d84dada4d476caa243332eaadb003f
Summary: Commit facebook@c996081 broke `Animated.createAnimatedComponent()` for most components outside of the standard ones. This is because it will throw an exception for any component without a `viewConfig` defined, which basically no composition style component has AFAIK. Related issues: facebook#10825, oblador/react-native-vector-icons#337, oblador/react-native-animatable#70 This PR removes the check which is more treating the symptoms really – but shouldn't `setNativeProps` duck typing be enough? Closes facebook#10827 Differential Revision: D4178183 Pulled By: ericvicenti fbshipit-source-id: b85cc78ed6d84dada4d476caa243332eaadb003f
6 participants
Commit c996081 broke
Animated.createAnimatedComponent()for most components outside of the standard ones. This is because it will throw an exception for any component without aviewConfigdefined, which basically no composition style component has AFAIK.Related issues: #10825, oblador/react-native-vector-icons#337, oblador/react-native-animatable#70
This PR removes the check which is more treating the symptoms really – but shouldn't
setNativePropsduck typing be enough?