Repository: zeit/swr (ts)

Issue:
Bug: Suspense Data undefined Race Condition
I'm not sure if this is a bug, or if this behavior is intended, but I was running into an issue with the line of code below. 

So, I'm using `useSWR(`/api/users/${id}`, fetch, {suspense: true})` where the key is dynamic based on the id of the object I want to get (in this contrived example the user's id). 

My code looks something like this exampled:

```
function useUser(id) {
  const user = useSWR(`/api/users/${id}`, fetch, {suspense: true})
  console.log(user)
  return user
}
```

When this hook is called multiple times with different IDs, the logs would look like this:

```
// render 1 with user id 1
{ id:1 }
// render 2 with user id 2
undefined
// render 3
{ id: 2 }
```

The reason behind this is the two LOC referenced below.

So, my question is, in render 2 on L342 `typeof data = Object`, not undefined because the data is reset on line 358 (afterwards). Should there be a line before line 342 that checks to see if the `keyRef.current` and the new `key` are then different and reset data there so that the revalidate can happen immediately? 

I quickly implemented this locally and this fixed my issue and prevented render 2 happening where the data was undefined.

Thanks! Loving this library so far.

https://github.com/zeit/swr/blob/9b51b7d066335d7249d114e2b9a2cb5d633795b6/src/use-swr.ts#L342
https://github.com/zeit/swr/blob/9b51b7d066335d7249d114e2b9a2cb5d633795b6/src/use-swr.ts#L358
BUG: Infinite loop with suspense on rejected promise
See: https://codesandbox.io/s/swr-playground-zloyr

Steps to reproduce:
- Use SWR
- Set suspense to true
- Set shouldRetryOnError to false
- Reject HTTP request (or just promise)

Actual result:
Endless loop of retrying

Expected result:
Error boundary should catch error'ed promise

Pull request description:
Fix suspense bugs
Bug fixes
- fixed #57 and added a test case for it
- fixed #66 and added a test case for it

Also
- introduced cache for errors to support suspense
- changed `noDedupe` to `dedupe` (undocumented API), which is more clear and removes 1 hook inside  (`useCallback`)
- improved code readability 


