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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the use of UNSAFE_componentWillReceiveProps #7

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yoedfried
Copy link

Use memoize instead of saving the style in the state like explained in this doc: https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#what-about-memoization

Copy link

@IntelliJAbhishek IntelliJAbhishek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a refoctor in StyleProvider.js replace componentDidUpdate

static getDerivedStateFromProps(nextProps) {
    if (nextProps.style !== this.props.style) {
       return {
            theme: new Theme(nextProps.style)
       };
    }else{
        return null;
    }
}

@IntelliJAbhishek
Copy link

@yoedfried can you commit with changes?

@yoedfried
Copy link
Author

getDerivedStateFromProps is a static method so you cant reach this.props.style.
The only option to do it is to save the style into the state each time.
In StyleProvider the componentDidUpdate should be fine because we compare the props.style, and set the state only in case it changed.
Anyway, i can use the memoize also in this function (instead of componentDidUpdate).

@IntelliJAbhishek
Copy link

IntelliJAbhishek commented Jul 26, 2019

Ohh my mistake. How does memoize works?

@yoedfried
Copy link
Author

memoize save the result of the function and return it back in case it called with the same vars.
I wasn't able to use it in StyleProvider (it cause me a problem).
Anyway, I think it is safe to use componentDidUpdate in there because of the if before the setState.
I wast able to change the theme in runtime with the change and without it, is it even supported by native base?

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.

2 participants