[RN 0.58] removeOrientationListener doesn't remove the listener

[lschuft]

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]

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 .

[miguelnfuertesc]

Thank you! @lschuft ... it works!

[marudy]

Hi there @lschuft and thanks for taking the time to write this detailed post.

Sorry I've been a bit busy of the past weeks and reply earlier. Will have a look soon and come back with a fix and a new release! :)

@manelsanz if you had a global listener you would still need to change/set height, width props per screen right? Or is it only the orientation change that you are thinking of not rewriting?

[manelsanz]

@marudy I want to register a global listener in order to know when screen orientation changes. On this listener, I would store the new orientation on Redux so through all my screens I could know the current orientation of screen. When state of Redux is updated, I would detect it in all my screens and will change style depending on orientation.

[marudy]

Hey guys, my time is very limited during this period. I would strongly appreciate if someone could take the time and create a PR for this 👍

[amedkml]

@lschuft Can you please approve that this approach is working as expected!

You can find a working example under development/examples/responsive-screen-orientation-change

[marudy]

@AhmedMKamal thanks again for offering a PR :) @lschuft can you have a look as well in order to get this PR ready? Thanks

[lschuft]

Thanks @AhmedMKamal ! @marudy I'm a bit busy with tons of work this week, will try to look into this in the weekend so we can close this.

[marudy]

Yes please @lschuft ! 👍

[marudy]

Hey @lschuft I know you are busy man, but if you could review here it would be great! Cheers