Skip to content

Commit

Permalink
fix(vanilla): bail out recomputing with the same value (#2016)
Browse files Browse the repository at this point in the history
* add a failing test

* a fix, is it hacky?

* refactor: simplify logic

* drop a test without sub, which may break in the future.

* fix with a hack
  • Loading branch information
dai-shi authored Jul 5, 2023
1 parent a9a639f commit 85cccb1
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 3 deletions.
14 changes: 11 additions & 3 deletions src/vanilla/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -373,9 +373,17 @@ export const createStore = () => {
// If a dependency changed since this atom was last computed,
// then we're out of date and need to recompute.
if (
Array.from(atomState.d).every(
([a, s]) => a === atom || getAtomState(a) === s
)
Array.from(atomState.d).every(([a, s]) => {
const aState = getAtomState(a)
return (
a === atom ||
aState === s ||
// TODO This is a hack, we should find a better solution.
(aState &&
!hasPromiseAtomValue(aState) &&
isEqualAtomValue(aState, s))
)
})
) {
return atomState
}
Expand Down
24 changes: 24 additions & 0 deletions tests/vanilla/store.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -348,3 +348,27 @@ it('should notify subscription with tree dependencies with bail-out', async () =
expect(cb).toBeCalledTimes(1)
expect(store.get(dep3Atom)).toBe(4)
})

it('should bail out with the same value with chained dependency (#2014)', async () => {
const store = createStore()
const objAtom = atom({ count: 1 })
const countAtom = atom((get) => get(objAtom).count)
const deriveFn = vi.fn((get: Getter) => get(countAtom))
const derivedAtom = atom(deriveFn)
const deriveFurtherFn = vi.fn((get: Getter) => {
get(objAtom) // intentional extra dependency
return get(derivedAtom)
})
const derivedFurtherAtom = atom(deriveFurtherFn)
const callback = vi.fn()
store.sub(derivedFurtherAtom, callback)
expect(store.get(derivedAtom)).toBe(1)
expect(store.get(derivedFurtherAtom)).toBe(1)
expect(callback).toHaveBeenCalledTimes(0)
expect(deriveFn).toHaveBeenCalledTimes(1)
expect(deriveFurtherFn).toHaveBeenCalledTimes(1)
store.set(objAtom, (obj) => ({ ...obj }))
expect(callback).toHaveBeenCalledTimes(0)
expect(deriveFn).toHaveBeenCalledTimes(1)
expect(deriveFurtherFn).toHaveBeenCalledTimes(2)
})

1 comment on commit 85cccb1

@vercel
Copy link

@vercel vercel bot commented on 85cccb1 Jul 5, 2023

Choose a reason for hiding this comment

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

Please sign in to comment.