fix(hook): fixes useStorage initial value bug (#368)

* fix(hook): updated types, added tests to check browser storage, added fix for setting storage on initial load * fix(hook): reverted ?? back to || in useStorage
antonioru · Jun 25, 2022 · 18e086f · 18e086f
1 parent 55f3095
commit 18e086f
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 79 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -896,3 +896,9 @@ Errored release
 ### Fixes
 
 - error handling for useStorage hook
+
+## [3.5.2] - 2022-06-24
+
+### Fixes
+
+- add more types for useStorage hook, fix bug where storage wasn't being set on initial render
diff --git a/src/factory/createStorageHook.ts b/src/factory/createStorageHook.ts
@@ -10,7 +10,7 @@ import noop from '../shared/noop'
  */
 const createStorageHook = (type: 'session' | 'local') => {
   type SetValue<TValue> = (value: TValue | ((previousValue: TValue) => TValue)) => void
-  const storageName = `${type}Storage`
+  const storageName: `${typeof type}Storage` = `${type}Storage`
 
   if (isClient && !isAPISupported(storageName)) {
     // eslint-disable-next-line no-console
@@ -31,25 +31,33 @@ const createStorageHook = (type: 'session' | 'local') => {
       }
       return [JSON.stringify(defaultValue) as unknown as TValue, noop]
     }
+    const storage = (window)[storageName]
+
+    const safelySetStorage = (valueToStore: string) => {
+      try {
+        storage.setItem(storageKey, valueToStore)
+        // eslint-disable-next-line no-empty
+      } catch (e) {}
+    }
 
-    const storage = (window as any)[storageName]
     const [storedValue, setStoredValue] = useState<TValue>(
       () => {
+        let valueToStore: string
         try {
-          return safelyParseJson(storage.getItem(storageKey) || JSON.stringify(defaultValue))
+          valueToStore = storage.getItem(storageKey) || JSON.stringify(defaultValue)
         } catch (e) {
-          return safelyParseJson(JSON.stringify(defaultValue))
+          valueToStore = JSON.stringify(defaultValue)
         }
+
+        safelySetStorage(valueToStore)
+        return safelyParseJson(valueToStore)
       },
     )
 
     const setValue: SetValue<TValue> = (value) => {
-      try {
-        const valueToStore = value instanceof Function ? value(storedValue) : value
-        storage.setItem(storageKey, JSON.stringify(valueToStore))
-        setStoredValue(valueToStore)
-      // eslint-disable-next-line no-empty
-      } catch (error) {}
+      const valueToStore = value instanceof Function ? value(storedValue) : value
+      safelySetStorage(JSON.stringify(valueToStore))
+      setStoredValue(valueToStore)
     }
 
     return [storedValue, setValue]

diff --git a/test/useLocalStorage.spec.js b/test/useLocalStorage.spec.js
@@ -1,6 +1,6 @@
-import React, { useEffect } from 'react'
-import { cleanup as cleanupReact, render } from '@testing-library/react'
+import { cleanup as cleanupReact } from '@testing-library/react'
 import { cleanup as cleanupHooks, renderHook } from '@testing-library/react-hooks'
+import { act } from 'react-dom/test-utils'
 import useLocalStorage from '../dist/useLocalStorage'
 import assertHook from './utils/assertHook'
 
@@ -17,44 +17,51 @@ describe('useLocalStorage', () => {
   assertHook(useLocalStorage)
 
   it('should return null when no default value defined', () => {
-    const { result, rerender } = renderHook(() => useLocalStorage('storageKey_1'))
+    const { result } = renderHook(() => useLocalStorage('storageKey_1'))
     const [value] = result.current
 
-    rerender()
-
     expect(value).to.equal(null)
   })
 
   it('should return default value', () => {
-    const { result, rerender } = renderHook(() => useLocalStorage('storageKey_2', 100))
+    const { result } = renderHook(() => useLocalStorage('storageKey_2', 100))
     const [value] = result.current
 
-    rerender()
-
     expect(value).to.equal(100)
+    expect(JSON.parse(window.localStorage.getItem('storageKey_2'))).to.equal(100)
   })
 
   it('should store and return new values', () => {
-    const TestComponent = (props) => {
-      // eslint-disable-next-line react/prop-types
-      const { newValue } = props
-      const [value, setValue] = useLocalStorage('storageKey_2', 100)
+    const { result } = renderHook(() =>
+      useLocalStorage("storageKey_3", 100)
+    )
 
-      const setNewState = (v) => {
-        setValue(v)
-      }
+    expect(result.current[0]).to.equal(100)
+    expect(JSON.parse(window.localStorage.getItem('storageKey_3'))).to.equal(100)
 
-      useEffect(() => {
-        setNewState(newValue)
-      }, [])
+    act(() => {
+      result.current[1](200)
+    })
 
-      return <p>{value}</p>
-    }
+    expect(result.current[0]).to.equal(200)
+    expect(JSON.parse(window.localStorage.getItem('storageKey_3'))).to.equal(200)
+  })
 
-    const { container } = render(<TestComponent newValue={200} />)
+  it('should accept a callback argument for setValue', () => {
+    const { result } = renderHook(() =>
+      useLocalStorage("storageKey_4", 100)
+    )
 
-    expect(container.querySelector('p').innerHTML).to.equal('200')
-  })
+    expect(result.current[0]).to.equal(100)
+    expect(JSON.parse(window.localStorage.getItem('storageKey_4'))).to.equal(100)
+
+    act(() => {
+      result.current[1](prev => prev + 100)
+    })
+
+    expect(result.current[0]).to.equal(200)
+    expect(JSON.parse(window.localStorage.getItem('storageKey_4'))).to.equal(200)
+  });
 
   it('should gracefully handle a getItem error and use the default value', () => {
     Object.defineProperty(window, "localStorage", {
@@ -66,17 +73,15 @@ describe('useLocalStorage', () => {
       },
     })
 
-    const { result, rerender } = renderHook(() =>
-      useLocalStorage("storageKey_3", 100)
+    const { result } = renderHook(() =>
+      useLocalStorage("storageKey_5", 100)
     )
     const [value] = result.current
 
-    rerender()
-
     expect(value).to.equal(100)
   })
 
-  it("should gracefully handle a setItem error and maintain the current value", () => {
+  it("should gracefully handle a setItem error and set the new value", () => {
     Object.defineProperty(window, "localStorage", {
       value: {
         ...window.localStorage,
@@ -86,14 +91,14 @@ describe('useLocalStorage', () => {
       },
     })
 
-    const { result, rerender } = renderHook(() =>
-      useLocalStorage("storageKey_4", 100)
+    const { result } = renderHook(() =>
+      useLocalStorage("storageKey_6", 100)
     )
-    const [value, setValue] = result.current
-    setValue(200)
 
-    rerender()
+    act(() => {
+      result.current[1](200)
+    })
 
-    expect(value).to.equal(100)
+    expect(result.current[0]).to.equal(200)
   })
 })
diff --git a/test/useSessionStorage.spec.js b/test/useSessionStorage.spec.js
@@ -1,6 +1,5 @@
-import React, { useEffect } from 'react'
-import { cleanup as cleanupReact, render } from '@testing-library/react'
-import { cleanup as cleanupHooks, renderHook } from '@testing-library/react-hooks'
+import { cleanup as cleanupReact } from '@testing-library/react'
+import { cleanup as cleanupHooks, renderHook, act } from '@testing-library/react-hooks'
 import useSessionStorage from '../dist/useSessionStorage'
 import assertHook from './utils/assertHook'
 
@@ -17,43 +16,50 @@ describe('useSessionStorage', () => {
   assertHook(useSessionStorage)
 
   it('should return null when no default value defined', () => {
-    const { result, rerender } = renderHook(() => useSessionStorage('storageKey_1'))
+    const { result } = renderHook(() => useSessionStorage('storageKey_1'))
     const [value] = result.current
 
-    rerender()
-
     expect(value).to.equal(null)
   })
 
   it('should return default value', () => {
-    const { result, rerender } = renderHook(() => useSessionStorage('storageKey_2', 100))
+    const { result } = renderHook(() => useSessionStorage('storageKey_2', 100))
     const [value] = result.current
 
-    rerender()
-
     expect(value).to.equal(100)
+    expect(JSON.parse(window.sessionStorage.getItem('storageKey_2'))).to.equal(100)
   })
 
   it('should store and return new values', () => {
-    const TestComponent = (props) => {
-      // eslint-disable-next-line react/prop-types
-      const { newValue } = props
-      const [value, setValue] = useSessionStorage('storageKey_2', 100)
+    const { result } = renderHook(() =>
+      useSessionStorage("storageKey_3", 100)
+    )
 
-      const setNewState = (v) => {
-        setValue(v)
-      }
+    expect(result.current[0]).to.equal(100)
+    expect(JSON.parse(window.sessionStorage.getItem('storageKey_3'))).to.equal(100)
 
-      useEffect(() => {
-        setNewState(newValue)
-      }, [])
+    act(() => {
+      result.current[1](200)
+    })
 
-      return <p>{value}</p>
-    }
+    expect(result.current[0]).to.equal(200)
+    expect(JSON.parse(window.sessionStorage.getItem('storageKey_3'))).to.equal(200)
+  })
+
+  it('should accept a callback argument for setValue', () => {
+    const { result } = renderHook(() =>
+      useSessionStorage("storageKey_4", 100)
+    )
 
-    const { container } = render(<TestComponent newValue={200} />)
+    expect(result.current[0]).to.equal(100)
+    expect(JSON.parse(window.sessionStorage.getItem('storageKey_4'))).to.equal(100)
 
-    expect(container.querySelector('p').innerHTML).to.equal('200')
+    act(() => {
+      result.current[1](prev => prev + 100)
+    })
+
+    expect(result.current[0]).to.equal(200)
+    expect(JSON.parse(window.sessionStorage.getItem('storageKey_4'))).to.equal(200)
   })
 
   it('should gracefully handle a getItem error and use the default value', () => {
@@ -66,17 +72,14 @@ describe('useSessionStorage', () => {
       },
     })
 
-    const { result, rerender } = renderHook(() =>
-      useSessionStorage("storageKey_3", 100)
+    const { result } = renderHook(() =>
+      useSessionStorage("storageKey_5", 100)
     )
     const [value] = result.current
-
-    rerender()
-
     expect(value).to.equal(100)
   })
 
-  it("should gracefully handle a setItem error and maintain the current value", () => {
+  it("should gracefully handle a setItem error and set the new value", () => {
     Object.defineProperty(window, "sessionStorage", {
       value: {
         ...window.sessionStorage,
@@ -86,14 +89,14 @@ describe('useSessionStorage', () => {
       },
     })
 
-    const { result, rerender } = renderHook(() =>
-      useSessionStorage("storageKey_4", 100)
+    const { result } = renderHook(() =>
+      useSessionStorage("storageKey_6", 100)
     )
-    const [value, setValue] = result.current
-    setValue(200)
 
-    rerender()
+    act(() => {
+      result.current[1](200)
+    })
 
-    expect(value).to.equal(100)
+    expect(result.current[0]).to.equal(200)
   })
 })