Repository: tannerlinsley/react-query
Pull request #3092 (2021-12-14 08:10:15)

fix: unsubscribe leak

vercel[bot]: This pull request is being automatically deployed with Vercel ([learn more](https://vercel.link/github-learn-more)).  
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: [https://vercel.com/tanstack/react-query/3dSiHoipiGJDDnicxmMiZ4WwQRba](https://vercel.com/tanstack/react-query/3dSiHoipiGJDDnicxmMiZ4WwQRba)  
✅ Preview: *In Progress*


codesandbox[bot]: This pull request is automatically built and testable in [CodeSandbox](https://codesandbox.io).

  To see build info of the built libraries, click [here](https://ci.codesandbox.io/status/tannerlinsley/react-query/pr/3092/builds/199141) or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b789930cffde118ad10dd3b0f250e454196b7f98:

|Sandbox| Source |
|--|--|
|[tannerlinsley/react-query: basic](https://codesandbox.io/s/tannerlinsley-react-query-basic-bsxmc)| Configuration |
|[tannerlinsley/react-query: basic-typescript](https://codesandbox.io/s/tannerlinsley-react-query-basic-typescript-94yuz)| Configuration |

codecov[bot]: # [Codecov](https://codecov.io/gh/tannerlinsley/react-query/pull/3092?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley) Report
> Merging [#3092](https://codecov.io/gh/tannerlinsley/react-query/pull/3092?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley) (b789930) into [master](https://codecov.io/gh/tannerlinsley/react-query/commit/ca335dec7b898592bea49734c4d121d1fd125cdf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley) (ca335de) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/tannerlinsley/react-query/pull/3092/graphs/tree.svg?width=650&height=150&src=pr&token=jqEbswkEDQ&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley)](https://codecov.io/gh/tannerlinsley/react-query/pull/3092?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley)

```diff
@@            Coverage Diff             @@
##           master    #3092      +/-   ##
==========================================
+ Coverage   96.46%   96.47%   +0.01%     
==========================================
  Files          45       45              
  Lines        2261     2271      +10     
  Branches      641      641              
==========================================
+ Hits         2181     2191      +10     
  Misses         77       77              
  Partials        3        3              
```


| [Impacted Files](https://codecov.io/gh/tannerlinsley/react-query/pull/3092?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley) | Coverage Δ | |
|---|---|---|
| [src/core/focusManager.ts](https://codecov.io/gh/tannerlinsley/react-query/pull/3092/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley#diff-c3JjL2NvcmUvZm9jdXNNYW5hZ2VyLnRz) | `100.00% <100.00%> (ø)` | |
| [src/core/onlineManager.ts](https://codecov.io/gh/tannerlinsley/react-query/pull/3092/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley#diff-c3JjL2NvcmUvb25saW5lTWFuYWdlci50cw==) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/tannerlinsley/react-query/pull/3092?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/tannerlinsley/react-query/pull/3092?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley). Last update [ca335de...b789930](https://codecov.io/gh/tannerlinsley/react-query/pull/3092?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley).


Patch:
diff --git a/src/core/focusManager.ts b/src/core/focusManager.ts
index 18bfdcbc4..db08d0fb9 100644
--- a/src/core/focusManager.ts
+++ b/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()
diff --git a/src/core/onlineManager.ts b/src/core/onlineManager.ts
index f02a03c9b..6efcf75aa 100644
--- a/src/core/onlineManager.ts
+++ b/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()
diff --git a/src/core/queryObserver.ts b/src/core/queryObserver.ts
index aa36cfd2c..0e412f632 100644
--- a/src/core/queryObserver.ts
+++ b/src/core/queryObserver.ts
@@ -790,9 +790,7 @@ function shouldFetchOptionally(
   return (
     options.enabled !== false &&
     (query !== prevQuery || prevOptions.enabled === false) &&
-    (!options.suspense ||
-      query.state.status !== 'error' ||
-      prevOptions.enabled === false) &&
+    (!options.suspense || query.state.status !== 'error') &&
     isStale(query, options)
   )
 }


Test patch:
diff --git a/src/core/tests/focusManager.test.tsx b/src/core/tests/focusManager.test.tsx
index ca071ec16..ad32115d2 100644
--- a/src/core/tests/focusManager.test.tsx
+++ b/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()
+  })
 })
diff --git a/src/core/tests/onlineManager.test.tsx b/src/core/tests/onlineManager.test.tsx
index 2e7d35c3f..fb1855cfb 100644
--- a/src/core/tests/onlineManager.test.tsx
+++ b/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()
+  })
 })
diff --git a/src/react/tests/suspense.test.tsx b/src/react/tests/suspense.test.tsx
index 06c1d6dd3..8ed6d58bb 100644
--- a/src/react/tests/suspense.test.tsx
+++ b/src/react/tests/suspense.test.tsx
@@ -891,6 +891,76 @@ describe("useQuery's in Suspense mode", () => {
     consoleMock.mockRestore()
   })
 
+  it('should error catched in error boundary without infinite loop when enabled changed', async () => {
+    const consoleMock = mockConsoleError()
+
+    const succeed = false
+
+    function Page() {
+      const queryKeys = '1'
+      const [enabled, setEnabled] = React.useState(false)
+
+      const result = useQuery(
+        [queryKeys],
+        async () => {
+          await sleep(10)
+          if (!succeed) {
+            throw new Error('Suspense Error Bingo')
+          } else {
+            return 'data'
+          }
+        },
+        {
+          retry: false,
+          suspense: true,
+          enabled,
+        }
+      )
+      return (
+        <div>
+          <span>rendered</span> <span>{result.data}</span>
+          <button
+            aria-label="fail"
+            onClick={() => {
+              setEnabled(true)
+            }}
+          >
+            fail
+          </button>
+        </div>
+      )
+    }
+
+    function App() {
+      const { reset } = useQueryErrorResetBoundary()
+      return (
+        <ErrorBoundary
+          onReset={reset}
+          fallbackRender={() => <div>error boundary</div>}
+        >
+          <React.Suspense fallback="Loading...">
+            <Page />
+          </React.Suspense>
+        </ErrorBoundary>
+      )
+    }
+
+    const rendered = renderWithClient(queryClient, <App />)
+
+    // render empty data with 'rendered' when enabled is false
+    await waitFor(() => rendered.getByText('rendered'))
+
+    // change enabled to true
+    fireEvent.click(rendered.getByLabelText('fail'))
+
+    // render pending fallback
+    await waitFor(() => rendered.getByText('Loading...'))
+
+    // render error boundary fallback (error boundary)
+    await waitFor(() => rendered.getByText('error boundary'))
+    consoleMock.mockRestore()
+  })
+
   it('should render the correct amount of times in Suspense mode when cacheTime is set to 0', async () => {
     const key = queryKey()
     let state: UseQueryResult<number> | null = null