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

fix(vanilla): deal with promise resolving race condition #2199

Merged
merged 6 commits into from
Oct 23, 2023
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
48 changes: 22 additions & 26 deletions src/vanilla/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,48 +259,44 @@ export const createStore = () => {
): AtomState<Value> => {
if (isPromiseLike(valueOrPromise)) {
let continuePromise: (next: Promise<Awaited<Value>>) => void
const updatePromiseDependencies = () => {
const prevAtomState = getAtomState(atom)
if (
!prevAtomState ||
!hasPromiseAtomValue(prevAtomState) ||
prevAtomState.v !== promise
) {
// not the latest promise
return
}
// update dependencies, that could have changed
const nextAtomState = setAtomValue(
atom,
promise as Value,
nextDependencies
)
if (mountedMap.has(atom) && prevAtomState.d !== nextAtomState.d) {
mountDependencies(atom, nextAtomState, prevAtomState.d)
}
}
const promise: Promise<Awaited<Value>> & PromiseMeta<Awaited<Value>> =
new Promise((resolve, reject) => {
let settled = false
valueOrPromise.then(
(v) => {
if (!settled) {
settled = true
const prevAtomState = getAtomState(atom)
// update dependencies, that could have changed
const nextAtomState = setAtomValue(
atom,
promise as Value,
nextDependencies
)
resolvePromise(promise, v)
resolve(v as Awaited<Value>)
if (
mountedMap.has(atom) &&
prevAtomState?.d !== nextAtomState.d
) {
mountDependencies(atom, nextAtomState, prevAtomState?.d)
}
updatePromiseDependencies()
}
},
(e) => {
if (!settled) {
settled = true
const prevAtomState = getAtomState(atom)
// update dependencies, that could have changed
const nextAtomState = setAtomValue(
atom,
promise as Value,
nextDependencies
)
rejectPromise(promise, e)
reject(e)
if (
mountedMap.has(atom) &&
prevAtomState?.d !== nextAtomState.d
) {
mountDependencies(atom, nextAtomState, prevAtomState?.d)
}
updatePromiseDependencies()
}
}
)
Expand Down
80 changes: 77 additions & 3 deletions tests/react/async2.test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { StrictMode, Suspense } from 'react'
import { fireEvent, render } from '@testing-library/react'
import { describe, it } from 'vitest'
import { useAtomValue, useSetAtom } from 'jotai/react'
import { fireEvent, render, waitFor } from '@testing-library/react'
import { assert, describe, expect, it } from 'vitest'
import { useAtom, useAtomValue, useSetAtom } from 'jotai/react'
import { atom } from 'jotai/vanilla'

describe('useAtom delay option test', () => {
Expand Down Expand Up @@ -141,3 +141,77 @@ describe('atom read function setSelf option test', () => {
await findByText('text: hello1')
})
})

describe('timing issue with setSelf', () => {
it('resolves dependencies reliably after a delay (#2192)', async () => {
expect.assertions(1)
const countAtom = atom(0)

let result: number | null = null
const resolve: (() => void)[] = []
const asyncAtom = atom(async (get) => {
const count = get(countAtom)
await new Promise<void>((r) => resolve.push(r))
return count
})

const derivedAtom = atom(
async (get, { setSelf }) => {
get(countAtom)
await Promise.resolve()
const resultCount = await get(asyncAtom)
result = resultCount
if (resultCount === 2) setSelf() // <-- necessary
},
() => {}
)

const derivedSyncAtom = atom((get) => {
get(derivedAtom)
})

const increment = (c: number) => c + 1
function TestComponent() {
useAtom(derivedSyncAtom)
const [count, setCount] = useAtom(countAtom)
const onClick = () => {
setCount(increment)
setCount(increment)
}
return (
<>
count: {count}
<button onClick={onClick}>button</button>
</>
)
}

const { getByText, findByText } = render(
<StrictMode>
<TestComponent />
</StrictMode>
)

await waitFor(() => assert(resolve.length === 1))
resolve[0]!()

// The use of fireEvent is required to reproduce the issue
fireEvent.click(getByText('button'))

await waitFor(() => assert(resolve.length === 3))
resolve[1]!()
resolve[2]!()

await waitFor(() => assert(result === 2))

// The use of fireEvent is required to reproduce the issue
fireEvent.click(getByText('button'))

await waitFor(() => assert(resolve.length === 5))
resolve[3]!()
resolve[4]!()

await findByText('count: 4')
expect(result).toBe(4) // 3
})
})
55 changes: 54 additions & 1 deletion tests/vanilla/store.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { describe, expect, it, vi } from 'vitest'
import { waitFor } from '@testing-library/dom'
import { assert, describe, expect, it, vi } from 'vitest'
import { atom, createStore } from 'jotai/vanilla'
import type { Getter } from 'jotai/vanilla'

Expand Down Expand Up @@ -424,6 +425,58 @@ it('should update derived atoms during write (#2107)', async () => {
expect(store.get(countAtom)).toBe(2)
})

it('resolves dependencies reliably after a delay (#2192)', async () => {
expect.assertions(1)
const countAtom = atom(0)
let result: number | null = null

const resolve: (() => void)[] = []
const asyncAtom = atom(async (get) => {
const count = get(countAtom)
await new Promise<void>((r) => resolve.push(r))
return count
})

const derivedAtom = atom(
async (get, { setSelf }) => {
get(countAtom)
await Promise.resolve()
result = await get(asyncAtom)
if (result === 2) setSelf() // <-- necessary
},
() => {}
)

const store = createStore()
store.sub(derivedAtom, () => {})

await waitFor(() => assert(resolve.length === 1))

resolve[0]!()
const increment = (c: number) => c + 1
store.set(countAtom, increment)
store.set(countAtom, increment)

await waitFor(() => assert(resolve.length === 3))

resolve[1]!()
resolve[2]!()
await waitFor(() => assert(result === 2))

store.set(countAtom, increment)
store.set(countAtom, increment)

await waitFor(() => assert(resolve.length === 5))

resolve[3]!()
resolve[4]!()

await new Promise(setImmediate)
await waitFor(() => assert(store.get(countAtom) === 4))

expect(result).toBe(4) // 3
})

it('should not recompute a derived atom value if unchanged (#2168)', async () => {
const store = createStore()
const countAtom = atom(1)
Expand Down
Loading