Unmount check (#433)

* fix: The `onLoadingSlow` and `onSuccess` callback miss unmount check
refactor handlers to event emitters
do unmount check for all events
Closes: #286

* add test for onError and onErrorRetry

src/use-swr.ts

@@ -215,6 +215,18 @@ function useSWR<Data = any, Error = any>(
   const unmountedRef = useRef(false)
   const keyRef = useRef(key)
 
+  // do unmount check for callbacks
+  const eventsRef = useRef({
+    emit: (
+      event,
+      ...params
+    ) => {
+      if (unmountedRef.current) return
+      config[event](...params)
+    }
+  })
+
+
   const boundMutate: responseInterface<Data, Error>['mutate'] = useCallback(
     (data, shouldRevalidate) => {
       return mutate(key, data, shouldRevalidate)
@@ -268,7 +280,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) eventsRef.current.emit('onLoadingSlow', key, config)
             }, config.loadingTimeout)
           }
 
@@ -289,7 +301,7 @@ function useSWR<Data = any, Error = any>(
 
           // trigger the success event,
           // only do this for the original request.
-          config.onSuccess(newData, key, config)
+          eventsRef.current.emit('onSuccess',newData, key, config)
         }
 
         // if the revalidation happened earlier than the local mutation,
@@ -347,11 +359,11 @@ function useSWR<Data = any, Error = any>(
         }
 
         // events and retry
-        config.onError(err, key, config)
+        eventsRef.current.emit('onError', err, key, config)
         if (config.shouldRetryOnError) {
           // when retrying, we always enable deduping
           const retryCount = (revalidateOpts.retryCount || 0) + 1
-          config.onErrorRetry(
+          eventsRef.current.emit('onErrorRetry',
             err,
             key,
             config,

test/use-swr.test.tsx

@@ -740,36 +740,122 @@ 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)
+  })
+
+  it('should not trigger the onError and onErrorRetry event after component unmount', async () => {
+    let retry = null,
+      failed = null
+    function Page() {
+      const { data } = useSWR(
+        'error-7',
+        () =>
+          new Promise((_, rej) =>
+            setTimeout(() => rej(new Error('error!')), 200)
+          ),
+        {
+          onError: (_, key) => {
+            failed = key
+          },
+          onErrorRetry: (_, key) => {
+            retry = key
+          },
+          dedupingInterval: 0
+        }
+      )
+      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(retry).toEqual(null)
+    expect(failed).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(retry).toEqual(null)
+    expect(failed).toEqual(null)
+  })
 })
 
 describe('useSWR - focus', () => {