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

Changes from 2 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
77 changes: 46 additions & 31 deletions src/vanilla/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,36 +488,42 @@ const buildStore = (getAtomState: StoreArgs[0]): Store => {
atomState: AtomState<Value>,
...args: Args
): Result => {
const getter: Getter = <V>(a: Atom<V>) =>
returnAtomValue(readAtomState(pending, a, getAtomState(a, atomState)))
const setter: Setter = <V, As extends unknown[], R>(
a: WritableAtom<V, As, R>,
...args: As
) => {
const aState = getAtomState(a, atomState)
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')
let isSync = true
try {
const getter: Getter = <V>(a: Atom<V>) =>
returnAtomValue(readAtomState(pending, a, getAtomState(a, atomState)))
const setter: Setter = <V, As extends unknown[], R>(
a: WritableAtom<V, As, R>,
...args: As
) => {
const aState = getAtomState(a, atomState)
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')
}
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)
}
} else {
r = writeAtomState(pending, a, aState, ...args) as R
}
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)
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, aState, ...args) as R
return r as R
}
flushPending(pending)
return r as R
return atom.write(getter, setter, ...args)
} finally {
isSync = false
}
const result = atom.write(getter, setter, ...args)
return result
}

const writeAtom = <Value, Args extends unknown[], Result>(
Expand Down Expand Up @@ -579,11 +585,20 @@ const buildStore = (getAtomState: StoreArgs[0]): Store => {
const mounted = atomState.m
const { onMount } = atom
addPendingFunction(pending, () => {
const onUnmount = onMount((...args) =>
writeAtomState(pending, atom, atomState, ...args),
)
if (onUnmount) {
mounted.u = onUnmount
let isSync = true
try {
const onUnmount = onMount((...args) => {
const result = writeAtomState(pending, atom, atomState, ...args)
if (!isSync) {
flushPending(pending)
}
return result
})
if (onUnmount) {
mounted.u = onUnmount
}
} finally {
isSync = false
}
})
}
Expand Down