fix: Only add beforeunload listener as needed (#303)

* fix: Only add the beforeunload listener as needed This plays better with the page navigation cache. * more
4Catalyzer · Mar 28, 2020 · 250acbd · 250acbd
1 parent 168805f
commit 250acbd
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 50 deletions.
diff --git a/src/createTransitionHookMiddleware.js b/src/createTransitionHookMiddleware.js
@@ -69,16 +69,46 @@ export default function createTransitionHookMiddleware({
   let nextStep = null;
   let hooks = [];
 
+  const onBeforeUnload = useBeforeUnload
+    ? /* istanbul ignore next: not testable with Karma */
+      (event) => {
+        const syncResult = runHooks(hooks, null, (result) => result);
+
+        if (syncResult === true || syncResult === undefined) {
+          // An asynchronous transition hook usually means there will be a
+          //  custom confirm dialog. However, we'll already be showing the
+          //  before unload dialog, and there's no way to prevent the custom
+          //  dialog from showing. In such cases, the application code will
+          //  need to explicitly handle the null location anyway, so don't
+          //  potentially show two confirmation dialogs.
+          return undefined;
+        }
+
+        const resultSafe = syncResult || '';
+
+        event.returnValue = resultSafe; // eslint-disable-line no-param-reassign
+        return resultSafe;
+      }
+    : null;
+
   function addHook(hook) {
+    // Add the beforeunload event listener only as needed, as its presence
+    //  prevents the page from being added to the page navigation cache.
+    if (hooks.length === 0 && onBeforeUnload) {
+      window.addEventListener('beforeunload', onBeforeUnload);
+    }
+
     hooks.push(hook);
 
     return () => {
       hooks = hooks.filter((item) => item !== hook);
+
+      if (hooks.length === 0 && onBeforeUnload) {
+        window.removeEventListener('beforeunload', onBeforeUnload);
+      }
     };
   }
 
-  let onBeforeUnload = null;
-
   function transitionHookMiddleware({ dispatch }) {
     return (next) => (action) => {
       const { type, payload } = action;
@@ -90,33 +120,6 @@ export default function createTransitionHookMiddleware({
       }
 
       switch (type) {
-        case ActionTypes.INIT:
-          // Only attach this listener once.
-          if (useBeforeUnload && !onBeforeUnload) {
-            /* istanbul ignore next: not testable with Karma */
-            onBeforeUnload = (event) => {
-              const syncResult = runHooks(hooks, null, (result) => result);
-
-              if (syncResult === true || syncResult === undefined) {
-                // An asynchronous transition hook usually means there will be
-                // a custom confirm dialog. However, we'll already be showing
-                // the before unload dialog, and there's no way to prevent the
-                // custom dialog from showing. In such cases, the application
-                // code will need to explicitly handle the null location
-                // anyway, so don't potentially show two confirmation dialogs.
-                return undefined;
-              }
-
-              const resultSafe = syncResult || '';
-
-              event.returnValue = resultSafe; // eslint-disable-line no-param-reassign
-              return resultSafe;
-            };
-
-            window.addEventListener('beforeunload', onBeforeUnload);
-          }
-
-          return next(action);
         case ActionTypes.TRANSITION:
           return runAllowTransition(hooks, payload, (allowTransition) => {
             if (!allowTransition) {
@@ -204,9 +207,8 @@ export default function createTransitionHookMiddleware({
           return undefined;
         }
         case ActionTypes.DISPOSE:
-          if (onBeforeUnload) {
+          if (hooks.length > 0 && onBeforeUnload) {
             window.removeEventListener('beforeunload', onBeforeUnload);
-            onBeforeUnload = null;
           }
 
           return next(action);

diff --git a/test/createTransitionHookMiddleware.test.js b/test/createTransitionHookMiddleware.test.js
@@ -417,28 +417,44 @@ describe('createTransitionHookMiddleware', () => {
     });
   });
 
-  it('should manage event listeners with useBeforeUnload', () => {
-    // Get rid of the old store. We'll replace it with a new one.
-    store.dispatch(Actions.dispose());
+  describe('useBeforeUnload', () => {
+    beforeEach(() => {
+      // Get rid of the old store. We'll replace it with a new one.
+      store.dispatch(Actions.dispose());
 
-    sandbox.stub(window, 'addEventListener');
-    sandbox.stub(window, 'removeEventListener');
+      sandbox.stub(window, 'addEventListener');
+      sandbox.stub(window, 'removeEventListener');
 
-    store = createStore(
-      () => null,
-      createHistoryEnhancer({ protocol, useBeforeUnload: true }),
-    );
+      store = createStore(
+        () => null,
+        createHistoryEnhancer({ protocol, useBeforeUnload: true }),
+      );
 
-    expect(window.addEventListener).not.to.have.been.called();
-    store.dispatch(Actions.init());
-    expect(window.addEventListener)
-      .to.have.been.calledOnce()
-      .and.to.have.been.called.with('beforeunload');
+      store.dispatch(Actions.init());
+    });
 
-    expect(window.removeEventListener).not.to.have.been.called();
-    store.dispatch(Actions.dispose());
-    expect(window.removeEventListener)
-      .to.have.been.calledOnce()
-      .and.to.have.been.called.with('beforeunload');
+    it('should manage event listener on adding and removing hook', () => {
+      expect(window.addEventListener).not.to.have.been.called();
+      const removeHook = store.farce.addTransitionHook(() => null);
+      expect(window.addEventListener)
+        .to.have.been.calledOnce()
+        .and.to.have.been.called.with('beforeunload');
+
+      expect(window.removeEventListener).not.to.have.been.called();
+      removeHook();
+      expect(window.removeEventListener)
+        .to.have.been.calledOnce()
+        .and.to.have.been.called.with('beforeunload');
+    });
+
+    it('should remove event listener on dispose', () => {
+      store.farce.addTransitionHook(() => null);
+
+      expect(window.removeEventListener).not.to.have.been.called();
+      store.dispatch(Actions.dispose());
+      expect(window.removeEventListener)
+        .to.have.been.calledOnce()
+        .and.to.have.been.called.with('beforeunload');
+    });
   });
 });