Repository: zeit/swr
Pull request #323 (2020-03-30 23:34:30)

Fix a race condition bug and remove keyRef check
Inside `revalidate`, `keyRef` will be overridden by the request key when one request finishes. Consider 2 requests initialized by 2 different keys `key1` and `key2` (key has changed after the 1st request fired), if the 1st took longer, `keyRef` will be changed to `key1` eventually.

In a new read access to the getter, `undefined` will be returned since the keys no longer match.

### Reproduction

Run the first `should respect requests after key has changed` test without this fix.

### Fix

`keyRef` check is not necessary at all, let's just remove it. Inside `useEffect` https://github.com/zeit/swr/blob/master/src/use-swr.ts#L376-L382, every time `key` updates, we will get the latest data from the cache and dispatch the state synchronously. 

```js
    const currentHookData = stateRef.current.data
    const latestKeyedData = cache.get(key) || config.initialData

    // update the state if the key changed or cache updated
    if (
      keyRef.current !== key ||
      !config.compare(currentHookData, latestKeyedData)
    ) {
      dispatch({ data: latestKeyedData })
      keyRef.current = key
    }
```

And any new read access after that will get the latest, correct, and keyed data. Check the second new test for more details.



Patch:
diff --git a/src/use-swr.ts b/src/use-swr.ts
index 701b822..fb09e93 100644
--- a/src/use-swr.ts
+++ b/src/use-swr.ts
@@ -289,7 +289,6 @@ function useSWR<Data = any, Error = any>(
 
         cache.set(key, newData, false)
         cache.set(keyErr, undefined, false)
-        keyRef.current = key
 
         // new state for the reducer
         const newState: actionType<Data, Error> = {
@@ -320,7 +319,6 @@ function useSWR<Data = any, Error = any>(
         delete CONCURRENT_PROMISES_TS[key]
 
         cache.set(keyErr, err, false)
-        keyRef.current = key
 
         // get a new error
         // don't use deep equal for errors
@@ -372,7 +370,7 @@ function useSWR<Data = any, Error = any>(
     const currentHookData = stateRef.current.data
     const latestKeyedData = cache.get(key) || config.initialData
 
-    // update the state if the key changed or cache updated
+    // update the state if the key changed (not the inital render) or cache updated
     if (
       keyRef.current !== key ||
       !config.compare(currentHookData, latestKeyedData)
@@ -442,7 +440,6 @@ function useSWR<Data = any, Error = any>(
         dispatch(newState)
       }
 
-      keyRef.current = key
       if (shouldRevalidate) {
         if (dedupe) {
           return softRevalidate()
@@ -594,13 +591,13 @@ function useSWR<Data = any, Error = any>(
         // so we need to match the latest key and data (fallback to `initialData`)
         get: function() {
           stateDependencies.current.error = true
-          return keyRef.current === key ? stateRef.current.error : initialError
+          return stateRef.current.error
         }
       },
       data: {
         get: function() {
           stateDependencies.current.data = true
-          return keyRef.current === key ? stateRef.current.data : initialData
+          return stateRef.current.data
         }
       },
       isValidating: {


Test patch:
diff --git a/test/use-swr.test.tsx b/test/use-swr.test.tsx
index a6d72d5..dee86cb 100644
--- a/test/use-swr.test.tsx
+++ b/test/use-swr.test.tsx
@@ -1323,3 +1323,76 @@ describe('useSWR - cache', () => {
     expect(listener).toHaveBeenCalledTimes(1)
   })
 })
+
+describe('useSWR - key', () => {
+  afterEach(cleanup)
+
+  it('should respect requests after key has changed', async () => {
+    let rerender
+
+    function Page() {
+      const [mounted, setMounted] = useState(0)
+      const key = `key-1-${mounted ? 'short' : 'long'}`
+      const { data } = useSWR(key, async () => {
+        if (mounted) {
+          await new Promise(res => setTimeout(res, 100))
+          return 'short request'
+        }
+        await new Promise(res => setTimeout(res, 200))
+        return 'long request'
+      })
+      useEffect(() => setMounted(1), [])
+      rerender = setMounted
+
+      return <div>{data}</div>
+    }
+
+    const { container } = render(<Page />)
+    expect(container.firstChild.textContent).toMatchInlineSnapshot(`""`)
+    await waitForDomChange({ container })
+    expect(container.firstChild.textContent).toMatchInlineSnapshot(
+      `"short request"`
+    )
+    await act(() => new Promise(res => setTimeout(res, 110))) // wait 100ms until "long request" finishes
+    expect(container.firstChild.textContent).toMatchInlineSnapshot(
+      `"short request"`
+    ) // should be "short request" still
+
+    // manually trigger a re-render from outside
+    // this triggers a re-render, and a read access to `swr.data`
+    // but the result should still be "short request"
+    await act(() => rerender(x => x + 1))
+    expect(container.firstChild.textContent).toMatchInlineSnapshot(
+      `"short request"`
+    )
+  })
+
+  it('should render undefined after key has changed', async () => {
+    function Page() {
+      const [mounted, setMounted] = useState(false)
+      const key = `key-${mounted ? '1' : '0'}`
+      const { data } = useSWR(key, async k => {
+        await new Promise(res => setTimeout(res, 200))
+        return k
+      })
+      useEffect(() => {
+        setTimeout(() => setMounted(true), 320)
+      }, [])
+      return <div>{data}</div>
+    }
+
+    //    time     data       key
+    // -> 0        undefined, '0'
+    // -> 200      0,         '0'
+    // -> 320      undefined, '1' <- this state is required; we can't show 0 here
+    // -> 520      1,         '1'
+    const { container } = render(<Page />)
+    expect(container.firstChild.textContent).toMatchInlineSnapshot(`""`) // undefined, time=0
+    await act(() => new Promise(res => setTimeout(res, 210)))
+    expect(container.firstChild.textContent).toMatchInlineSnapshot(`"key-0"`) // 0, time=210
+    await act(() => new Promise(res => setTimeout(res, 200)))
+    expect(container.firstChild.textContent).toMatchInlineSnapshot(`""`) // undefined, time=410
+    await act(() => new Promise(res => setTimeout(res, 140)))
+    expect(container.firstChild.textContent).toMatchInlineSnapshot(`"key-1"`) // 1, time=550
+  })
+})