fix: unsubscribe correctly in onlineManager and focusManager (TanStack#3092)

Author: TkDodo

src/core/focusManager.ts

@@ -1,23 +1,51 @@
 import { Subscribable } from './subscribable'
 import { isServer } from './utils'
 
-class FocusManager extends Subscribable {
+type SetupFn = (
+  setFocused: (focused?: boolean) => void
+) => (() => void) | undefined
+
+export class FocusManager extends Subscribable {
   private focused?: boolean
-  private removeEventListener?: () => void
+  private cleanup?: () => void
+
+  private setup: SetupFn
+
+  constructor() {
+    super()
+    this.setup = onFocus => {
+      if (!isServer && window?.addEventListener) {
+        const listener = () => onFocus()
+        // Listen to visibillitychange and focus
+        window.addEventListener('visibilitychange', listener, false)
+        window.addEventListener('focus', listener, false)
+
+        return () => {
+          // Be sure to unsubscribe if a new handler is set
+          window.removeEventListener('visibilitychange', listener)
+          window.removeEventListener('focus', listener)
+        }
+      }
+    }
+  }
 
   protected onSubscribe(): void {
-    if (!this.removeEventListener) {
-      this.setDefaultEventListener()
+    if (!this.cleanup) {
+      this.setEventListener(this.setup)
     }
   }
 
-  setEventListener(
-    setup: (setFocused: (focused?: boolean) => void) => () => void
-  ): void {
-    if (this.removeEventListener) {
-      this.removeEventListener()
+  protected onUnsubscribe() {
+    if (!this.hasListeners()) {
+      this.cleanup?.()
+      this.cleanup = undefined
     }
-    this.removeEventListener = setup(focused => {
+  }
+
+  setEventListener(setup: SetupFn): void {
+    this.setup = setup
+    this.cleanup?.()
+    this.cleanup = setup(focused => {
       if (typeof focused === 'boolean') {
         this.setFocused(focused)
       } else {
@@ -54,23 +82,6 @@ class FocusManager extends Subscribable {
       document.visibilityState
     )
   }
-
-  private setDefaultEventListener() {
-    if (!isServer && window?.addEventListener) {
-      this.setEventListener(onFocus => {
-        const listener = () => onFocus()
-        // Listen to visibillitychange and focus
-        window.addEventListener('visibilitychange', listener, false)
-        window.addEventListener('focus', listener, false)
-
-        return () => {
-          // Be sure to unsubscribe if a new handler is set
-          window.removeEventListener('visibilitychange', listener)
-          window.removeEventListener('focus', listener)
-        }
-      })
-    }
-  }
 }
 
 export const focusManager = new FocusManager()

src/core/onlineManager.ts

@@ -1,23 +1,51 @@
 import { Subscribable } from './subscribable'
 import { isServer } from './utils'
 
-class OnlineManager extends Subscribable {
+type SetupFn = (
+  setOnline: (online?: boolean) => void
+) => (() => void) | undefined
+
+export class OnlineManager extends Subscribable {
   private online?: boolean
-  private removeEventListener?: () => void
+  private cleanup?: () => void
+
+  private setup: SetupFn
+
+  constructor() {
+    super()
+    this.setup = onOnline => {
+      if (!isServer && window?.addEventListener) {
+        const listener = () => onOnline()
+        // Listen to online
+        window.addEventListener('online', listener, false)
+        window.addEventListener('offline', listener, false)
+
+        return () => {
+          // Be sure to unsubscribe if a new handler is set
+          window.removeEventListener('online', listener)
+          window.removeEventListener('offline', listener)
+        }
+      }
+    }
+  }
 
   protected onSubscribe(): void {
-    if (!this.removeEventListener) {
-      this.setDefaultEventListener()
+    if (!this.cleanup) {
+      this.setEventListener(this.setup)
     }
   }
 
-  setEventListener(
-    setup: (setOnline: (online?: boolean) => void) => () => void
-  ): void {
-    if (this.removeEventListener) {
-      this.removeEventListener()
+  protected onUnsubscribe() {
+    if (!this.hasListeners()) {
+      this.cleanup?.()
+      this.cleanup = undefined
     }
-    this.removeEventListener = setup((online?: boolean) => {
+  }
+
+  setEventListener(setup: SetupFn): void {
+    this.setup = setup
+    this.cleanup?.()
+    this.cleanup = setup((online?: boolean) => {
       if (typeof online === 'boolean') {
         this.setOnline(online)
       } else {
@@ -54,23 +82,6 @@ class OnlineManager extends Subscribable {
 
     return navigator.onLine
   }
-
-  private setDefaultEventListener() {
-    if (!isServer && window?.addEventListener) {
-      this.setEventListener(onOnline => {
-        const listener = () => onOnline()
-        // Listen to online
-        window.addEventListener('online', listener, false)
-        window.addEventListener('offline', listener, false)
-
-        return () => {
-          // Be sure to unsubscribe if a new handler is set
-          window.removeEventListener('online', listener)
-          window.removeEventListener('offline', listener)
-        }
-      })
-    }
-  }
 }
 
 export const onlineManager = new OnlineManager()

src/core/tests/focusManager.test.tsx

@@ -1,10 +1,10 @@
 import { sleep } from '../utils'
-import { focusManager } from '../focusManager'
+import { FocusManager } from '../focusManager'
 
 describe('focusManager', () => {
-  afterEach(() => {
-    // Reset removeEventListener private property to avoid side effects between tests
-    focusManager['removeEventListener'] = undefined
+  let focusManager: FocusManager
+  beforeEach(() => {
+    focusManager = new FocusManager()
   })
 
   it('should call previous remove handler when replacing an event listener', () => {
@@ -61,16 +61,14 @@ describe('focusManager', () => {
     globalThis.document = document
   })
 
-  it('should not set window listener if window.addEventListener is not defined', async () => {
+  test('cleanup should still be undefined if window.addEventListener is not defined', async () => {
     const { addEventListener } = globalThis.window
 
     // @ts-expect-error
     globalThis.window.addEventListener = undefined
 
-    const setEventListenerSpy = jest.spyOn(focusManager, 'setEventListener')
-
     const unsubscribe = focusManager.subscribe()
-    expect(setEventListenerSpy).toHaveBeenCalledTimes(0)
+    expect(focusManager['cleanup']).toBeUndefined()
 
     unsubscribe()
     globalThis.window.addEventListener = addEventListener
@@ -103,4 +101,43 @@ describe('focusManager', () => {
     addEventListenerSpy.mockRestore()
     removeEventListenerSpy.mockRestore()
   })
+
+  test('should call removeEventListener when last listener unsubscribes', () => {
+    const addEventListenerSpy = jest.spyOn(
+      globalThis.window,
+      'addEventListener'
+    )
+
+    const removeEventListenerSpy = jest.spyOn(
+      globalThis.window,
+      'removeEventListener'
+    )
+
+    const unsubscribe1 = focusManager.subscribe(() => undefined)
+    const unsubscribe2 = focusManager.subscribe(() => undefined)
+    expect(addEventListenerSpy).toHaveBeenCalledTimes(2) // visibilitychange + focus
+
+    unsubscribe1()
+    expect(removeEventListenerSpy).toHaveBeenCalledTimes(0)
+    unsubscribe2()
+    expect(removeEventListenerSpy).toHaveBeenCalledTimes(2) // visibilitychange + focus
+  })
+
+  test('should keep setup function even if last listener unsubscribes', () => {
+    const setupSpy = jest.fn().mockImplementation(() => () => undefined)
+
+    focusManager.setEventListener(setupSpy)
+
+    const unsubscribe1 = focusManager.subscribe(() => undefined)
+
+    expect(setupSpy).toHaveBeenCalledTimes(1)
+
+    unsubscribe1()
+
+    const unsubscribe2 = focusManager.subscribe(() => undefined)
+
+    expect(setupSpy).toHaveBeenCalledTimes(2)
+
+    unsubscribe2()
+  })
 })

src/core/tests/onlineManager.test.tsx

@@ -1,10 +1,10 @@
-import { onlineManager } from '../onlineManager'
+import { OnlineManager } from '../onlineManager'
 import { sleep } from '../utils'
 
 describe('onlineManager', () => {
-  afterEach(() => {
-    // Reset removeEventListener private property to avoid side effects between tests
-    onlineManager['removeEventListener'] = undefined
+  let onlineManager: OnlineManager
+  beforeEach(() => {
+    onlineManager = new OnlineManager()
   })
 
   test('isOnline should return true if navigator is undefined', () => {
@@ -56,16 +56,14 @@ describe('onlineManager', () => {
     expect(remove2Spy).not.toHaveBeenCalled()
   })
 
-  test('setEventListener should not set window listener if window.addEventListener is not defined', async () => {
+  test('cleanup should still be undefined if window.addEventListener is not defined', async () => {
     const { addEventListener } = globalThis.window
 
     // @ts-expect-error
     globalThis.window.addEventListener = undefined
 
-    const setEventListenerSpy = jest.spyOn(onlineManager, 'setEventListener')
-
     const unsubscribe = onlineManager.subscribe()
-    expect(setEventListenerSpy).toHaveBeenCalledTimes(0)
+    expect(onlineManager['cleanup']).toBeUndefined()
 
     unsubscribe()
     globalThis.window.addEventListener = addEventListener
@@ -98,4 +96,43 @@ describe('onlineManager', () => {
     addEventListenerSpy.mockRestore()
     removeEventListenerSpy.mockRestore()
   })
+
+  test('should call removeEventListener when last listener unsubscribes', () => {
+    const addEventListenerSpy = jest.spyOn(
+      globalThis.window,
+      'addEventListener'
+    )
+
+    const removeEventListenerSpy = jest.spyOn(
+      globalThis.window,
+      'removeEventListener'
+    )
+
+    const unsubscribe1 = onlineManager.subscribe(() => undefined)
+    const unsubscribe2 = onlineManager.subscribe(() => undefined)
+    expect(addEventListenerSpy).toHaveBeenCalledTimes(2) // online + offline
+
+    unsubscribe1()
+    expect(removeEventListenerSpy).toHaveBeenCalledTimes(0)
+    unsubscribe2()
+    expect(removeEventListenerSpy).toHaveBeenCalledTimes(2) // online + offline
+  })
+
+  test('should keep setup function even if last listener unsubscribes', () => {
+    const setupSpy = jest.fn().mockImplementation(() => () => undefined)
+
+    onlineManager.setEventListener(setupSpy)
+
+    const unsubscribe1 = onlineManager.subscribe(() => undefined)
+
+    expect(setupSpy).toHaveBeenCalledTimes(1)
+
+    unsubscribe1()
+
+    const unsubscribe2 = onlineManager.subscribe(() => undefined)
+
+    expect(setupSpy).toHaveBeenCalledTimes(2)
+
+    unsubscribe2()
+  })
 })