Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use different lifecycle methods for React 16.4 support. #128

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

None yet

1 participant