Skip to content

Commit

Permalink
fix(core): recompute dependents in edge cases (#2742)
Browse files Browse the repository at this point in the history
* add failing test

* wip: a tentative fix for this special case

* complicate the test to make it fail

* wip: remove force flag

* comment
  • Loading branch information
dai-shi authored Oct 15, 2024
1 parent cb6dc0e commit 3b2c101
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 10 deletions.
18 changes: 9 additions & 9 deletions src/vanilla/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,13 +288,14 @@ const buildStore = (getAtomState: StoreArgs[0]): Store => {
pending: Pending | undefined,
atom: Atom<Value>,
atomState: AtomState<Value>,
force?: (a: AnyAtom) => boolean,
dirtyAtoms?: Set<AnyAtom>,
): AtomState<Value> => {
// See if we can skip recomputing this atom.
if (!force?.(atom) && isAtomStateInitialized(atomState)) {
// If the atom is mounted, we can use the cache.
if (isAtomStateInitialized(atomState)) {
// If the atom is mounted, we can use cached atom state.
// because it should have been updated by dependencies.
if (atomState.m) {
// We can't use the cache if the atom is dirty.
if (atomState.m && !dirtyAtoms?.has(atom)) {
return atomState
}
// Otherwise, check if the dependencies have changed.
Expand All @@ -304,8 +305,8 @@ const buildStore = (getAtomState: StoreArgs[0]): Store => {
([a, n]) =>
// Recursively, read the atom state of the dependency, and
// check if the atom epoch number is unchanged
readAtomState(pending, a, getAtomState(a, atomState), force).n ===
n,
readAtomState(pending, a, getAtomState(a, atomState), dirtyAtoms)
.n === n,
)
) {
return atomState
Expand All @@ -332,7 +333,7 @@ const buildStore = (getAtomState: StoreArgs[0]): Store => {
pending,
a,
getAtomState(a, atomState),
force,
dirtyAtoms,
)
if (isSync) {
addDependency(pending, atom, atomState, a, aState)
Expand Down Expand Up @@ -460,7 +461,6 @@ const buildStore = (getAtomState: StoreArgs[0]): Store => {
// Step 2: use the topsorted atom list to recompute all affected atoms
// Track what's changed, so that we can short circuit when possible
const changedAtoms = new Set<AnyAtom>([atom])
const isMarked = (a: AnyAtom) => markedAtoms.has(a)
for (let i = topsortedAtoms.length - 1; i >= 0; --i) {
const [a, aState, prevEpochNumber] = topsortedAtoms[i]!
let hasChangedDeps = false
Expand All @@ -471,7 +471,7 @@ const buildStore = (getAtomState: StoreArgs[0]): Store => {
}
}
if (hasChangedDeps) {
readAtomState(pending, a, aState, isMarked)
readAtomState(pending, a, aState, markedAtoms)
mountDependencies(pending, a, aState)
if (prevEpochNumber !== aState.n) {
addPendingAtom(pending, a, aState)
Expand Down
40 changes: 39 additions & 1 deletion tests/vanilla/dependency.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { expect, it } from 'vitest'
import { expect, it, vi } from 'vitest'
import { atom, createStore } from 'jotai/vanilla'

it('can propagate updates with async atom chains', async () => {
Expand Down Expand Up @@ -275,6 +275,44 @@ it('refreshes deps for each async read', async () => {
expect(values).toEqual([0, 1])
})

it('should not re-evaluate stable derived atom values in situations where dependencies are re-ordered (#2738)', () => {
const callCounter = vi.fn()
const countAtom = atom(0)
const rootAtom = atom(false)
const stableDep = atom((get) => {
get(rootAtom)
return 1
})
const stableDepDep = atom((get) => {
get(stableDep)
callCounter()
return 2 + get(countAtom)
})

const newAtom = atom((get) => {
if (get(rootAtom) || get(countAtom) > 0) {
return get(stableDepDep)
}

return get(stableDep)
})

const store = createStore()
store.sub(stableDepDep, () => {})
store.sub(newAtom, () => {})
expect(store.get(stableDepDep)).toBe(2)
expect(callCounter).toHaveBeenCalledTimes(1)

store.set(rootAtom, true)
expect(store.get(newAtom)).toBe(2)
expect(callCounter).toHaveBeenCalledTimes(1)

store.set(rootAtom, false)
store.set(countAtom, 1)
expect(store.get(newAtom)).toBe(3)
expect(callCounter).toHaveBeenCalledTimes(2)
})

it('handles complex dependency chains', async () => {
const baseAtom = atom(1)
const derived1 = atom((get) => get(baseAtom) * 2)
Expand Down

0 comments on commit 3b2c101

Please sign in to comment.