Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recurse #29

Merged
merged 1 commit into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ yarn add jotai-effect
```ts
type CleanupFn = () => void

type EffectFn = (get: Getter, set: Setter) => CleanupFn | void
type EffectFn = (get: Getter, set: Setter & { recurse: Setter }) => CleanupFn | void

function atomEffect(effectFn: EffectFn): Atom<void>
```
Expand Down Expand Up @@ -90,7 +90,7 @@ function MyComponent() {
</details>

- **Resistent To Infinite Loops:**
`atomEffect` does not rerun when it changes a value that it is watching.
`atomEffect` does not rerun when it changes a value that it is watching with `set`.

<!-- prettier-ignore -->
<details style="cursor: pointer; user-select: none;">
Expand All @@ -104,6 +104,24 @@ function MyComponent() {
set(countAtom, increment)
})
```
- **Supports Recursion:**
Recursion is supported with `set.recurse` for both sync and async use cases, but is not supported in the cleanup function.

<!-- prettier-ignore -->
<details style="cursor: pointer; user-select: none;">
<summary>Example</summary>

```js
const countAtom = atom(0)
atomEffect((get, set) => {
// increments count once per second
const count = get(countAtom)
const timeoutId = setTimeout(() => {
set.recurse(countAtom, increment)
}, 1000)
return () => clearTimeout(timeoutId)
})
```

</details>

Expand Down
26 changes: 26 additions & 0 deletions __tests__/atomEffect.strict.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,32 @@ it('should not cause infinite loops when effect updates the watched atom asynchr
expect(runCount).toBe(2)
})

it('should allow synchronous infinite loops with opt-in for first run', async () => {
expect.assertions(1)
let runCount = 0
const watchedAtom = atom(0)
const effectAtom = atomEffect((get, { recurse }) => {
const value = get(watchedAtom)
runCount++
if (value < 5) {
recurse(watchedAtom, increment)
}
})
const store = getDefaultStore()
function useTest() {
useAtom(effectAtom, { store })
const setCount = useSetAtom(watchedAtom, { store })
return () => act(async () => setCount(increment))
}
const { result } = renderHook(useTest, { wrapper })
await delay(0)
await act(async () => result.current())
await delay(0)
expect({ runCount, watched: store.get(watchedAtom) }).toEqual({
runCount: 7, // extra run for strict mode render
watched: 6,
})
})
it('should conditionally run the effect and cleanup when effectAtom is unmounted', async () => {
expect.assertions(6)

Expand Down
235 changes: 235 additions & 0 deletions __tests__/atomEffect.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,241 @@ it('should not cause infinite loops when effect updates the watched atom asynchr
expect(runCount).toBe(2)
})

it('should allow synchronous infinite loops with opt-in for first run', async () => {
expect.assertions(1)
let runCount = 0
const watchedAtom = atom(0)
let done = false
const effectAtom = atomEffect((get, { recurse }) => {
const value = get(watchedAtom)
runCount++
if (value >= 3) {
done = true
return
}
recurse(watchedAtom, increment)
})
const store = getDefaultStore()
store.sub(effectAtom, () => void 0)
await waitFor(() => assert(done))
expect({ runCount, watched: store.get(watchedAtom) }).toEqual({
runCount: 4,
watched: 3,
})
})

it('should allow synchronous infinite loops with opt-in', async () => {
expect.assertions(2)
let runCount = 0
const watchedAtom = atom(0)
const effectAtom = atomEffect((get, { recurse }) => {
const value = get(watchedAtom)
runCount++
if (value === 0) {
return
}
if (value >= 5) {
return
}
recurse(watchedAtom, increment)
})
const store = getDefaultStore()
store.sub(effectAtom, () => void 0)
await delay(0)
store.set(watchedAtom, increment)
await waitFor(() => assert(store.get(watchedAtom) === 5))
expect(store.get(watchedAtom)).toBe(5)
expect(runCount).toBe(6)
})

it('should allow multiple synchronous infinite loops with opt-in', async () => {
expect.assertions(1)
let runCount = 0
const watchedAtom = atom(0)
const effectAtom = atomEffect((get, { recurse }) => {
const value = get(watchedAtom)
runCount++
if (value === 0) {
return
}
if (value >= 3) {
return
}
recurse(watchedAtom, increment)
recurse(watchedAtom, increment)
})
const store = getDefaultStore()
store.sub(effectAtom, () => void 0)
await delay(0)
store.set(watchedAtom, increment)
await delay(0)
expect({ runCount, value: store.get(watchedAtom) }).toEqual({
runCount: 6,
value: 5,
})
})

it('should batch while synchronous recursing', async () => {
expect.assertions(2)
let runCount = 0
const lettersAtom = atom('a')
const numbersAtom = atom(0)
const watchedAtom = atom(0)
const lettersAndNumbersAtom = atom([] as string[])
const updateAtom = atom(0, (_get, set) => {
set(lettersAtom, incrementLetter)
set(numbersAtom, increment)
})
const effectAtom = atomEffect((get, set) => {
const letters = get(lettersAtom)
const numbers = get(numbersAtom)
get(watchedAtom)
const thisRunCount = runCount++
if (thisRunCount === 0) {
return
}
if (thisRunCount >= 3) {
return
}
set(lettersAndNumbersAtom, (lettersAndNumbers: string[]) => [
...lettersAndNumbers,
letters + String(numbers),
])
set.recurse(updateAtom)
})
const store = getDefaultStore()
store.sub(effectAtom, () => void 0)
await delay(0)
store.set(watchedAtom, increment)
await delay(0)
expect(store.get(lettersAndNumbersAtom)).toEqual(['a0', 'b1'])
expect(runCount).toBe(4)
})

it('should allow asynchronous infinite loops with task delay', async () => {
expect.assertions(2)
let runCount = 0
const watchedAtom = atom(0)
let done = false
const effectAtom = atomEffect((get, { recurse }) => {
const value = get(watchedAtom)
runCount++
if (value >= 3) {
done = true
return
}
delay(0).then(() => {
recurse(watchedAtom, increment)
})
})
const store = getDefaultStore()
store.sub(effectAtom, () => void 0)
await waitFor(() => assert(done))
expect(store.get(watchedAtom)).toBe(3)
expect(runCount).toBe(4)
})

it('should allow asynchronous infinite loops with microtask delay', async () => {
expect.assertions(2)
let runCount = 0
const watchedAtom = atom(0)
watchedAtom.debugLabel = 'watchedAtom' // remove
let done = false
const effectAtom = atomEffect((get, { recurse }) => {
const value = get(watchedAtom)
runCount++
if (value >= 3) {
done = true
return
}
Promise.resolve().then(() => {
recurse(watchedAtom, increment)
})
})
const store = getDefaultStore()
store.sub(effectAtom, () => void 0)
// await waitFor(() => assert(done))
done
await delay(500)
expect(store.get(watchedAtom)).toBe(3)
expect(runCount).toBe(4)
})

it('should work with both recurse and set', async () => {
expect.assertions(3)
let runCount = 0
const watchedAtom = atom(0)
const countAtom = atom(0)
const effectAtom = atomEffect((get, set) => {
const value = get(watchedAtom)
get(countAtom)
runCount++
if (value === 0 || value % 3) {
set.recurse(watchedAtom, increment)
set(countAtom, increment)
return
}
set(watchedAtom, increment)
})
const store = getDefaultStore()
store.sub(effectAtom, () => void 0)
await waitFor(() => assert(store.get(countAtom) === 3))
expect(store.get(countAtom)).toBe(3)
expect(store.get(watchedAtom)).toBe(4)
expect(runCount).toBe(4)
})

it('should disallow synchronous infinite loops in cleanup', async () => {
expect.assertions(3)
const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {})
let runCount = 0
const watchedAtom = atom(0)
const anotherAtom = atom(0)
const effectAtom = atomEffect((get, { recurse }) => {
get(watchedAtom)
get(anotherAtom)
runCount++
return () => {
recurse(watchedAtom, increment)
}
})
const store = getDefaultStore()
store.sub(effectAtom, () => void 0)
await delay(0)
store.set(anotherAtom, increment)
await delay(0)
expect(warnSpy).toHaveBeenCalled()
expect(store.get(watchedAtom)).toBe(0)
expect(runCount).toBe(2)
warnSpy.mockRestore()
})

// FIXME: is there a way to disallow asynchronous infinite loops in cleanup?
Copy link
Member Author

@dmaskasky dmaskasky Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dai-shi I couldn't find a way to detect and prohibit async recursion in the cleanup function.

atomEffect((get, set) => {
  get(watchedAtom)
  return () => {
    setTimeout(() => {
      set.recurse(watchedAtom, increment)
    }, 1000)
  }
})

IMO, recursion in the cleanup function is counter-intuitive and should not be allowed. The problem I ran into is how, technically, to distinguish it from async recursion in the effect function.

atomEffect((get, set) => {
  get(watchedAtom)
  setTimeout(() => {
    set.recurse(watchedAtom, increment)
  }, 1000)
})

I was wondering if you had any ideas.


it('should return value from recurse', async () => {
expect.assertions(1)
const countAtom = atom(0)
const incrementCountAtom = atom(null, (get, set) => {
set(countAtom, increment)
return get(countAtom)
})
const results = [] as number[]
let done = false
const effectAtom = atomEffect((get, { recurse }) => {
const value = get(countAtom)
if (value < 5) {
const result = recurse(incrementCountAtom)
results.unshift(result)
done = true
return
}
})
const store = getDefaultStore()
store.sub(effectAtom, () => void 0)
await waitFor(() => assert(done))
expect(results).toEqual([1, 2, 3, 4, 5])
})

it('should conditionally run the effect and cleanup when effectAtom is unmounted', async () => {
expect.assertions(6)

Expand Down
Loading