Skip to content

Commit

Permalink
🐛 adding the active hook during render isn't a good idea (after all)
Browse files Browse the repository at this point in the history
this became even more true after the introduction of React 18 that probably fix this issue with the need for hacks
  • Loading branch information
astoilkov committed Apr 7, 2022
1 parent 0fd466f commit 33189b3
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 40 deletions.
27 changes: 12 additions & 15 deletions src/useLocalStorageState.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import storage from './storage'
import { unstable_batchedUpdates } from 'react-dom'
import type { Dispatch, SetStateAction } from 'react'
import { useRef, useMemo, useEffect, useReducer, useCallback } from 'react'
import { useRef, useMemo, useEffect, useReducer, useCallback, useLayoutEffect } from 'react'

// `activeHooks` holds all active hooks. we use the array to update all hooks with the same key —
// calling `setValue` of one hook triggers an update for all other hooks with the same key
Expand Down Expand Up @@ -58,6 +58,10 @@ function useClientLocalStorageState<T>(
const [id, forceUpdate] = useReducer((number) => number + 1, 0)
const updateHooks = useCallback(() => {
unstable_batchedUpdates(() => {
// - it fixes "🐛 `setValue()` during render doesn't work":
// https://github.com/astoilkov/use-local-storage-state/issues/43
forceUpdate()

for (const hook of activeHooks) {
if (hook.key === key) {
hook.forceUpdate()
Expand Down Expand Up @@ -96,20 +100,13 @@ function useClientLocalStorageState<T>(

// - adds this hook to the `activeHooks` array. see the `activeHooks` declaration above for a
// more detailed explanation
// - not inside a React effect because:
// - it fixes "🐛 `setValue()` during render doesn't work":
// https://github.com/astoilkov/use-local-storage-state/issues/43
// - if you call `setValue()` during render and you have two localStorage instances with the
// same key — React throws an error. we want this behavior because otherwise it will just
// silently fail
const activeHookRef = useRef({ key, forceUpdate })
activeHooks.add(activeHookRef.current)
useEffect(
() => () => {
activeHooks.delete(activeHookRef.current)
},
[],
)
useLayoutEffect(() => {
const hook = { key, forceUpdate }
activeHooks.add(hook)
return (): void => {
activeHooks.delete(hook)
}
}, [key])

// - SSR support
// - not inside a `useLayoutEffect` because this way we skip the calls to `useEffect()` and
Expand Down
25 changes: 0 additions & 25 deletions test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -578,31 +578,6 @@ describe('useLocalStorageState()', () => {
expect(queryByText(/^1$/u)).toBeTruthy()
})

it(`calling setValue() during render when there are two instances of useLocalStorageState() with the same key should throw an error`, () => {
function App() {
return (
<>
<Component update={false} />
<Component update={true} />
</>
)
}

function Component({ update }: { update: boolean }) {
const [value, setValue] = useLocalStorageState('number', {
defaultValue: 0,
})

if (update && value === 0) {
setValue(1)
}

return <div>{value}</div>
}

expect(() => render(<App />)).toThrow()
})

it(`calling setValue() from useLayoutEffect() should update all useLocalStorageState() instances`, () => {
function App() {
return (
Expand Down

0 comments on commit 33189b3

Please sign in to comment.