alexbrillant / react-native-deck-swiper

tinder like react-native deck swiper
ISC License
1.55k stars 461 forks source link

Possible memory leak in onSwipedAll() #178

Open mcavaliere opened 6 years ago

mcavaliere commented 6 years ago

I'm getting the following error while using react-native-deck-swiper within a react-navigation view that is connected to Redux:

Warning: Can't call setState (or forceUpdate) on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.

It happens when I dispatch an action telling react-navigation to redirect me to another screen inside the swiper's onSwipedAll() callback.

What appears to be happening is a race condition within incrementCardIndex() .

  incrementCardIndex = onSwiped => {
    // ...

    this.onSwipedCallbacks(onSwiped, swipedAllCards)
    this.setCardIndex(newCardIndex, swipedAllCards)
  }

 // ...

  setCardIndex = (newCardIndex, swipedAllCards) => {
    this.setState(
      // ...
    )
  }

Since the callback is getting fired before setCardIndex(), it's trying to redirect the screen (and triggering an unmount) before setState() gets called.

MarcoMig commented 5 years ago

I'm having the same issue, did you solve it?

mcavaliere commented 5 years ago

@MarcoMig I had to restructure the app a bit to do what I want, and it got resolved.

But I think the workaround came from avoiding calling setState() inside onSwipedAll(), and letting onSwipedAll() only dispatch a redirect action. That way setState() was only called before the redirect, and finished way before any unmounting occurred.

In my case it looked something like this:

{/* render() */}
<Swiper
  onSwiped={ (index) => { this.onSwiped(index) } }
  onSwipedAll={ this.props.onComplete }
/>
// this.onSwiped
onSwiped(cardIndex) {
  // This was the magic part for me. Since this will never happen for the last card in the stack, 
  //   it never runs the risk of calling the setState() below at the same time that onSwipedAll() 
  //   triggers the dispatch of the navigation action. 
  if (cardIndex+2 <= Constants.QUESTION_COUNT) {
    this.setState({
      // Update this to the index of the next card.
      currentCardIndex: cardIndex+1,

      // Below we add another +1 to translate from zero-based indexes to
      //  1-based card numbering.
      currentCardNumber: cardIndex+2
    });
  }
}
const mapDispatchToProps = (dispatch) => () => {
  return {
    goToResults: () => {
      dispatch(
        NavigationActions.navigate({ routeName: 'Results' })
      );
    }
  }
}

This is only partial code, but hopefully it helps clarify. Let me know if it works out for you.

MarcoMig commented 5 years ago

Thanks for the answer, I found another workaround that I think is also valid. I added an new property on componentWillMount = () => { ... this._mounted = true .... } and on componentWillUnmount: componentWillUnmount = () => { this._mounted = false ... }

Now before to call the setCarIndex we check if the component is mounted: setCardIndex = (newCardIndex, swipedAllCards) => { if (this._mounted) { this.setState( { ...this.calculateCardIndexes(newCardIndex, this.state.cards), swipedAllCards: swipedAllCards, panResponderLocked: false }, this.resetPanAndScale ) } }

Hope this might can help someone else

omrihaim commented 5 years ago

Hi @MarcoMig , is your change part of 1.5.26? As i am currently getting this error, while I don't set state when all cards are swiped called.

Thanks

ViktorPontinen commented 5 years ago

Getting same error/warning for unknown(to me) reason.

React Native: 0.57

Any ideas? Currently have this in render:

  render() {
    let { loading } = this.state
    // From redux store
    let { cards, cardIndex } = this.props

    if (loading) {
      return (
        <WrapperSplashCenter>
          <Spinner size={50} thickness={2} />
        </WrapperSplashCenter>
      )
    } else if (Array.isArray(cards) && cards.length > 0 && cardIndex < cards.length) {
      return (
        <WrapperSplashCenter>
          <Swiper
            cards={cards}
            cardIndex={cardIndex}
            showSecondCard={true}
            renderCard={card => {
              card.type === 'ad' ? <AdCard card={card} /> : <Card card={card} />
            }}
            verticalSwipe={false}
            backgroundColor='transparent'
            cardVerticalMargin={50 / 2}
            cardHorizontalMargin={50 / 2}
            onSwipedLeft={index => this._onDislike(index)}
            onSwipedRight={index => this._onLike(index)}
            onTapCard={index => this._onCardPress(index)}
            animateCardOpacity={true}
            animateOverlayLabelOpacity={true}
          />
        </WrapperSplashCenter>
      )
    } else {
      return <NoFound retry={() => this.retryLoadCards} />
    }
  }

EDIT: Here's the relevant stack trace:

Warning: Can't call %s on a component that is not yet mounted. This is a no-op, but it might indicate a bug in your application. Instead, assign to `this.state` directly or define a `state = {};` class property with the desired state in the %s component., forceUpdate, Swiper
- node_modules/react/cjs/react.development.js:217:39 in warningWithoutStack
- node_modules/react/cjs/react.development.js:244:26 in warnNoop
- node_modules/react/cjs/react.development.js:280:13 in enqueueForceUpdate
- node_modules/react/cjs/react.development.js:382:34 in forceUpdate
- node_modules/react-native-deck-swiper/Swiper.js:113:6 in computeCardStyle
- node_modules/react-native-deck-swiper/Swiper.js:118:4 in initializeCardStyle
- node_modules/react-native-deck-swiper/Swiper.js:75:4 in Swiper

Also attempted a fix by moving this.initializeCardStyle()in the constructor to inside a componentDidMount(), seems to be working but will do some more testing. No idea if this will have any unwanted side-effects.

KentonParton commented 5 years ago

@mcavaliere For some context, just a few moments ago I was using version 1.5.22 and I was experiencing this same error but only when the last card was swiped. I just updated to the latest version 1.6.4 and I am now prompted with a similar message stating "Warning: Can't call "forceUpdate" on a component that is not yet mounted..." After commenting out the "this.forceUpdate in Swiper.js on line 111 I no longer get the warning on startup or on the last card.

@alexbrillant is it fine to remove the first forceUpdate in the "initializeCardStyle()"? Thanks for your contribution.

 initializeCardStyle = () => {
    this.forceUpdate()
    Dimensions.addEventListener('change', () => {
      this.forceUpdate()
    })
  }