-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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.
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
NativeMethodsMixin DEV-only methods should not warn #12212
NativeMethodsMixin DEV-only methods should not warn #12212
Conversation
// Add a flag to suppress these warnings for this special case. | ||
// TODO (bvaughn) Remove this flag once the above methods have been removed. | ||
NativeMethodsMixin_DEV.componentWillMount.__suppressDeprecationWarning = true; | ||
NativeMethodsMixin_DEV.componentWillReceiveProps.__suppressDeprecationWarning = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do any first- or third-party components use this with createReactClass
like
createClass({
mixins: [NativeMethodsMixin],
?
If yes, what happens if such component declares componentWillReceiveProps
, but not componentWillMount
? Could it be that suppression check here will cause it to show no message for custom componentWillReceiveProps
(which we should have warned about)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many built-in components like View
and ActivityIndicator
use createReactClass
with NativeMethodsMixin
in this way. None of them use legacy componentWillReceiveProps
or componentWillMount
methods though because I've code-modded them all away (before enabling this warning for the RN renderer).
I don't know if third-party components use the mixin, but hopefully not since it's behind __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh. Probably okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct though, that if I was wrong about the 2nd point, and a component only declared cWRP, the warning would be suppressed.
I could expand the check you linked to to verify that flag on both of the lifecycles if you think it's worth doing. I think it would be a simple matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. It was trivial to make the check robust enough to cover that case too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Circling back- the potentially troublesome check was later improved via /pull/12647
* Disable DEV-only warnings for RN NativeMethodsMixin/create-react-class * Tiny bit of cleanup * Make strict-mode suppression check a little more robust
Built-in React Native components (e.g.
View
) currently trigger deprecation warnings for lifecyclescomponentWillMount
andcomponentWillReceiveProps
due to these DEV-only mixins added by theNativeMethodsMixin
component.Long term, we should remove the deprecated lifecycles but for the time being, I think we should just suppress the warnings from the builtin methods specifically using a similar technique as the
react-lifecycles-compat
polyfill. This will hopefully improve the in-between period for the RN community significantly.