pmndrs / jotai

👻 Primitive and flexible state management for React
https://jotai.org
MIT License
18.61k stars 607 forks source link

fix(store2): reusing continuable promise causes dependencies to be lost #2713

Closed dmaskasky closed 2 months ago

dmaskasky commented 2 months ago

Related Bug Reports or Discussions

https://github.com/jotaijs/jotai-effect/issues/42

Summary

When a promise is reused on subsequent recomputations, the dependencies are not preserved. ~This is affecting the functionality of atomEffect.~ A workaround has been found. This test is broken since https://github.com/pmndrs/jotai/commit/1a40452738cc40176d44d27b9e7f52dbd156f0ba

Check List

vercel[bot] commented 2 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
jotai ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 25, 2024 6:41am
codesandbox-ci[bot] commented 2 months ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

github-actions[bot] commented 2 months ago

LiveCodes Preview in LiveCodes

Latest commit: b712521de39ca7be9c2eb84816db92a44fbd8fd9
Last updated: Aug 25, 2024 6:40am (UTC)

Playground Link
React demo https://livecodes.io?x=id/3TMMXA4M4

See documentations for usage instructions.

dmaskasky commented 2 months ago

I think I understand the issue from the outside better.

When derivedAtom is read, it adds the dependency baseAtom. Then it returns a promise that resolves after the next microtask. Before the promise resolves, it calls setSelf, which updates baseAtom. This causes derivedAtom to recompute. Because inProgress is now true, it early returns the original promise before baseAtom is added as a dependency in this run. Before 2.9.0, the dependencies from prior runs would be preserved if the original promise is returned. However in 2.9.0, the dependencyMap is reset on each recursive run.

const baseAtom = atom(0)
let promise
const derivedAtom = atom<Promise<number>, [], void>(
  (get, { setSelf }) => {
    if (inProgress) {
      // bailing out when inProgress is true causes the original dependencies to be lost
      return promise
    }
    const value = get(baseAtom)
    promise = Promise.resolve().then(() => {
      inProgress = true
      // causes the derivedAtom to be re-evaluated, which triggers the inProgress bail
      setSelf()
      return value
    })
    return promise
  },
  (_get, _set) => {
    // setting baseAtom to Infinity causes the derivedAtom to be re-evaluated
    _set(baseAtom, c => c + 1)
  },
)
dai-shi commented 2 months ago

let inProgress = false

It feels like violating the contract. Can we somehow avoid it?

194cdc2 for example.

dmaskasky commented 2 months ago

It feels like violating the contract. Can we somehow avoid it?

194cdc2 for example.

Ok, I moved it to a ref atom.

dai-shi commented 2 months ago

It feels like violating the contract. Can we somehow avoid it? 194cdc2 for example.

Ok, I moved it to a ref atom.

Can you also divide derivedAtom into two atoms?

dai-shi commented 2 months ago

reusing continuable promise causes dependencies to be lost

is the expected behavior. I'll cherry-pick my commit and open a new PR.

dmaskasky commented 2 months ago

Thanks for looking into this further.