Skip to content

fix: should update dependents with the value of the unwrapped atom when the promise resolves #2920

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

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 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
19 changes: 14 additions & 5 deletions src/vanilla/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ type Mounted = {
readonly l: Set<() => void>
/** Set of mounted atoms that the atom depends on. */
readonly d: Set<AnyAtom>
/** Set of dependencies added asynchronously */
readonly q: Set<AnyAtom>
/** Set of mounted atoms that depends on the atom. */
readonly t: Set<AnyAtom>
/** Function to run when the atom is unmounted. */
Expand Down Expand Up @@ -310,10 +312,8 @@ const buildStore = (...storeArgs: StoreArgs): Store => {
for (const a of atomState.d.keys()) {
addPendingPromiseToDependency(atom, valueOrPromise, ensureAtomState(a))
}
atomState.v = valueOrPromise
} else {
atomState.v = valueOrPromise
}
atomState.v = valueOrPromise
delete atomState.e
delete atomState.x
if (!hasPrevValue || !Object.is(prevValue, atomState.v)) {
Expand Down Expand Up @@ -375,6 +375,9 @@ const buildStore = (...storeArgs: StoreArgs): Store => {
addDependency(atom, atomState, a, aState)
} else {
const batch = createBatch()
if (isPendingPromise(atomState.v) && !atomState.m?.d.has(a)) {
atomState.m?.q.add(a)
}
addDependency(atom, atomState, a, aState)
mountDependencies(batch, atom, atomState)
flushBatch(batch)
Expand Down Expand Up @@ -414,6 +417,11 @@ const buildStore = (...storeArgs: StoreArgs): Store => {
const valueOrPromise = atomRead(atom, getter, options as never)
setAtomStateValueOrPromise(atom, atomState, valueOrPromise)
if (isPromiseLike(valueOrPromise)) {
if (batch) {
addBatchFunc(batch, 0, () => atomState.m?.q.clear())
} else {
atomState.m?.q.clear()
}
valueOrPromise.onCancel?.(() => controller?.abort())
const complete = () => {
if (atomState.m) {
Expand Down Expand Up @@ -594,7 +602,7 @@ const buildStore = (...storeArgs: StoreArgs): Store => {
atom: AnyAtom,
atomState: AtomState,
) => {
if (atomState.m && !isPendingPromise(atomState.v)) {
if (atomState.m) {
for (const a of atomState.d.keys()) {
if (!atomState.m.d.has(a)) {
const aMounted = mountAtom(batch, a, ensureAtomState(a))
Expand All @@ -603,7 +611,7 @@ const buildStore = (...storeArgs: StoreArgs): Store => {
}
}
for (const a of atomState.m.d || []) {
if (!atomState.d.has(a)) {
if (!atomState.d.has(a) && !atomState.m.q.has(a)) {
atomState.m.d.delete(a)
const aMounted = unmountAtom(batch, a, ensureAtomState(a))
aMounted?.t.delete(atom)
Expand All @@ -629,6 +637,7 @@ const buildStore = (...storeArgs: StoreArgs): Store => {
atomState.m = {
l: new Set(),
d: new Set(atomState.d.keys()),
q: new Set(),
t: new Set(),
}
atomState.h?.(batch)
Expand Down
86 changes: 86 additions & 0 deletions tests/vanilla/dependency.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -425,3 +425,89 @@ it('batches sync writes', () => {
expect(fetch).toBeCalledWith(1)
expect(store.get(a)).toBe(1)
})

it('mounts and unmounts sync and async dependencies correctly', async () => {
const mounted = [0, 0, 0, 0, 0] as [number, number, number, number, number]
const a = atom(0)
a.onMount = () => {
++mounted[0]
return () => {
--mounted[0]
}
}

const b = atom(0)
b.onMount = () => {
++mounted[1]
return () => {
--mounted[1]
}
}

const c = atom(0)
c.onMount = () => {
++mounted[2]
return () => {
--mounted[2]
}
}

const d = atom(0)
d.onMount = () => {
++mounted[3]
return () => {
--mounted[3]
}
}

const e = atom(0)
e.onMount = () => {
++mounted[4]
return () => {
--mounted[4]
}
}

let resolve: (() => Promise<void>) | undefined
const f = atom((get) => {
if (!get(a)) {
get(b)
} else {
get(c)
}
const promise = new Promise<void>((r) => {
resolve = () => {
r()
return promise
}
}).then(() => {
if (!get(a)) {
get(d)
} else {
get(e)
}
})
return promise
})

const store = createStore()
// mount a, b synchronously
const unsub = store.sub(f, () => {})
expect(mounted).toEqual([1, 1, 0, 0, 0])

// mount d asynchronously
await resolve!()
expect(mounted).toEqual([1, 1, 0, 1, 0])

// unmount b, mount c synchronously
store.set(a, 1)
expect(mounted).toEqual([1, 0, 1, 1, 0])

// unmount d, mount e asynchronously
await resolve!()
expect(mounted).toEqual([1, 0, 1, 0, 1])

// unmount a, b, d, e synchronously
unsub()
expect(mounted).toEqual([0, 0, 0, 0, 0])
})
1 change: 0 additions & 1 deletion tests/vanilla/store.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1091,7 +1091,6 @@ it('recomputes dependents of unmounted atoms', () => {
const a = atom(0)
a.debugLabel = 'a'
const bRead = vi.fn((get: Getter) => {
console.log('bRead')
return get(a)
})
const b = atom(bRead)
Expand Down
14 changes: 14 additions & 0 deletions tests/vanilla/utils/unwrap.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,3 +150,17 @@ describe('unwrap', () => {
expect(store.get(syncAtom)).toEqual('concrete')
})
})

it('should update dependents with the value of the unwrapped atom when the promise resolves', async () => {
const store = createStore()
const asyncTarget = atom(() => Promise.resolve('value'))
const target = unwrap(asyncTarget)
const results: string[] = []
const derived = atom(async (get) => {
await Promise.resolve()
results.push('effect ' + get(target))
})
store.sub(derived, () => {})
await new Promise((r) => setTimeout(r))
expect(results).toEqual(['effect undefined', 'effect value'])
})
Comment on lines +154 to +166
Copy link
Member

Choose a reason for hiding this comment

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

Looks good. So, it's a regression, right? My bad... I'll see what I can do with the impl. It will be step by step and take a bit of time. Sorry folks for waiting.

Loading