fix(onlineManager): always initialize with online: true (#5714)

* fix(onlineManager): always initialize with `online: true`

instead of relying on navigator.onLine, because that indicator is broken AF in chrome

https://bugs.chromium.org/p/chromium/issues/list?q=navigator.online

* docs: document subscribe methods

* docs

* test: fix types in tests

setting to undefined is now illegal, so we have to reset to `true`, which is the default now

* fix(tests): switch from mocking navigator.onLine to mocking onlineManager.isOnline

* fix: offline toggle in devtools

it should now be enough to set online to true / false, without firing the event, because we always set & respect that value. we don't override the event handler, so real online / offline events might interfere here

* chore: fix tests

with the implementation of onlineManager, where we default to `true`, we need an explicit `'offline'` event to get it to false; otherwise, switching back to true doesn't change the state, so subscribers are not informed

* chore: prettier write

* chore: fix eslint

* chore: delete an old, flaky test that doesn't test much

Author: TkDodo

docs/react/guides/migrating-to-v5.md

@@ -276,6 +276,14 @@ There are some caveats to this change however, which you must be aware of:
 
 The `visibilitychange` event is used exclusively now. This is possible because we only support browsers that support the `visibilitychange` event. This fixes a bunch of issues [as listed here](https://github.com/TanStack/query/pull/4805).
 
+### Network status no longer relies on the `navigator.onLine` property
+
+`navigator.onLine` doesn't work well in Chromium based browsers. There are [a lot of issues](https://bugs.chromium.org/p/chromium/issues/list?q=navigator.online) around false negatives, which lead to Queries being wrongfully marked as `offline`.
+
+To circumvent this, we now always start with `online: true` and only listen to `online` and `offline` events to update the status.
+
+This should reduce the likelihood of false negatives, however, it might mean false positives for offline apps that load via serviceWorkers, which can work even without an internet connection.
+
 ### Removed custom `context` prop in favor of custom `queryClient` instance
 
 In v4, we introduced the possibility to pass a custom `context` to all react-query hooks. This allowed for proper isolation when using MicroFrontends.

docs/react/reference/focusManager.md

@@ -10,6 +10,7 @@ It can be used to change the default event listeners or to manually change the f
 Its available methods are:
 
 - [`setEventListener`](#focusmanagerseteventlistener)
+- [`subscribe`](#focusmanagersubscribe)
 - [`setFocused`](#focusmanagersetfocused)
 - [`isFocused`](#focusmanagerisfocused)
 
@@ -33,9 +34,21 @@ focusManager.setEventListener((handleFocus) => {
 })
 ```
 
+## `focusManager.subscribe`
+
+`subscribe` can be used to subscribe to changes in the visibility state. It returns an unsubscribe function:
+
+```tsx
+import { focusManager } from '@tanstack/react-query'
+
+const unsubscribe = focusManager.subscribe(isVisible => {
+  console.log('isVisible', isVisible)
+})
+```
+
 ## `focusManager.setFocused`
 
-`setFocused` can be used to manually set the focus state. Set `undefined` to fallback to the default focus check.
+`setFocused` can be used to manually set the focus state. Set `undefined` to fall back to the default focus check.
 
 ```tsx
 import { focusManager } from '@tanstack/react-query'

docs/react/reference/onlineManager.md

@@ -3,13 +3,20 @@ id: OnlineManager
 title: OnlineManager
 ---
 
-The `OnlineManager` manages the online state within TanStack Query.
+The `OnlineManager` manages the online state within TanStack Query. It can be used to change the default event listeners or to manually change the online state.
 
-It can be used to change the default event listeners or to manually change the online state.
+> Per default, the `onlineManager` assumes an active network connection, and listens to the `online` and `offline` events on the `window` object to detect changes.
+
+> In previous versions, `navigator.onLine` was used to determine the network status. However, it doesn't work well in Chromium based browsers. There are [a lot of issues](https://bugs.chromium.org/p/chromium/issues/list?q=navigator.online) around false negatives, which lead to Queries being wrongfully marked as `offline`.
+
+> To circumvent this, we now always start with `online: true` and only listen to `online` and `offline` events to update the status.
+
+> This should reduce the likelihood of false negatives, however, it might mean false positives for offline apps that load via serviceWorkers, which can work even without an internet connection.
 
 Its available methods are:
 
 - [`setEventListener`](#onlinemanagerseteventlistener)
+- [`subscribe`](#onlinemanagersubscribe)
 - [`setOnline`](#onlinemanagersetonline)
 - [`isOnline`](#onlinemanagerisonline)
 
@@ -28,9 +35,21 @@ onlineManager.setEventListener(setOnline => {
 })
 ```
 
+## `onlineManager.subscribe`
+
+`subscribe` can be used to subscribe to changes in the online state. It returns an unsubscribe function:
+
+```tsx
+import { onlineManager } from '@tanstack/react-query'
+
+const unsubscribe = onlineManager.subscribe(isOnline => {
+  console.log('isOnline', isOnline)
+})
+```
+
 ## `onlineManager.setOnline`
 
-`setOnline` can be used to manually set the online state. Set `undefined` to fallback to the default online check.
+`setOnline` can be used to manually set the online state.
 
 ```tsx
 import { onlineManager } from '@tanstack/react-query'
@@ -40,14 +59,11 @@ onlineManager.setOnline(true)
 
 // Set to offline
 onlineManager.setOnline(false)
-
-// Fallback to the default online check
-onlineManager.setOnline(undefined)
 ```
 
 **Options**
 
-- `online: boolean | undefined`
+- `online: boolean`
 
 ## `onlineManager.isOnline`
 

packages/query-core/src/onlineManager.ts

@@ -1,14 +1,11 @@
 import { Subscribable } from './subscribable'
 import { isServer } from './utils'
 
-type SetupFn = (
-  setOnline: (online?: boolean) => void,
-) => (() => void) | undefined
+type Listener = (online: boolean) => void
+type SetupFn = (setOnline: Listener) => (() => void) | undefined
 
-const onlineEvents = ['online', 'offline'] as const
-
-export class OnlineManager extends Subscribable {
-  #online?: boolean
+export class OnlineManager extends Subscribable<Listener> {
+  #online = true
   #cleanup?: () => void
 
   #setup: SetupFn
@@ -19,17 +16,16 @@ export class OnlineManager extends Subscribable {
       // addEventListener does not exist in React Native, but window does
       // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
       if (!isServer && window.addEventListener) {
-        const listener = () => onOnline()
+        const onlineListener = () => onOnline(true)
+        const offlineListener = () => onOnline(false)
         // Listen to online
-        onlineEvents.forEach((event) => {
-          window.addEventListener(event, listener, false)
-        })
+        window.addEventListener('online', onlineListener, false)
+        window.addEventListener('offline', offlineListener, false)
 
         return () => {
           // Be sure to unsubscribe if a new handler is set
-          onlineEvents.forEach((event) => {
-            window.removeEventListener(event, listener)
-          })
+          window.removeEventListener('online', onlineListener)
+          window.removeEventListener('offline', offlineListener)
         }
       }
 
@@ -53,43 +49,22 @@ export class OnlineManager extends Subscribable {
   setEventListener(setup: SetupFn): void {
     this.#setup = setup
     this.#cleanup?.()
-    this.#cleanup = setup((online?: boolean) => {
-      if (typeof online === 'boolean') {
-        this.setOnline(online)
-      } else {
-        this.onOnline()
-      }
-    })
+    this.#cleanup = setup(this.setOnline.bind(this))
   }
 
-  setOnline(online?: boolean): void {
+  setOnline(online: boolean): void {
     const changed = this.#online !== online
 
     if (changed) {
       this.#online = online
-      this.onOnline()
+      this.listeners.forEach((listener) => {
+        listener(online)
+      })
     }
   }
 
-  onOnline(): void {
-    this.listeners.forEach((listener) => {
-      listener()
-    })
-  }
-
   isOnline(): boolean {
-    if (typeof this.#online === 'boolean') {
-      return this.#online
-    }
-
-    if (
-      typeof navigator === 'undefined' ||
-      typeof navigator.onLine === 'undefined'
-    ) {
-      return true
-    }
-
-    return navigator.onLine
+    return this.#online
   }
 }
 

packages/query-core/src/tests/hydration.test.tsx

@@ -5,7 +5,7 @@ import { MutationCache } from '../mutationCache'
 import {
   createQueryClient,
   executeMutation,
-  mockNavigatorOnLine,
+  mockOnlineManagerIsOnline,
   sleep,
 } from './utils'
 
@@ -347,7 +347,7 @@ describe('dehydration and rehydration', () => {
   test('should be able to dehydrate mutations and continue on hydration', async () => {
     const consoleMock = vi.spyOn(console, 'error')
     consoleMock.mockImplementation(() => undefined)
-    const onlineMock = mockNavigatorOnLine(false)
+    const onlineMock = mockOnlineManagerIsOnline(false)
 
     const serverAddTodo = vi
       .fn()

packages/query-core/src/tests/onlineManager.test.tsx

@@ -31,7 +31,7 @@ describe('onlineManager', () => {
   test('setEventListener should use online boolean arg', async () => {
     let count = 0
 
-    const setup = (setOnline: (online?: boolean) => void) => {
+    const setup = (setOnline: (online: boolean) => void) => {
       setTimeout(() => {
         count++
         setOnline(false)
@@ -154,19 +154,15 @@ describe('onlineManager', () => {
 
     onlineManager.subscribe(listener)
 
-    onlineManager.setOnline(true)
-    onlineManager.setOnline(true)
-
-    expect(listener).toHaveBeenCalledTimes(1)
-
     onlineManager.setOnline(false)
     onlineManager.setOnline(false)
 
-    expect(listener).toHaveBeenCalledTimes(2)
+    expect(listener).toHaveBeenNthCalledWith(1, false)
 
-    onlineManager.setOnline(undefined)
-    onlineManager.setOnline(undefined)
+    onlineManager.setOnline(true)
+    onlineManager.setOnline(true)
 
-    expect(listener).toHaveBeenCalledTimes(3)
+    expect(listener).toHaveBeenCalledTimes(2)
+    expect(listener).toHaveBeenNthCalledWith(2, true)
   })
 })

packages/query-core/src/tests/queryClient.test.tsx

@@ -1,7 +1,6 @@
 import { waitFor } from '@testing-library/react'
 import '@testing-library/jest-dom'
 
-import { vi } from 'vitest'
 import {
   MutationObserver,
   QueryObserver,
@@ -11,7 +10,7 @@ import {
 import { noop } from '../utils'
 import {
   createQueryClient,
-  mockNavigatorOnLine,
+  mockOnlineManagerIsOnline,
   queryKey,
   sleep,
 } from './utils'
@@ -1074,7 +1073,7 @@ describe('queryClient', () => {
       const key1 = queryKey()
       const queryFn1 = vi.fn<unknown[], string>().mockReturnValue('data1')
       await queryClient.fetchQuery({ queryKey: key1, queryFn: queryFn1 })
-      const onlineMock = mockNavigatorOnLine(false)
+      const onlineMock = mockOnlineManagerIsOnline(false)
 
       await queryClient.refetchQueries({ queryKey: key1 })
 
@@ -1088,7 +1087,7 @@ describe('queryClient', () => {
       queryClient.setQueryDefaults(key1, { networkMode: 'always' })
       const queryFn1 = vi.fn<unknown[], string>().mockReturnValue('data1')
       await queryClient.fetchQuery({ queryKey: key1, queryFn: queryFn1 })
-      const onlineMock = mockNavigatorOnLine(false)
+      const onlineMock = mockOnlineManagerIsOnline(false)
 
       await queryClient.refetchQueries({ queryKey: key1 })
 
@@ -1394,7 +1393,7 @@ describe('queryClient', () => {
       queryCacheOnFocusSpy.mockRestore()
       queryCacheOnOnlineSpy.mockRestore()
       mutationCacheResumePausedMutationsSpy.mockRestore()
-      onlineManager.setOnline(undefined)
+      onlineManager.setOnline(true)
     })
 
     test('should resume paused mutations when coming online', async () => {
@@ -1424,7 +1423,7 @@ describe('queryClient', () => {
         expect(observer1.getCurrentResult().status).toBe('success')
       })
 
-      onlineManager.setOnline(undefined)
+      onlineManager.setOnline(true)
     })
 
     test('should resume paused mutations one after the other when invoked manually at the same time', async () => {
@@ -1459,7 +1458,7 @@ describe('queryClient', () => {
         expect(observer2.getCurrentResult().isPaused).toBeTruthy()
       })
 
-      onlineManager.setOnline(undefined)
+      onlineManager.setOnline(true)
       void queryClient.resumePausedMutations()
       await sleep(5)
       await queryClient.resumePausedMutations()
@@ -1491,6 +1490,7 @@ describe('queryClient', () => {
         'resumePausedMutations',
       )
 
+      onlineManager.setOnline(false)
       onlineManager.setOnline(true)
       expect(queryCacheOnOnlineSpy).toHaveBeenCalledTimes(1)
       expect(mutationCacheResumePausedMutationsSpy).toHaveBeenCalledTimes(1)
@@ -1503,7 +1503,7 @@ describe('queryClient', () => {
       queryCacheOnOnlineSpy.mockRestore()
       mutationCacheResumePausedMutationsSpy.mockRestore()
       focusManager.setFocused(undefined)
-      onlineManager.setOnline(undefined)
+      onlineManager.setOnline(true)
     })
 
     test('should not notify queryCache and mutationCache after multiple mounts/unmounts', async () => {
@@ -1538,7 +1538,7 @@ describe('queryClient', () => {
       queryCacheOnOnlineSpy.mockRestore()
       mutationCacheResumePausedMutationsSpy.mockRestore()
       focusManager.setFocused(undefined)
-      onlineManager.setOnline(undefined)
+      onlineManager.setOnline(true)
     })
   })
 

packages/query-core/src/tests/utils.ts

@@ -1,6 +1,6 @@
 import { act } from '@testing-library/react'
 import { vi } from 'vitest'
-import { QueryClient } from '..'
+import { QueryClient, onlineManager } from '..'
 import * as utils from '../utils'
 import type { SpyInstance } from 'vitest'
 import type { MutationOptions, QueryClientConfig } from '..'
@@ -15,8 +15,10 @@ export function mockVisibilityState(
   return vi.spyOn(document, 'visibilityState', 'get').mockReturnValue(value)
 }
 
-export function mockNavigatorOnLine(value: boolean): SpyInstance<[], boolean> {
-  return vi.spyOn(navigator, 'onLine', 'get').mockReturnValue(value)
+export function mockOnlineManagerIsOnline(
+  value: boolean,
+): SpyInstance<[], boolean> {
+  return vi.spyOn(onlineManager, 'isOnline').mockReturnValue(value)
 }
 
 let queryKeyCount = 0

packages/query-devtools/src/Devtools.tsx

@@ -485,13 +485,11 @@ export const DevtoolsPanel: Component<DevtoolsPanelProps> = (props) => {
             <button
               onClick={() => {
                 if (offline()) {
-                  onlineManager().setOnline(undefined)
+                  onlineManager().setOnline(true)
                   setOffline(false)
-                  window.dispatchEvent(new Event('online'))
                 } else {
                   onlineManager().setOnline(false)
                   setOffline(true)
-                  window.dispatchEvent(new Event('offline'))
                 }
               }}
               class={styles.actionsBtn}