fix(vanilla): store should flush pending write triggered asynchronously and indirectly

[iwoplaza]

Related Issues or Discussions

Sorry for the onslaught of PRs, I believe this one to be an actual fix to a regression caused by the #2396 refactor

Summary

For this bug to occur a few things have to happen:

1. An atom that we wish to update indirectly, e.g.

const emailAtom = atom('[email protected]')

2. An atom that simply writes to the atom above in its write function, e.g.:

const updateEmailAtom = atom(null, (_get, set, value: string) => set(emailAtom, value.trim()))

3. An atom that executes the above atom asynchronously, e.g.:

const initializeAndVerifyAtom = atom(null, async (_get, set, fields: FormFields) => {
  const email = fields.email
  await verifyEmail(email)
  
  // this write is no longer sync, so should be flushed immediately, but the
  // actual atom that will get set (emailAtom) thinks it is being set synchronously,
  // so it just adds itself to the non-existent top of `pendingStack`.
  set(updateEmailAtom, email)
})

Instead of tracking isSync in writeAtomState, we can use the length of pendingStack to determine if the atom we are currently updating will get flushed in a batch, or do we have to handle the flushing immediately.

Check List

• yarn run prettier for formatting code and docs

[vercel]

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	Mar 13, 2024 10:34am

[codesandbox-ci]

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]

Preview in LiveCodes

Latest commit: 8afafc9
Last updated: Mar 13, 2024 10:33am (UTC)

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

---

See documentations for usage instructions.

[dai-shi]

This one looks good.
Did you confirm if the new test passes before #2396 refactor?

The fix seems a little bit unexpected to me, but probably can't be helped. I'll take another look when I'm relaxed.

[iwoplaza]

Can confirm that the new test passes before #2396, and fails right after.

[iwoplaza]

I updated the solution a bit, so that it behaves more like how it did before #2396. Since before this fix, setAtomState was pushing to a pending stack item that did not exist (and therefore was never flushed). This new solution ensures there is a Set in the pending stack if the setter was called asynchronously (with no item in the pending stack)

[dai-shi]

the new one feels a little bit better.

[iwoplaza]

What do you think about adding a console.warn('[Bug] tried to register flush onto an empty stack') outside of production mode, so:

const pendingStackTop = pendingStack[pendingStack.length - 1];
if (pendingStackTop) {
  pendingStackTop.add(a)
} else if (import.meta.env?.MODE !== 'production') {
  console.warn('[Bug] tried to register flush onto an empty stack')
}

instead of

pendingStack[pendingStack.length - 1]?.add(a)

might make it easier to catch bugs like this in the future, or to note running setAtomState without providing a value on the stack first is erroneous.

[dai-shi]

that sounds good. let's do it.

[iwoplaza]

Scratch that, there are places where setting the state of a mounted atom should not cause it to be flushed immediately, like in the updatePromiseDependencies function. The store implementation is still a little hard for me to maneuver, where is that update to the dependencies flushed?

[dai-shi]

I was skeptical about [a], so this looks good in code.

[dai-shi]

The store implementation is still a little hard for me to maneuver

Well, it's same for me.

[dai-shi]

I was planning to release it with v2.8.0.
But, as #2454 is reported, let's ship v2.7.1.