farce icon indicating copy to clipboard operation
farce copied to clipboard

Calling "DISPOSE" action doesn't result in removing navigation listeners

Open catamphetamine opened this issue 4 months ago • 0 comments

In createNavigationListenerMiddleware.js it handles a ActionTypes.DISPOSE action:

https://github.com/4Catalyzer/farce/blob/5f79cb9c2bbae44a8c4ab7e9ae0afa8b5657f966/src/createNavigationListenerMiddleware.js#L223-L228

But there seems to be a couple of issues with that code:

  • The condition listenerEntries.length > 0 && onBeforeUnload seems to be incorrect:
    • onBeforeUnload is a function name rather than a variable name, so it always exists.
    • A more appropriate condition would've been listenerEntries.some((item) => item.beforeUnload)
  • Safe coding practices would also require emptying the list of listenerEntries themselves during the "dispose" phase.

The code above also just happens to be written in such a way that it both isn't correct but at the same time always has the correct outcome because it always removes that event listener on DISPOSE which it's supposed to do. So it's a bit funny that it introduces no bugs at all.

My fix would've been:

case ActionTypes.DISPOSE:
  if (listenerEntries.some((item) => item.beforeUnload)) {
    removeBeforeUnloadListener(onBeforeUnload);
  }
  listenerEntries = [];

  return next(action);

If anyone's interested, I published this workaround as part of navigation-stack package: https://www.npmjs.com/package/navigation-stack

catamphetamine avatar Aug 04 '25 15:08 catamphetamine