Patch:
diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS
index 1abc0c5..168236b 100644
--- a/.github/CODEOWNERS
+++ b/.github/CODEOWNERS
@@ -1,1 +1,1 @@
-*    @quietshu @Timer
+*    @quietshu @Timer @pacocoursey
diff --git a/src/types.ts b/src/types.ts
index d9d7521..7d4167c 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -33,7 +33,7 @@ export interface ConfigInterface<Data = any, Error = any> {
 
 export interface RevalidateOptionInterface {
   retryCount?: number
-  noDedupe?: boolean
+  dedupe?: boolean
 }
 
 type keyFunction = () => string
diff --git a/src/use-swr.ts b/src/use-swr.ts
index 36f3238..2dbb81a 100644
--- a/src/use-swr.ts
+++ b/src/use-swr.ts
@@ -33,19 +33,25 @@ import SWRConfigContext from './swr-config-context'
 import isDocumentVisible from './libs/is-document-visible'
 import useHydration from './libs/use-hydration'
 
+const IS_SERVER = typeof window === 'undefined'
+
+// TODO: introduce namepsace for the cache
+const getErrorKey = key => (key ? 'err@' + key : '')
+
 const trigger: triggerInterface = (key, shouldRevalidate = true) => {
   const updaters = CACHE_REVALIDATORS[key]
-  if (updaters) {
+  if (key && updaters) {
     const currentData = cacheGet(key)
+    const currentError = cacheGet(getErrorKey(key))
     for (let i = 0; i < updaters.length; ++i) {
-      updaters[i](shouldRevalidate, currentData)
+      updaters[i](shouldRevalidate, currentData, currentError)
     }
   }
 }
 
 const broadcastState: broadcastStateInterface = (key, data, error) => {
   const updaters = CACHE_REVALIDATORS[key]
-  if (updaters) {
+  if (key && updaters) {
     for (let i = 0; i < updaters.length; ++i) {
       updaters[i](false, data, error)
     }
@@ -53,6 +59,8 @@ const broadcastState: broadcastStateInterface = (key, data, error) => {
 }
 
 const mutate: mutateInterface = (key, data, shouldRevalidate = true) => {
+  if (!key) return
+
   // update timestamp
   MUTATION_TS[key] = Date.now() - 1
 
@@ -111,9 +119,13 @@ function useSWR<Data = any, Error = any>(
       key = ''
     }
   } else {
-    key = _key
+    // convert null to ''
+    key = String(_key || '')
   }
 
+  // `keyErr` is the cache key for error objects
+  const keyErr = getErrorKey(key)
+
   config = Object.assign(
     {},
     defaultConfig,
@@ -126,114 +138,144 @@ function useSWR<Data = any, Error = any>(
     fn = config.fetcher
   }
 
-  // stale: get from cache
-  let [data, setData] = useState(
-    config.suspense ? cacheGet(key) : useHydration() ? undefined : cacheGet(key)
-  )
   // it is fine to call `useHydration` conditionally here
   // because `config.suspense` should never change
-  let [error, setError] = useState()
+  const shouldReadCache = config.suspense || !useHydration()
+
+  // stale: get from cache
+  let [data, setData] = useState(shouldReadCache ? cacheGet(key) : undefined)
+  let [error, setError] = useState(
+    shouldReadCache ? cacheGet(keyErr) : undefined
+  )
+
+  // if there's any ongoing request
   let [isValidating, setIsValidating] = useState(false)
 
   // error ref inside revalidate (is last request errored?)
-  const errorRef = useRef(null)
   const unmountedRef = useRef(false)
   const keyRef = useRef(key)
+  const errorRef = useRef(error)
   const dataRef = useRef(data)
 
+  // start a revalidation
   const revalidate = useCallback(
     async (
       revalidateOpts: RevalidateOptionInterface = {}
     ): Promise<boolean> => {
       if (!key) return false
       if (unmountedRef.current) return false
+      revalidateOpts = Object.assign({ dedupe: false }, revalidateOpts)
 
       let loading = true
-      let isOriginalRequest = !!(
-        typeof CONCURRENT_PROMISES[key] === 'undefined' ||
-        revalidateOpts.noDedupe
-      )
+      let shouldDeduping =
+        typeof CONCURRENT_PROMISES[key] !== 'undefined' && revalidateOpts.dedupe
 
+      // start fetching
       try {
         setIsValidating(true)
 
         let newData
-        let ts
+        let startAt
 
-        if (!isOriginalRequest) {
-          // different component, dedupe requests
-          // need the new data for the state
-          ts = CONCURRENT_PROMISES_TS[key]
+        if (shouldDeduping) {
+          // there's already an ongoing request,
+          // this one needs to be deduplicated.
+          startAt = CONCURRENT_PROMISES_TS[key]
           newData = await CONCURRENT_PROMISES[key]
         } else {
-          // if no cache being rendered (blank page),
-          // we trigger the loading slow event
-          if (!cacheGet(key)) {
+          // if no cache being rendered currently (it shows a blank page),
+          // we trigger the loading slow event.
+          if (config.loadingTimeout && !cacheGet(key)) {
             setTimeout(() => {
               if (loading) config.onLoadingSlow(key, config)
             }, config.loadingTimeout)
           }
+
           CONCURRENT_PROMISES[key] = fn(key)
-          CONCURRENT_PROMISES_TS[key] = ts = Date.now()
+          CONCURRENT_PROMISES_TS[key] = startAt = Date.now()
+
           setTimeout(() => {
             delete CONCURRENT_PROMISES[key]
             delete CONCURRENT_PROMISES_TS[key]
           }, config.dedupingInterval)
+
           newData = await CONCURRENT_PROMISES[key]
 
-          // trigger the success event
-          // (only do it for the original request)
+          // trigger the success event,
+          // only do this for the original request.
           config.onSuccess(newData, key, config)
         }
 
-        // if the revalidation happened earlier than local mutations,
-        // we should ignore the result because it could override.
-        if (MUTATION_TS[key] && ts <= MUTATION_TS[key]) {
+        // if the revalidation happened earlier than the local mutation,
+        // we have to ignore the result because it could override.
+        // meanwhile, a new revalidation should be triggered by the mutation.
+        if (MUTATION_TS[key] && startAt <= MUTATION_TS[key]) {
           setIsValidating(false)
           return false
         }
 
-        errorRef.current = null
+        cacheSet(key, newData)
+        cacheSet(keyErr, undefined)
+        keyRef.current = key
 
+        // batch state updates
         unstable_batchedUpdates(() => {
           setIsValidating(false)
-          setError(undefined)
+
+          if (typeof errorRef.current !== 'undefined') {
+            // we don't have an error
+            setError(undefined)
+            errorRef.current = undefined
+          }
           if (deepEqual(dataRef.current, newData)) {
             // deep compare to avoid extra re-render
             // do nothing
           } else {
             // data changed
             setData(newData)
-            cacheSet(key, newData)
-            if (isOriginalRequest) {
-              // also update other SWRs from cache
-              broadcastState(key, newData, null)
-            }
-            keyRef.current = key
             dataRef.current = newData
           }
+
+          if (!shouldDeduping) {
+            // also update other hooks
+            broadcastState(key, newData, undefined)
+          }
         })
       } catch (err) {
         delete CONCURRENT_PROMISES[key]
-        unstable_batchedUpdates(() => {
-          setIsValidating(false)
-          setError(err)
-        })
-        if (isOriginalRequest) {
-          broadcastState(key, undefined, err)
+        delete CONCURRENT_PROMISES_TS[key]
+
+        cacheSet(keyErr, err)
+        keyRef.current = key
+
+        // get a new error
+        // don't use deep equal for errors
+        if (errorRef.current !== err) {
+          errorRef.current = err
+
+          unstable_batchedUpdates(() => {
+            // we keep the stale data
+            setIsValidating(false)
+            setError(err)
+          })
+
+          if (!shouldDeduping) {
+            // also broadcast to update other hooks
+            broadcastState(key, undefined, err)
+          }
         }
 
+        // events and retry
         config.onError(err, key, config)
-        errorRef.current = err
-
         if (config.shouldRetryOnError) {
+          // when retrying, we always enable deduping
           const retryCount = (revalidateOpts.retryCount || 0) + 1
           config.onErrorRetry(
             err,
             key,
             config,
             revalidate,
-            Object.assign({}, revalidateOpts, { retryCount })
+            Object.assign({ dedupe: true }, revalidateOpts, { retryCount })
           )
         }
       }
@@ -243,9 +285,6 @@ function useSWR<Data = any, Error = any>(
     },
     [key]
   )
-  const forceRevalidate = useCallback(() => revalidate({ noDedupe: true }), [
-    revalidate
-  ])
 
   // mounted
   useLayoutEffect(() => {
@@ -254,30 +293,44 @@ function useSWR<Data = any, Error = any>(
     // after `key` updates, we need to mark it as mounted
     unmountedRef.current = false
 
-    const _newData = cacheGet(key)
+    // after the component is mounted (hydrated),
+    // we need to update the data from the cache
+    // and trigger a revalidation
 
-    // update the state if the cache changed OR the key changed
-    if ((_newData && !deepEqual(data, _newData)) || keyRef.current !== key) {
-      setData(_newData)
-      dataRef.current = _newData
+    const currentHookData = dataRef.current
+    const latestKeyedData = cacheGet(key)
+
+    // update the state if the key changed or cache updated
+    if (
+      keyRef.current !== key ||
+      !deepEqual(currentHookData, latestKeyedData)
+    ) {
+      setData(latestKeyedData)
+      dataRef.current = latestKeyedData
       keyRef.current = key
     }
 
-    // revalidate after mounted
-    if (_newData && window['requestIdleCallback']) {
+    // revalidate with deduping
+    const softRevalidate = () => revalidate({ dedupe: true })
+
+    // trigger a revalidation
+    if (
+      typeof latestKeyedData !== 'undefined' &&
+      window['requestIdleCallback']
+    ) {
       // delay revalidate if there's cache
       // to not block the rendering
-      window['requestIdleCallback'](revalidate)
+      window['requestIdleCallback'](softRevalidate)
     } else {
-      revalidate()
+      softRevalidate()
     }
 
     // whenever the window gets focused, revalidate
-    // throttle: avoid being called twice from both listeners
-    // and tabs being switched quickly
-    const onFocus = throttle(revalidate, config.focusThrottleInterval)
-
+    let onFocus
     if (config.revalidateOnFocus) {
+      // throttle: avoid being called twice from both listeners
+      // and tabs being switched quickly
+      onFocus = throttle(softRevalidate, config.focusThrottleInterval)
       if (!FOCUS_REVALIDATORS[key]) {
         FOCUS_REVALIDATORS[key] = [onFocus]
       } else {
@@ -285,13 +338,13 @@ function useSWR<Data = any, Error = any>(
       }
     }
 
-    // updater
+    // register global cache update listener
     const onUpdate: updaterInterface<Data, Error> = (
       shouldRevalidate = true,
       updatedData,
       updatedError
     ) => {
-      // update data from the cache
+      // update hook state
       unstable_batchedUpdates(() => {
         if (
           typeof updatedData !== 'undefined' &&
@@ -300,10 +353,9 @@ function useSWR<Data = any, Error = any>(
           setData(updatedData)
           dataRef.current = updatedData
         }
-        if (
-          typeof updatedError !== 'undefined' &&
-          !deepEqual(errorRef.current, updatedError)
-        ) {
+        // always update error
+        // because it can be `undefined`
+        if (errorRef.current !== updatedError) {
           setError(updatedError)
           errorRef.current = updatedError
         }
@@ -311,7 +363,7 @@ function useSWR<Data = any, Error = any>(
       })
 
       if (shouldRevalidate) {
-        return revalidate()
+        return softRevalidate()
       }
       return false
     }
@@ -323,36 +375,36 @@ function useSWR<Data = any, Error = any>(
       CACHE_REVALIDATORS[key].push(onUpdate)
     }
 
-    // polling
-    let id = null
-    async function tick() {
-      if (
-        !errorRef.current &&
-        (config.refreshWhenHidden || isDocumentVisible())
-      ) {
-        // only revalidate when the page is visible
-        // if API request errored, we stop polling in this round
-        // and let the error retry function handle it
-        await revalidate()
-      }
-
-      const interval = config.refreshInterval
-      id = setTimeout(tick, interval)
-    }
+    // set up polling
+    let timeout = null
     if (config.refreshInterval) {
-      id = setTimeout(tick, config.refreshInterval)
+      const tick = async () => {
+        if (
+          !errorRef.current &&
+          (config.refreshWhenHidden || isDocumentVisible())
+        ) {
+          // only revalidate when the page is visible
+          // if API request errored, we stop polling in this round
+          // and let the error retry function handle it
+          await softRevalidate()
+        }
+
+        const interval = config.refreshInterval
+        timeout = setTimeout(tick, interval)
+      }
+      timeout = setTimeout(tick, config.refreshInterval)
     }
 
     return () => {
       // cleanup
       setData = () => null
-      setIsValidating = () => null
       setError = () => null
+      setIsValidating = () => null
 
       // mark it as unmounted
       unmountedRef.current = true
 
-      if (FOCUS_REVALIDATORS[key]) {
+      if (onFocus && FOCUS_REVALIDATORS[key]) {
         const index = FOCUS_REVALIDATORS[key].indexOf(onFocus)
         if (index >= 0) FOCUS_REVALIDATORS[key].splice(index, 1)
       }
@@ -361,21 +413,59 @@ function useSWR<Data = any, Error = any>(
         if (index >= 0) CACHE_REVALIDATORS[key].splice(index, 1)
       }
 
-      if (id !== null) {
-        clearTimeout(id)
+      if (timeout !== null) {
+        clearTimeout(timeout)
       }
     }
   }, [key, config.refreshInterval, revalidate])
 
-  // suspense (client side only)
-  if (config.suspense && typeof data === 'undefined') {
-    if (typeof window !== 'undefined') {
+  // suspense
+  if (config.suspense) {
+    if (IS_SERVER)
+      throw new Error('Suspense on server side is not yet supported!')
+
+    // in suspense mode, we can't return empty state
+    // (it should be suspended)
+
+    // try to get data and error from cache
+    let latestData = cacheGet(key)
+    let latestError = cacheGet(keyErr)
+
+    if (
+      typeof latestData === 'undefined' &&
+      typeof latestError === 'undefined'
+    ) {
+      // need to start the request if it hasn't
       if (!CONCURRENT_PROMISES[key]) {
-        // need to trigger revalidate immediately
-        // to throw the promise
+        // trigger revalidate immediately
+        // to get the promise
         revalidate()
       }
-      throw CONCURRENT_PROMISES[key]
+
+      if (
+        CONCURRENT_PROMISES[key] &&
+        typeof CONCURRENT_PROMISES[key].then === 'function'
+      ) {
+        // if it is a promise
+        throw CONCURRENT_PROMISES[key]
+      }
+
+      // it's a value, return it directly (override)
+      latestData = CONCURRENT_PROMISES[key]
+    }
+
+    if (typeof latestData === 'undefined' && latestError) {
+      // in suspense mode, throw error if there's no content
+      throw latestError
+    }
+
+    // return the latest data / error from cache
+    // in case `key` has changed
+    return {
+      error: latestError,
+      data: latestData,
+      revalidate,
+      isValidating
     }
   }
 
@@ -385,7 +475,7 @@ function useSWR<Data = any, Error = any>(
     // but the previous state will stay
     // so we need to match the latest key and data
     data: keyRef.current === key ? data : undefined,
-    revalidate: forceRevalidate, // handler
+    revalidate, // handler
     isValidating
   }
 }


Test patch:
diff --git a/test/use-swr.test.tsx b/test/use-swr.test.tsx
index 83bec3e..1406928 100644
--- a/test/use-swr.test.tsx
+++ b/test/use-swr.test.tsx
@@ -1,4 +1,4 @@
-import React, { Suspense } from 'react'
+import React, { Suspense, ReactNode, useState, useEffect } from 'react'
 import {
   cleanup,
   render,
@@ -9,6 +9,21 @@ import {
 
 import useSWR, { trigger, mutate, SWRConfig } from '../src'
 
+class ErrorBoundary extends React.Component<{ fallback: ReactNode }> {
+  state = { hasError: false }
+  static getDerivedStateFromError() {
+    return {
+      hasError: true
+    }
+  }
+  render() {
+    if (this.state.hasError) {
+      return this.props.fallback
+    }
+    return this.props.children
+  }
+}
+
 describe('useSWR', () => {
   afterEach(cleanup)
 
@@ -606,4 +621,120 @@ describe('useSWR - suspense', () => {
     await act(() => new Promise(res => setTimeout(res, 100))) // should recover
     expect(container.textContent).toMatchInlineSnapshot(`"3"`)
   })
+
+  it('should work for non-promises', async () => {
+    function Section() {
+      const { data } = useSWR('suspense-4', () => 'hello', {
+        suspense: true
+      })
+      return <div>{data}</div>
+    }
+    const { container } = render(
+      <Suspense fallback={<div>fallback</div>}>
+        <Section />
+      </Suspense>
+    )
+
+    // hydration
+    expect(container.textContent).toMatchInlineSnapshot(`"hello"`)
+  })
+
+  it('should throw errors', async () => {
+    function Section() {
+      const { data } = useSWR(
+        'suspense-5',
+        () =>
+          new Promise((_, reject) => setTimeout(() => reject('error'), 100)),
+        {
+          suspense: true
+        }
+      )
+      return <div>{data}</div>
+    }
+    // https://reactjs.org/docs/concurrent-mode-suspense.html#handling-errors
+    const { container } = render(
+      <ErrorBoundary fallback={<div>error boundary</div>}>
+        <Suspense fallback={<div>fallback</div>}>
+          <Section />
+        </Suspense>
+      </ErrorBoundary>
+    )
+
+    // hydration
+    expect(container.textContent).toMatchInlineSnapshot(`"fallback"`)
+    await act(() => new Promise(res => setTimeout(res, 150))) // still suspending
+    expect(container.textContent).toMatchInlineSnapshot(`"error boundary"`)
+
+    console.info('*The warning above can be ignored (caught by ErrorBoundary).')
+  })
+
+  it('should render cached data with error', async () => {
+    mutate('suspense-6', 'hello')
+
+    function Section() {
+      const { data, error } = useSWR(
+        // this value is cached
+        'suspense-6',
+        () =>
+          new Promise((_, reject) => setTimeout(() => reject('error'), 100)),
+        {
+          suspense: true
+        }
+      )
+      return (
+        <div>
+          {data}, {error}
+        </div>
+      )
+    }
+
+    const { container } = render(
+      <Suspense fallback={<div>fallback</div>}>
+        <Section />
+      </Suspense>
+    )
+
+    expect(container.textContent).toMatchInlineSnapshot(`"hello, "`) // directly from cache
+    await act(() => new Promise(res => setTimeout(res, 150))) // still suspending
+    expect(container.textContent).toMatchInlineSnapshot(`"hello, error"`) // get error with cache
+  })
+
+  it('should pause when key changes', async () => {
+    const renderedResults = []
+
+    function Section() {
+      const [key, setKey] = useState('suspense-7')
+      const { data } = useSWR(
+        key,
+        k => new Promise(res => setTimeout(() => res(k), 50)),
+        {
+          suspense: true
+        }
+      )
+
+      useEffect(() => {
+        if (data === 'suspense-7') {
+          setKey('suspense-8')
+        }
+      }, [data])
+
+      if (data !== renderedResults[renderedResults.length - 1]) {
+        renderedResults.push(data)
+      }
+
+      return <>{data}</>
+    }
+
+    render(
+      <Suspense fallback={<div>fallback</div>}>
+        <Section />
+      </Suspense>
+    )
+
+    await act(() => new Promise(res => setTimeout(res, 110)))
+
+    // fixes https://github.com/zeit/swr/issues/57
+    // 'suspense-7' -> undefined -> 'suspense-8'
+    expect(renderedResults).toEqual(['suspense-7', 'suspense-8'])
+  })
 })