fix: flushPending in async write #2804
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
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. |
|
Size Change: +369 B (+0.41%) Total Size: 89.7 kB
ℹ️ View Unchanged
|
|
| Playground | Link |
|---|---|
| React demo | https://livecodes.io?x=id/DZYHUW7L4 |
See documentations for usage instructions.
| if (!isSync) { | ||
| flushPending(pending) | ||
| } |
There was a problem hiding this comment.
Edge case: do we care if this changes the behavior a little bit for subscriber listeners?
const a = atom(0)
const w = atom(null, (get, set) => {
console.log('before write')
set(a, v=> v + 1)
console.log('after write')
})
w.onMount = (setSelf) => {
setSelf()
console.log('after setSelf')
}
store.sub(a, () => {
console.log('a listener fired')
})
store.sub(w, () => {})
/*
CURRENT:
1. 'before write'
2. 'a listener fired'
3. 'after write'
4. 'after setSelf'
NEW:
1. 'before write'
2. 'after write'
3. 'after setSelf'
4. 'a listener fired'
*/ There was a problem hiding this comment.
Is it why the current test is failing?
Do you know how to make a failing test if we don't have isSync?
There was a problem hiding this comment.
Is it why the current test is failing?
Yes
Do you know how to make a failing test if we don't have
isSync?
Thinking on this more...
I think we need to keep this synchronous call to flushPending in set. Some third-party libraries depend on this behavior, so we need some way to call all subscribers.
There was a problem hiding this comment.
Hm, I was thinking more to the opposite. "NEW" behavior seems more reasonable than "CURRENT" behavior. I'm not sure if I understand those edge cases.
There was a problem hiding this comment.
Yes, I agree. This is better.
Hmm, it looks like atomWithObservable and atomWithStorage are not failing after all. I no longer think the behavior has changed in any significant way.
* call onmount flushPending in finally block * temp remove unmount test cases. to be fixed later with pending injection (#2810) * flushpending everywhere * Apply suggestions from code review --------- Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>
We used to have
isSyncin writeAtomState to conditionally flush pending, but when I did #2463, I removed it. I thought it worked without it, but the behavior wasn't something expected. This PR introducesisSyncagain in writeAtomSync, and also in mountAtom and unmountAtom.