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.

Sign up for GitHub

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: flushPending in async write #2804

Merged
merged 8 commits into from
Nov 14, 2024
Merged

fix: flushPending in async write #2804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e452fb6
fix: flushPending in async write
dai-shi Nov 10, 2024
843583b
isSync in onMount to
dai-shi Nov 10, 2024
a8b9b2a
Merge branch 'main' into fix/flush-pending-async
dai-shi Nov 11, 2024
c6d57c5
apply the change
dai-shi Nov 11, 2024
8f58d5f
add test for flushPending (#2809)
dmaskasky Nov 11, 2024
d3135c0
Merge branch 'main' into fix/flush-pending-async
dai-shi Nov 14, 2024
e900fbc
Fix/flush pending finally everywhere (#2816)
dmaskasky Nov 14, 2024
484b5c4
fix type assertion
dai-shi Nov 14, 2024
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
101 changes: 66 additions & 35 deletions src/vanilla/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,15 +333,18 @@ const buildStore = (
}
// a !== atom
const aState = readAtomState(pending, a, dirtyAtoms)
if (isSync) {
addDependency(pending, atom, atomState, a, aState)
} else {
const pending = createPending()
addDependency(pending, atom, atomState, a, aState)
mountDependencies(pending, atom, atomState)
flushPending(pending)
try {
return returnAtomValue(aState)
} finally {
if (isSync) {
addDependency(pending, atom, atomState, a, aState)
} else {
const pending = createPending()
addDependency(pending, atom, atomState, a, aState)
mountDependencies(pending, atom, atomState)
flushPending(pending)
}
}
return returnAtomValue(aState)
}
let controller: AbortController | undefined
let setSelf: ((...args: unknown[]) => unknown) | undefined
Expand Down Expand Up @@ -485,46 +488,56 @@ const buildStore = (
atom: WritableAtom<Value, Args, Result>,
...args: Args
): Result => {
let isSync = true
const getter: Getter = <V>(a: Atom<V>) =>
returnAtomValue(readAtomState(pending, a))
const setter: Setter = <V, As extends unknown[], R>(
a: WritableAtom<V, As, R>,
...args: As
) => {
const aState = getAtomState(a)
let r: R | undefined
if (isSelfAtom(atom, a)) {
if (!hasInitialValue(a)) {
// NOTE technically possible but restricted as it may cause bugs
throw new Error('atom not writable')
try {
if (isSelfAtom(atom, a)) {
if (!hasInitialValue(a)) {
// NOTE technically possible but restricted as it may cause bugs
throw new Error('atom not writable')
}
const hasPrevValue = 'v' in aState
const prevValue = aState.v
const v = args[0] as V
setAtomStateValueOrPromise(a, aState, v)
mountDependencies(pending, a, aState)
if (!hasPrevValue || !Object.is(prevValue, aState.v)) {
addPendingAtom(pending, a, aState)
recomputeDependents(pending, a, aState)
}
return undefined as R
} else {
return writeAtomState(pending, a, ...args)
}
const hasPrevValue = 'v' in aState
const prevValue = aState.v
const v = args[0] as V
setAtomStateValueOrPromise(a, aState, v)
mountDependencies(pending, a, aState)
if (!hasPrevValue || !Object.is(prevValue, aState.v)) {
addPendingAtom(pending, a, aState)
recomputeDependents(pending, a, aState)
} finally {
if (!isSync) {
flushPending(pending)
}
Comment on lines +519 to 521
Copy link
Collaborator

Choose a reason for hiding this comment

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

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'
*/

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it why the current test is failing?

Do you know how to make a failing test if we don't have isSync?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@dmaskasky dmaskasky Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

} else {
r = writeAtomState(pending, a, ...args) as R
}
flushPending(pending)
return r as R
}
const result = atomWrite(atom, getter, setter, ...args)
return result
try {
return atomWrite(atom, getter, setter, ...args)
} finally {
isSync = false
}
}

const writeAtom = <Value, Args extends unknown[], Result>(
atom: WritableAtom<Value, Args, Result>,
...args: Args
): Result => {
const pending = createPending()
const result = writeAtomState(pending, atom, ...args)
flushPending(pending)
return result
try {
return writeAtomState(pending, atom, ...args)
} finally {
flushPending(pending)
}
}

const mountDependencies = (
Expand Down Expand Up @@ -575,11 +588,29 @@ const buildStore = (
if (isActuallyWritableAtom(atom)) {
const mounted = atomState.m
addPendingFunction(pending, () => {
const onUnmount = atomOnMount(atom, (...args) =>
writeAtomState(pending, atom, ...args),
)
if (onUnmount) {
mounted.u = onUnmount
let isSync = true
try {
const onUnmount = atomOnMount(atom, (...args) => {
try {
return writeAtomState(pending, atom, ...args)
} finally {
if (!isSync) {
flushPending(pending)
}
}
})
if (onUnmount) {
mounted.u = () => {
isSync = true
try {
onUnmount()
} finally {
isSync = false
}
}
}
} finally {
isSync = false
}
})
}
Expand Down
67 changes: 67 additions & 0 deletions tests/vanilla/store.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,3 +579,70 @@ it('should update derived atom even if dependances changed (#2697)', () => {
store.set(primitiveAtom, 1)
expect(onChangeDerived).toHaveBeenCalledTimes(1)
})

describe('should invoke flushPending only after all atoms are updated (#2804)', () => {
const store = createStore()

it('should invoke flushPending only after all atoms are updated with set', () => {
const a = atom(0)
const setResult = []
const w = atom(null, (_get, set, value: number) => {
setResult.push('before set')
set(a, value)
setResult.push('after set')
})
store.sub(a, () => {
setResult.push('a value changed - ' + store.get(a))
})
setResult.push('before store.set')
store.set(w, 1)
setResult.push('after store.set')
expect(setResult).not.toEqual([
'before store.set',
'before set',
'a value changed - 1',
'after set',
'after store.set',
])
expect(setResult).toEqual([
'before store.set',
'before set',
'after set',
'a value changed - 1',
'after store.set',
])
})

it('should invoke flushPending only after all atoms are updated with mount', () => {
const mountResult = []
const a = atom(0)
const m = atom(null, (_get, set, value: number) => {
set(a, value)
})
m.onMount = (setSelf) => {
mountResult.push('before onMount setSelf')
setSelf(1)
mountResult.push('after onMount setSelf')
}
mountResult.push('before store.sub')
store.sub(a, () => {
mountResult.push('a value changed - ' + store.get(a))
})
const unsub = store.sub(m, () => {})
mountResult.push('after store.sub')
expect(mountResult).not.toEqual([
'before store.sub',
'before onMount setSelf',
'a value changed - 1',
'after onMount setSelf',
'after store.sub',
])
expect(mountResult).toEqual([
'before store.sub',
'before onMount setSelf',
'after onMount setSelf',
'a value changed - 1',
'after store.sub',
])
})
})