[RN 0.58] removeOrientationListener doesn't remove the listener

lschuft commented 5 years ago

Hi there, thank for you for this package.

I'm using it in the Login screen of an app I'm making, and I'm having an issue using the removeOrientationListener method. When I debug, I see that it does not remove the subscription, so when I change screens, I get the following warning:

Warning: Can't perform a React state update 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.
ExceptionsManager.js:82
    in Login (created by Context.Consumer)
    in Connect(Login) (at screens.js:43)
    in NetworkConnectivity (at ReduxNetworkProvider.js:47)
    in ReduxNetworkProvider (created by Context.Consumer)
    in Connect(ReduxNetworkProvider) (at screens.js:42)
    in Provider (at screens.js:41)
    in StoreWrapper (at ComponentWrapper.js:29)
    in WrappedComponent (at renderApplication.js:34)
    in RCTView (at View.js:45)
    in View (at AppContainer.js:98)
    in RCTView (at View.js:45)
    in View (at AppContainer.js:115)
    in AppContainer (at renderApplication.js:33)

I'm using RN 0.58.4. The issue is in EventEmitter.js:213, in the removeListener method. This requires that the original handler specified in listenOrientationChange be included in the parameters (right now, an empty function is passed instead) because of this check:

// The subscription may have been removed during this event loop.
// its listener matches the listener in method parameters
if (subscription && subscription.listener === listener) {
  subscription.remove();
}

Perhaps this was a change in recent RN version. I needed a quick fix so I patched up index.js of this library to workaround this problem, like this:

const listenOrientationChange = that => {
  //Save the handler to a property in the Component
  that.orientationChangeHandler = newDimensions => {
    // Retrieve and save new dimensions
    screenWidth = newDimensions.window.width;
    screenHeight = newDimensions.window.height;

    // Trigger screen's rerender with a state update of the orientation variable
    that.setState({
      orientation: screenWidth < screenHeight ? 'portrait' : 'landscape'
    });
  }

  Dimensions.addEventListener('change', that.orientationChangeHandler);
};

const removeOrientationListener = that => {
  //Retrieve the original handler stored in the Component
  //Requires passing (this) as a parameter to this function, too
  Dimensions.removeEventListener('change', that.orientationChangeHandler);
};

This solved the problem in my case, although a prettier solution should be possible (don't think it's okay to mess with the component properties...).

Please let me know if something isn't clear. I don't include the code of my Login page because it follows exactly this README example.

manelsanz commented 5 years ago

I have the same problem. And I take advantage to ask for a global listener for detect orientation changes, in order to set current orientation on Redux when it changes and use it along all my app. Thank you, @marudy .