fix: The onLoadingSlow and onSuccess callback miss unmount check

do unmount check for all callbacks Closes: #286
vercel · Jun 7, 2020 · 730315a · 730315a
1 parent 6e1b9b1
commit 730315a
Show file tree

Hide file tree

Showing 3 changed files with 104 additions and 34 deletions.
diff --git a/package.json b/package.json
@@ -54,7 +54,7 @@
     "react": "16.11.0",
     "react-dom": "16.11.0",
     "ts-jest": "24.1.0",
-    "typescript": "3.6.4"
+    "typescript": "3.6.3"
   },
   "dependencies": {
     "fast-deep-equal": "2.0.1"

diff --git a/src/use-swr.ts b/src/use-swr.ts
@@ -215,6 +215,35 @@ function useSWR<Data = any, Error = any>(
   const unmountedRef = useRef(false)
   const keyRef = useRef(key)
 
+  // do unmount check for callbacks
+  const callbacksRef = useRef<
+    Pick<
+      ConfigInterface<Data, Error>,
+      'onLoadingSlow' | 'onSuccess' | 'onError' | 'onErrorRetry'
+    >
+  >({
+    onLoadingSlow: (...param) => {
+      if (config.onLoadingSlow && !unmountedRef.current) {
+        config.onLoadingSlow(...param)
+      }
+    },
+    onSuccess: (...param) => {
+      if (config.onSuccess && !unmountedRef.current) {
+        config.onSuccess(...param)
+      }
+    },
+    onError: (...param) => {
+      if (config.onError && !unmountedRef.current) {
+        config.onError(...param)
+      }
+    },
+    onErrorRetry: (...param) => {
+      if (config.onErrorRetry && !unmountedRef.current) {
+        config.onErrorRetry(...param)
+      }
+    }
+  })
+
   const boundMutate: responseInterface<Data, Error>['mutate'] = useCallback(
     (data, shouldRevalidate) => {
       return mutate(key, data, shouldRevalidate)
@@ -268,7 +297,7 @@ function useSWR<Data = any, Error = any>(
           // we trigger the loading slow event.
           if (config.loadingTimeout && !cache.get(key)) {
             setTimeout(() => {
-              if (loading) config.onLoadingSlow(key, config)
+              if (loading) callbacksRef.current.onLoadingSlow(key, config)
             }, config.loadingTimeout)
           }
 
@@ -289,7 +318,7 @@ function useSWR<Data = any, Error = any>(
 
           // trigger the success event,
           // only do this for the original request.
-          config.onSuccess(newData, key, config)
+          callbacksRef.current.onSuccess(newData, key, config)
         }
 
         // if the revalidation happened earlier than the local mutation,
@@ -347,11 +376,11 @@ function useSWR<Data = any, Error = any>(
         }
 
         // events and retry
-        config.onError(err, key, config)
+        callbacksRef.current.onError(err, key, config)
         if (config.shouldRetryOnError) {
           // when retrying, we always enable deduping
           const retryCount = (revalidateOpts.retryCount || 0) + 1
-          config.onErrorRetry(
+          callbacksRef.current.onErrorRetry(
             err,
             key,
             config,

diff --git a/test/use-swr.test.tsx b/test/use-swr.test.tsx
@@ -740,36 +740,77 @@ describe('useSWR - error', () => {
       `"hello, SWR"`
     )
   })
-})
+  it('should trigger limited error retries if errorRetryCount exists', async () => {
+    let count = 0
+    function Page() {
+      const { data, error } = useSWR(
+        'error-5',
+        () => {
+          return new Promise((_, rej) =>
+            setTimeout(() => rej(new Error('error: ' + count++)), 100)
+          )
+        },
+        {
+          errorRetryCount: 1,
+          errorRetryInterval: 50,
+          dedupingInterval: 0
+        }
+      )
+      if (error) return <div>{error.message}</div>
+      return <div>hello, {data}</div>
+    }
+    const { container } = render(<Page />)
 
-it('should trigger limited error retries if errorRetryCount exists', async () => {
-  let count = 0
-  function Page() {
-    const { data, error } = useSWR(
-      'error-5',
-      () => {
-        return new Promise((_, rej) =>
-          setTimeout(() => rej(new Error('error: ' + count++)), 100)
-        )
-      },
-      {
-        errorRetryCount: 1,
-        errorRetryInterval: 50,
-        dedupingInterval: 0
-      }
-    )
-    if (error) return <div>{error.message}</div>
-    return <div>hello, {data}</div>
-  }
-  const { container } = render(<Page />)
-
-  expect(container.firstChild.textContent).toMatchInlineSnapshot(`"hello, "`)
-  await waitForDomChange({ container })
-  expect(container.firstChild.textContent).toMatchInlineSnapshot(`"error: 0"`)
-  await act(() => new Promise(res => setTimeout(res, 210))) // retry
-  expect(container.firstChild.textContent).toMatchInlineSnapshot(`"error: 1"`)
-  await act(() => new Promise(res => setTimeout(res, 210))) // retry
-  expect(container.firstChild.textContent).toMatchInlineSnapshot(`"error: 1"`)
+    expect(container.firstChild.textContent).toMatchInlineSnapshot(`"hello, "`)
+    await waitForDomChange({ container })
+    expect(container.firstChild.textContent).toMatchInlineSnapshot(`"error: 0"`)
+    await act(() => new Promise(res => setTimeout(res, 210))) // retry
+    expect(container.firstChild.textContent).toMatchInlineSnapshot(`"error: 1"`)
+    await act(() => new Promise(res => setTimeout(res, 210))) // retry
+    expect(container.firstChild.textContent).toMatchInlineSnapshot(`"error: 1"`)
+  })
+
+  it('should not trigger the onLoadingSlow and onSuccess event after component unmount', async () => {
+    let loadingSlow = null,
+      success = null
+    function Page() {
+      const { data } = useSWR(
+        'error-6',
+        () => new Promise(res => setTimeout(() => res('SWR'), 200)),
+        {
+          onLoadingSlow: key => {
+            loadingSlow = key
+          },
+          onSuccess: (_, key) => {
+            success = key
+          },
+          loadingTimeout: 100
+        }
+      )
+      return <div>{data}</div>
+    }
+
+    function App() {
+      const [on, toggle] = useState(true)
+      return (
+        <div id="app" onClick={() => toggle(s => !s)}>
+          {on && <Page />}
+        </div>
+      )
+    }
+
+    const { container } = render(<App />)
+
+    expect(loadingSlow).toEqual(null)
+    expect(success).toEqual(null)
+
+    await act(async () => new Promise(res => setTimeout(res, 10)))
+    await act(() => fireEvent.click(container.firstElementChild))
+    await act(async () => new Promise(res => setTimeout(res, 200)))
+
+    expect(success).toEqual(null)
+    expect(loadingSlow).toEqual(null)
+  })
 })
 
 describe('useSWR - focus', () => {