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(store): derived atom not updating when its value changes if another atom is read it conditionally #2700

Merged
merged 4 commits into from
Aug 12, 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
28 changes: 15 additions & 13 deletions src/vanilla/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -466,8 +466,7 @@ export const createStore = (): Store => {
returnAtomValue(readAtomState(undefined, atom))

const recomputeDependents = (pending: Pending, atom: AnyAtom) => {
const getDependents = (a: AnyAtom): Set<AnyAtom> => {
const aState = getAtomState(a)
const getDependents = (a: AnyAtom, aState: AtomState): Set<AnyAtom> => {
const dependents = new Set(aState.m?.t)
for (const atomWithPendingContinuablePromise of aState.p) {
dependents.add(atomWithPendingContinuablePromise)
Expand All @@ -484,23 +483,28 @@ export const createStore = (): Store => {

// Step 1: traverse the dependency graph to build the topsorted atom list
// We don't bother to check for cycles, which simplifies the algorithm.
const topsortedAtoms: AnyAtom[] = []
const topsortedAtoms: (readonly [
atom: AnyAtom,
atomState: AtomState,
epochNumber: number,
])[] = []
const markedAtoms = new Set<AnyAtom>()
const visit = (n: AnyAtom) => {
if (markedAtoms.has(n)) {
const visit = (a: AnyAtom) => {
if (markedAtoms.has(a)) {
return
}
markedAtoms.add(n)
for (const m of getDependents(n)) {
markedAtoms.add(a)
const aState = getAtomState(a)
for (const d of getDependents(a, aState)) {
// we shouldn't use isSelfAtom here.
if (n !== m) {
visit(m)
if (a !== d) {
visit(d)
}
}
// The algorithm calls for pushing onto the front of the list. For
// performance, we will simply push onto the end, and then will iterate in
// reverse order later.
topsortedAtoms.push(n)
topsortedAtoms.push([a, aState, aState.n])
}
// Visit the root atom. This is the only atom in the dependency graph
// without incoming edges, which is one reason we can simplify the algorithm
Expand All @@ -510,9 +514,7 @@ export const createStore = (): Store => {
const changedAtoms = new Set<AnyAtom>([atom])
const isMarked = (a: AnyAtom) => markedAtoms.has(a)
for (let i = topsortedAtoms.length - 1; i >= 0; --i) {
const a = topsortedAtoms[i]!
const aState = getAtomState(a)
const prevEpochNumber = aState.n
const [a, aState, prevEpochNumber] = topsortedAtoms[i]!
let hasChangedDeps = false
for (const dep of aState.d.keys()) {
if (dep !== a && changedAtoms.has(dep)) {
Expand Down
20 changes: 20 additions & 0 deletions tests/vanilla/store.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -572,3 +572,23 @@ it('Unmount an atom that is no longer dependent within a derived atom (#2658)',
store.set(condAtom, false)
expect(onUnmount).toHaveBeenCalledTimes(1)
})

it('should update derived atom even if dependances changed (#2697)', () => {
const primitiveAtom = atom<number | undefined>(undefined)
const derivedAtom = atom((get) => get(primitiveAtom))
const conditionalAtom = atom((get) => {
const base = get(primitiveAtom)
if (!base) return
return get(derivedAtom)
})

const store = createStore()
const onChangeDerived = vi.fn()

store.sub(derivedAtom, onChangeDerived)
store.sub(conditionalAtom, () => {})

expect(onChangeDerived).toHaveBeenCalledTimes(0)
store.set(primitiveAtom, 1)
expect(onChangeDerived).toHaveBeenCalledTimes(1)
})