Skip to content

Conversation

@joshjhargreaves
Copy link

React 16.4 deprecates componentWillMount, componentWillReceiveProps, componentWillUpdate & renames them to UNSAFE_componentWillMount, UNSAFE_componentWillRecieveProps, UNSAFE_componentWillUpdate.

Slider.js uses componentWillRecieveProps and componentWillMount. It however doesn't look like it actually needs to use them!

Firstly _panResponder is initialized in componentWillMount. It feels fairly arbitrary where we initialize this in fact, but the most important thing is that it is initialized before the first render. Moving this intialization to the constructor guarantees this also, and the pan-responder basic usage shows initialization in the constructor, for example: https://facebook.github.io/react-native/docs/panresponder.html#basic-usage.

Secondly, Slider uses componentWillReceiveProps to trigger some animations. Async React will behave differently in the future: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#side-effects-on-props-change

Like componentWillUpdate, componentWillReceiveProps might get called multiple times for a single update. For this reason it is important to avoid putting side effects in this method. Instead, componentDidUpdate should be used since it is guaranteed to be invoked only once per update

Therefore I have moved the logic from within componentWillRecieveProps to componentDidUpdate. One semantic difference is that this PR uses the current value of this.props.animateTransitions, which will be the current (after the update) prop value of animateTransitions in componentDidUpdate whereas in componentWillReceiveProps, nextProps was not checked for this value, this.props was used, meaning it was not the new value.

It seems to me that it's more correct to use the current value of animateTransitions after the update, but let me know if you don't think that this is the case!

Thirdly....thanks for making & maintaining this great library 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant