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

Buggy RefreshControl #7414

Closed
ashwinbaweja opened this issue May 5, 2016 · 4 comments
Closed

Buggy RefreshControl #7414

ashwinbaweja opened this issue May 5, 2016 · 4 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@ashwinbaweja
Copy link

According to the docs for RefreshControl:

Note: refreshing is a controlled prop, this is why it needs to be set to true in the onRefresh function otherwise the refresh indicator will stop immediatly.

Within my onRefresh function, I'm using setState to set an isRefreshing flag to true. However, according to the React docs:

setState() does not immediately mutate this.state but creates a pending state transition. Accessing this.state after calling this method can potentially return the existing value.

As a result, when the following code is run in RefreshControl.js,

_onRefresh() {
    this.props.onRefresh && this.props.onRefresh();

    if (this._nativeRef) {
      this._nativeRef.setNativeProps({refreshing: this.props.refreshing});
    }
  }

this.props.refreshing is still false (since the state hasn't been updated and thus RefreshControl hasn't been re-rendered), causing the refresh indicator to stop immediately.

When I comment out the setNativeProps function call, my refresh indicator works as expected.

Am I missing something/doing something incorrectly here?

  • version 0.26.0-rc
  • occurs on iOS (both simulator and phone), haven't tested on Android
@damusnet
Copy link
Contributor

damusnet commented May 5, 2016

@facebook-github-bot stack-overlow

@janicduplessis
Copy link
Contributor

I already saw this happen at some point but I couldn't reproduce it consistently. Assuming that set state will be sync is definetly a bad idea, I'll try to figure out a better way to do this.

@ide
Copy link
Contributor

ide commented May 7, 2016

I think we want this code to work even if this.props were immutable. So instead of calling setNativeProps in _onRefresh, we should probably either let render() take care of setting the prop (why isn't this the case already?) or call setNativeProps in componentWillUpdate.

@janicduplessis
Copy link
Contributor

janicduplessis commented May 7, 2016

The goal of this code is to keep the native refresh control in sync with the JS props. If the user doesn't call setState({refreshing: true}) in the onRefresh callback the state will get out of sync with the native refresh control.

Maybe a better way to do this is call setNativeProps({refreshing: false}) before calling the onRefresh callback but I'm not 100% sure it will work. Another way is to use setImmediate to call the setNativeProps at the end of the js frame.

@ghost ghost closed this as completed in 9aa37e8 May 9, 2016
ptmt pushed a commit to ptmt/react-native that referenced this issue May 9, 2016
Summary:
There was an issue with the way we made `RefreshControl` a controlled component when react doesn't render synchronously. This fixes the issue by using the same technique used in the commit 0cd2904 for`PickerAndroid`

**Test plan (required)**

Tested normal behaviour and tested that if not setting the `refreshing` prop to `true` during the `onRefresh` callback that the `RefreshControl` stops refreshing immediately. Also made sure that `setNativeProps` is only called if needed when the native refreshing state is not in sync with JS.

Fix facebook#7414
Closes facebook#7445

Differential Revision: D3274981

fb-gh-sync-id: a1b9d46329f552984e33d11fa0e29ad6da689511
fbshipit-source-id: a1b9d46329f552984e33d11fa0e29ad6da689511
zebulgar pushed a commit to nightingale/react-native that referenced this issue Jun 18, 2016
Summary:
There was an issue with the way we made `RefreshControl` a controlled component when react doesn't render synchronously. This fixes the issue by using the same technique used in the commit 0cd2904 for`PickerAndroid`

**Test plan (required)**

Tested normal behaviour and tested that if not setting the `refreshing` prop to `true` during the `onRefresh` callback that the `RefreshControl` stops refreshing immediately. Also made sure that `setNativeProps` is only called if needed when the native refreshing state is not in sync with JS.

Fix facebook#7414
Closes facebook#7445

Differential Revision: D3274981

fb-gh-sync-id: a1b9d46329f552984e33d11fa0e29ad6da689511
fbshipit-source-id: a1b9d46329f552984e33d11fa0e29ad6da689511
samerce pushed a commit to iodine/react-native that referenced this issue Aug 23, 2016
Summary:
There was an issue with the way we made `RefreshControl` a controlled component when react doesn't render synchronously. This fixes the issue by using the same technique used in the commit 0cd2904 for`PickerAndroid`

**Test plan (required)**

Tested normal behaviour and tested that if not setting the `refreshing` prop to `true` during the `onRefresh` callback that the `RefreshControl` stops refreshing immediately. Also made sure that `setNativeProps` is only called if needed when the native refreshing state is not in sync with JS.

Fix facebook#7414
Closes facebook#7445

Differential Revision: D3274981

fb-gh-sync-id: a1b9d46329f552984e33d11fa0e29ad6da689511
fbshipit-source-id: a1b9d46329f552984e33d11fa0e29ad6da689511
mpretty-cyro pushed a commit to HomePass/react-native that referenced this issue Aug 25, 2016
Summary:
There was an issue with the way we made `RefreshControl` a controlled component when react doesn't render synchronously. This fixes the issue by using the same technique used in the commit 0cd2904 for`PickerAndroid`

**Test plan (required)**

Tested normal behaviour and tested that if not setting the `refreshing` prop to `true` during the `onRefresh` callback that the `RefreshControl` stops refreshing immediately. Also made sure that `setNativeProps` is only called if needed when the native refreshing state is not in sync with JS.

Fix facebook#7414
Closes facebook#7445

Differential Revision: D3274981

fb-gh-sync-id: a1b9d46329f552984e33d11fa0e29ad6da689511
fbshipit-source-id: a1b9d46329f552984e33d11fa0e29ad6da689511
@facebook facebook locked as resolved and limited conversation to collaborators May 24, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 19, 2018
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants