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

feat(core): add atom hooks for atom mount, unmount and improve dev store #2895

Merged
merged 3 commits into from
Jan 3, 2025
+14 −20
Merged

feat(core): add atom hooks for atom mount, unmount and improve dev store #2895

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3223576
feat: add hook for atom mount and unmount
dmaskasky Dec 30, 2024
8501f1d
Apply suggestions from code review
dai-shi Jan 3, 2025
fd8dcb2
Merge branch 'main' into sync-mount-hook
dai-shi Jan 3, 2025
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
34 changes: 14 additions & 20 deletions src/vanilla/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ type AtomState<Value = AnyValue> = {
n: number
/** Object to store mounted state of the atom. */
m?: Mounted // only available if the atom is mounted
/** Listener to notify when the atom is mounted or unmounted. */
h?: () => void
Copy link
Member

Choose a reason for hiding this comment

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

If you want to change this with another character, we can reconsider.

Copy link
Collaborator Author

@dmaskasky dmaskasky Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

How about

h?: (action: 'm' | 'u', batch: Batch) => void

Copy link
Member

Choose a reason for hiding this comment

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

'm' | 'u' doesn't look too bad even though it increases the bundle size. I'm hesitant to expose batch here, and looking for an alternative.

Copy link
Member

Choose a reason for hiding this comment

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

We may change 'h' in the future, but as this is considered internal, it's fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm hesitant to expose batch here, and looking for an alternative.

Its pretty central to syncBatch scheduling.

Copy link
Member

Choose a reason for hiding this comment

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

I'm working on it, and not sure if it's successful, but I feel like reverting this PR... Will see.

Copy link
Member

Choose a reason for hiding this comment

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

Will see.

It failed.

/** Atom value */
v?: Value
/** Atom error */
Expand Down Expand Up @@ -625,6 +627,7 @@ const buildStore = (
d: new Set(atomState.d.keys()),
t: new Set(),
}
atomState.h?.()
if (isActuallyWritableAtom(atom)) {
const mounted = atomState.m
let setAtom: (...args: unknown[]) => unknown
Expand Down Expand Up @@ -674,6 +677,7 @@ const buildStore = (
addBatchFunc(batch, 'L', () => onUnmount(batch))
}
delete atomState.m
atomState.h?.()
// unmount dependencies
for (const a of atomState.d.keys()) {
const aMounted = unmountAtom(batch, a, getAtomState(a))
Expand Down Expand Up @@ -712,7 +716,6 @@ const buildStore = (
}

const deriveDevStoreRev4 = (store: Store): Store & DevStoreRev4 => {
const proxyAtomStateMap = new WeakMap()
const debugMountedAtoms = new Set<AnyAtom>()
let savedGetAtomState: StoreArgs[0]
let inRestoreAtom = 0
Expand All @@ -721,26 +724,17 @@ const deriveDevStoreRev4 = (store: Store): Store & DevStoreRev4 => {
savedGetAtomState = getAtomState
return [
(atom) => {
let proxyAtomState = proxyAtomStateMap.get(atom)
if (!proxyAtomState) {
const atomState = getAtomState(atom)
proxyAtomState = new Proxy(atomState, {
set(target, prop, value) {
if (prop === 'm') {
debugMountedAtoms.add(atom)
}
return Reflect.set(target, prop, value)
},
deleteProperty(target, prop) {
if (prop === 'm') {
debugMountedAtoms.delete(atom)
}
return Reflect.deleteProperty(target, prop)
},
})
proxyAtomStateMap.set(atom, proxyAtomState)
const atomState = getAtomState(atom)
const originalMounted = atomState.h
atomState.h = () => {
originalMounted?.()
if (atomState.m) {
debugMountedAtoms.add(atom)
} else {
debugMountedAtoms.delete(atom)
}
}
return proxyAtomState
return atomState
},
atomRead,
(atom, getter, setter, ...args) => {
Expand Down
Loading