feat(core) atom.whileWatched (useEffect-like primitive) and createExternalStoreAtom#312
Open
justjake wants to merge 16 commits intoTanStack:mainfrom
Open
feat(core) atom.whileWatched (useEffect-like primitive) and createExternalStoreAtom#312justjake wants to merge 16 commits intoTanStack:mainfrom
justjake wants to merge 16 commits intoTanStack:mainfrom
Conversation
justjake
changed the title
📝 WalkthroughWalkthroughAdds a "while watched" lifecycle to atoms and stores and a new Changes
Sequence DiagramsequenceDiagram
participant Subscriber
participant DerivedAtom as Derived Atom
participant TriggerAtom as Trigger Atom
participant ExternalStore as External Store
Subscriber->>DerivedAtom: subscribe()
DerivedAtom->>TriggerAtom: depend (derived reads trigger)
TriggerAtom->>TriggerAtom: watched() invoked -> run _watchedSubs
TriggerAtom->>ExternalStore: subscribe(invalidate) (via whileWatched)
ExternalStore->>TriggerAtom: invalidate() (on external change)
TriggerAtom->>DerivedAtom: notify dependency changed
DerivedAtom->>ExternalStore: getSnapshot()
DerivedAtom->>Subscriber: deliver new value
Subscriber->>DerivedAtom: unsubscribe()
DerivedAtom->>TriggerAtom: last subscriber removed (unwatched)
TriggerAtom->>TriggerAtom: unwatched() invoked -> run _watchedCleanups
TriggerAtom->>ExternalStore: call unsubscribe()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
packages/store/tests/while-watched.test.ts (1)
103-124: Add astop()-while-watched test case.Current
stop()tests cover idempotency and future activations, but not cancellation during an active watch. Add a case wherestop()is called before unsubscribing and assert cleanup runs immediately and only once.Suggested test addition
test('stop() is idempotent', () => { const atom = createAtom(0) const stop = atom.whileWatched(() => {}) expect(() => { stop() stop() }).not.toThrow() }) + +test('stop() while watched runs cleanup immediately and only once', () => { + const atom = createAtom(0) + const cleanup = vi.fn() + const stop = atom.whileWatched(() => cleanup) + const sub = atom.subscribe(() => {}) + + stop() + expect(cleanup).toHaveBeenCalledTimes(1) + + sub.unsubscribe() + expect(cleanup).toHaveBeenCalledTimes(1) +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/store/tests/while-watched.test.ts` around lines 103 - 124, Add a new test to verify that calling stop() during an active watch cancels immediately and runs cleanup exactly once: create an atom via createAtom(0), call atom.whileWatched(effect) to get stop, ensure the effect was invoked once after subscribing (use atom.subscribe(() => {}) and keep the subscription active), then call stop() before unsubscribing and assert the cleanup/effect-run count increments exactly once and that subsequent unsubscribe does not call the cleanup again; reference the whileWatched method and the returned stop function and use a vi.fn() for effect to assert exact call counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/functions/createExternalStoreAtom.md`:
- Line 15: The "Defined in" link for createExternalStoreAtom points to a fork
URL; update the link in docs/reference/functions/createExternalStoreAtom.md (the
"Defined in" line) to use the canonical main repository path pointing to atom.ts
at the correct location (referenced symbol: createExternalStoreAtom /
atom.ts:190) so the documentation links to the upstream repo instead of the
fork.
- Around line 17-36: Fix the markdown by closing the open code fence for the
example and correcting the typo: change "Thic" to "This" in the description for
createExternalStoreAtom and add a terminating triple backtick (```) immediately
before the "## Type Parameters" heading so the example block ends properly.
In `@docs/reference/type-aliases/WatchedEffect.md`:
- Line 12: Update the "Defined in" link in
docs/reference/type-aliases/WatchedEffect.md so it points to the project's
canonical repository URL instead of the fork; locate the "Defined in:
[atom.ts:33](...)" line and replace the current forked GitHub URL with the main
repo URL that matches the package source (referencing atom.ts line 33) so future
readers get the authoritative source reference.
- Around line 9-10: The documented type signature for WatchedEffect is wrong:
update the rendered signature to indicate the callback may return either a
cleanup function (() => void), void, or undefined (i.e., (() => void) | void |
undefined) so it doesn't imply a function is always returned; change the
WatchedEffect documentation and any other occurrences (notably the similar
entries around lines 17-19) to use that corrected union form and ensure the
prose matches this behavior.
In `@packages/store/src/atom.ts`:
- Around line 171-173: The JSDoc for the exported API describing the "pulls
external state into an atom" behavior has a typo ("Thic"); update the comment
text to read "This can be used..." in the JSDoc block that documents the
external-state-to-atom utility (the comment above the function that implements
the useSyncExternalStore-like atom behavior) so the public docs are correct.
- Around line 265-273: stop() currently only removes the listener from
atom._watchedSubs but does not invoke any active cleanup when atom._watched is
true; update stop() so that when removing the listener it also runs and clears
the listener's active cleanup (e.g. call listener._cleanup() or
listener.cleanup() if present and then delete that property), and if removing
the last watched subscriber set atom._watched to false and invoke/clear any
atom-level cleanup (atom._cleanup) as needed; reference the stop() function,
atom._watchedSubs, atom._watched, listener._cleanup / listener.cleanup, and
atom._cleanup to locate and implement these changes.
In `@packages/store/src/types.ts`:
- Line 40: Fix the minor grammar in the API doc comment for the return value of
the whileWatched-related type by changing "an `cleanup` function" to "a
`cleanup` function"; update the doc comment that describes the return value for
the whileWatched listener (the text mentioning `cleanup`) so it reads "a
`cleanup` function" instead of "an `cleanup`".
---
Nitpick comments:
In `@packages/store/tests/while-watched.test.ts`:
- Around line 103-124: Add a new test to verify that calling stop() during an
active watch cancels immediately and runs cleanup exactly once: create an atom
via createAtom(0), call atom.whileWatched(effect) to get stop, ensure the effect
was invoked once after subscribing (use atom.subscribe(() => {}) and keep the
subscription active), then call stop() before unsubscribing and assert the
cleanup/effect-run count increments exactly once and that subsequent unsubscribe
does not call the cleanup again; reference the whileWatched method and the
returned stop function and use a vi.fn() for effect to assert exact call counts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7ec2563-56c3-4ec7-a40b-106287ae95e8
📒 Files selected for processing (11)
.changeset/ninety-actors-hide.mddocs/config.jsondocs/reference/functions/createExternalStoreAtom.mddocs/reference/type-aliases/WatchedEffect.mdpackages/store/src/alien.tspackages/store/src/atom.tspackages/store/src/signal.tspackages/store/src/store.tspackages/store/src/types.tspackages/store/tests/external-store-atom.test.tspackages/store/tests/while-watched.test.ts
Comment on lines
+9
to
+10
| type WatchedEffect = () => () => void | void | undefined; | ||
| ``` |
There was a problem hiding this comment.
Type signature rendering is inaccurate.
The doc currently implies the callback always returns a function. It should reflect (() => void) | void | undefined.
🛠️ Proposed signature/returns fix
-type WatchedEffect = () => () => void | void | undefined;
+type WatchedEffect = () => (() => void) | void | undefined;
...
-() => `void` \| `void` \| `undefined`
+(() => `void`) \| `void` \| `undefined`Also applies to: 17-19
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/reference/type-aliases/WatchedEffect.md` around lines 9 - 10, The
documented type signature for WatchedEffect is wrong: update the rendered
signature to indicate the callback may return either a cleanup function (() =>
void), void, or undefined (i.e., (() => void) | void | undefined) so it doesn't
imply a function is always returned; change the WatchedEffect documentation and
any other occurrences (notably the similar entries around lines 17-19) to use
that corrected union form and ensure the prose matches this behavior.
Comment on lines
+265
to
+273
| return () => { | ||
| if (!atom._watchedSubs) { | ||
| return | ||
| } | ||
| const index = atom._watchedSubs.indexOf(listener) | ||
| if (index !== -1) { | ||
| atom._watchedSubs.splice(index, 1) | ||
| } | ||
| } |
There was a problem hiding this comment.
stop() does not clean up active watchers, causing leaks while still watched.
On Line 265–Line 273, stop() removes the listener from _watchedSubs but does not run/remove its active cleanup when _watched === true. This can leave resources (e.g. external subscriptions) active until full unwatch.
Proposed fix
whileWatched(listener: WatchedEffect): () => void {
atom._watchedSubs ??= []
atom._watchedSubs.push(listener)
if (atom._watched) {
atom._watchedCleanups ??= []
atom._watchedCleanups.push(listener())
}
return () => {
if (!atom._watchedSubs) {
return
}
const index = atom._watchedSubs.indexOf(listener)
if (index !== -1) {
atom._watchedSubs.splice(index, 1)
+ if (atom._watched && atom._watchedCleanups) {
+ const [cleanup] = atom._watchedCleanups.splice(index, 1)
+ cleanup?.()
+ }
}
}
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/store/src/atom.ts` around lines 265 - 273, stop() currently only
removes the listener from atom._watchedSubs but does not invoke any active
cleanup when atom._watched is true; update stop() so that when removing the
listener it also runs and clears the listener's active cleanup (e.g. call
listener._cleanup() or listener.cleanup() if present and then delete that
property), and if removing the last watched subscriber set atom._watched to
false and invoke/clear any atom-level cleanup (atom._cleanup) as needed;
reference the stop() function, atom._watchedSubs, atom._watched,
listener._cleanup / listener.cleanup, and atom._cleanup to locate and implement
these changes.
Author
There was a problem hiding this comment.
It would be a bug to remove / fire the teardown function when removing the setup subscriber.
There was a problem hiding this comment.
@justjake, understood — thank you for the clarification. The design intent is that stop() only de-registers the effect from future watch cycles; the current cycle's cleanup still lives in _watchedCleanups and fires naturally when the atom loses its last subscriber via the unwatched hook. Firing the teardown eagerly in stop() would incorrectly cancel a still-active subscription mid-watch. I'll retract the suggestion.
(◕‿◕✿)
✏️ Learnings added
Learnt from: justjake
Repo: TanStack/store PR: 312
File: packages/store/src/atom.ts:265-273
Timestamp: 2026-04-17T17:42:08.842Z
Learning: In `packages/store/src/atom.ts` (TanStack Store), the `stop()` function returned by `atom.whileWatched(listener)` is intentionally designed to only remove the listener from `_watchedSubs` (preventing future re-registration) without firing or removing the active cleanup from `_watchedCleanups`. The current cycle's cleanup remains in `_watchedCleanups` and is called naturally when the atom becomes unwatched (via the `unwatched` hook calling `_watchedCleanups.forEach`). Eagerly running the cleanup in `stop()` would be a bug, as the subscription should stay active for the duration of the current watched period.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/store/src/atom.ts (1)
258-273:⚠️ Potential issue | 🟠 MajorStopping an active
whileWatchedlistener still leaves its cleanup running.When
stop()is called after Line 263 has registered a cleanup, Lines 269-271 remove the listener from_watchedSubsbut never remove or invoke the matching entry in_watchedCleanups. The external resource stays live until fullunwatched(), and the cleanup array can drift out of index alignment with the listener array.Proposed fix
whileWatched(listener: WatchedEffect): () => void { atom._watchedSubs ??= [] atom._watchedSubs.push(listener) if (atom._watched) { atom._watchedCleanups ??= [] atom._watchedCleanups.push(listener()) } return () => { if (!atom._watchedSubs) { return } const index = atom._watchedSubs.indexOf(listener) if (index !== -1) { atom._watchedSubs.splice(index, 1) + if (atom._watched && atom._watchedCleanups) { + const [cleanup] = atom._watchedCleanups.splice(index, 1) + cleanup?.() + } } } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/store/src/atom.ts` around lines 258 - 273, The unsubscribe returned by whileWatched currently removes the listener from atom._watchedSubs but leaves the corresponding entry in atom._watchedCleanups, causing the cleanup to keep running and arrays to drift; update the returned function in whileWatched to locate the listener index, remove the same index from atom._watchedCleanups (if present), and invoke the stored cleanup function (or safely skip if undefined) so the external resource is released immediately; ensure this logic references atom._watchedSubs, atom._watchedCleanups, and the listener returned by whileWatched and preserves behavior when atom._watched is false or when unwatched() later runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/store/src/atom.ts`:
- Around line 195-203: The computed atom is calling trigger.get() inside its
read which creates a persistent dependency (trigger -> atom) and keeps
subscribe(invalidate) active; change the implementation so the computed read
does not perform a subscribing get on trigger. Replace the subscribing read
(trigger.get()) with a non-subscribing peek/read (or an equivalent API) so the
computed returns getSnapshot() without creating the dependency, or move the
subscribe(invalidate) logic into the returned atom's whileWatched handler (use
trigger.whileWatched / the atom's whileWatched) so subscription to the external
store is only established when the returned atom itself is observed; reference
trigger, invalidate, trigger.whileWatched, createAtom, and getSnapshot when
making the change.
---
Duplicate comments:
In `@packages/store/src/atom.ts`:
- Around line 258-273: The unsubscribe returned by whileWatched currently
removes the listener from atom._watchedSubs but leaves the corresponding entry
in atom._watchedCleanups, causing the cleanup to keep running and arrays to
drift; update the returned function in whileWatched to locate the listener
index, remove the same index from atom._watchedCleanups (if present), and invoke
the stored cleanup function (or safely skip if undefined) so the external
resource is released immediately; ensure this logic references
atom._watchedSubs, atom._watchedCleanups, and the listener returned by
whileWatched and preserves behavior when atom._watched is false or when
unwatched() later runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 205a6861-ba8c-4c99-a55a-3827fb328d5b
📒 Files selected for processing (2)
packages/store/src/atom.tspackages/store/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/store/src/types.ts
Comment on lines
+195
to
+203
| const trigger = createAtom(0) | ||
| const invalidate = () => trigger.set((n) => n + 1) | ||
| trigger.whileWatched(() => subscribe(invalidate)) | ||
|
|
||
| return createAtom(() => { | ||
| // Return latest snapshot when `trigger` changes | ||
| trigger.get() | ||
| return getSnapshot() | ||
| }, options) |
There was a problem hiding this comment.
Plain get() can keep the external store subscribed indefinitely.
Line 201 tracks trigger from inside a computed atom. Because computed reads run with activeSub = atom, a one-off atom.get() still creates a trigger -> atom dependency, and packages/store/src/alien.ts calls watched(dep) on that first link. Since that link is only removed when the dependency graph is later purged, Line 197 can leave subscribe(invalidate) active even when the external atom has no subscribers. That breaks the “subscribe only while observed” contract.
Consider avoiding trigger.get() for unobserved reads, or backing this with a mutable atom that reads getSnapshot() on demand and only subscribes via atom.whileWatched.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/store/src/atom.ts` around lines 195 - 203, The computed atom is
calling trigger.get() inside its read which creates a persistent dependency
(trigger -> atom) and keeps subscribe(invalidate) active; change the
implementation so the computed read does not perform a subscribing get on
trigger. Replace the subscribing read (trigger.get()) with a non-subscribing
peek/read (or an equivalent API) so the computed returns getSnapshot() without
creating the dependency, or move the subscribe(invalidate) logic into the
returned atom's whileWatched handler (use trigger.whileWatched / the atom's
whileWatched) so subscription to the external store is only established when the
returned atom itself is observed; reference trigger, invalidate,
trigger.whileWatched, createAtom, and getSnapshot when making the change.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/store/src/atom.ts (1)
196-203:⚠️ Potential issue | 🟠 MajorAvoid subscribing the external store on plain
get().Line 202 still creates the
trigger -> atomdependency during computed evaluation. A one-offatom.get()can therefore marktriggeras watched and keepsubscribe(invalidate)active even though the returned atom has no subscribers, which breaks the “only while watched” contract.Move the subscription to the returned atom
export function createExternalStoreAtom<T>( getSnapshot: () => T, subscribe: (onStoreChange: () => void) => () => void, options?: AtomOptions<T>, ): ReadonlyAtom<T> { const trigger = createAtom(0) const invalidate = () => trigger.set((n) => n + 1) - trigger.whileWatched(() => subscribe(invalidate)) - - return createAtom(() => { + const atom = createAtom(() => { // Return latest snapshot when `trigger` changes trigger.get() return getSnapshot() }, options) + atom.whileWatched(() => subscribe(invalidate)) + return atom }
🧹 Nitpick comments (1)
packages/store/src/atom.ts (1)
41-47: Clarify thatstop()only affects future watch cycles.“Returns a
stopfunction which cancels the listener” reads like immediate teardown, but the implementation intentionally keeps the current cycle active and only prevents future re-registration. Tightening the wording here would make the public contract much clearer.Based on learnings: In
packages/store/src/atom.ts,stop()is intentionally limited to removing the listener from future watch cycles and must not fire or remove the current cycle’s cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/store/src/atom.ts` around lines 41 - 47, The docstring for whileWatched (type: (effect: WatchedEffect) => () => void) is misleading: change the return description to state that the returned stop() only prevents the provided effect from being re-registered in future watch cycles and does not invoke the current cycle’s cleanup or remove the active listener immediately; reference the whileWatched signature and WatchedEffect type, and explicitly note that existing active effect cleanup will still run only when the current watch cycle ends.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/store/src/atom.ts`:
- Around line 259-273: The stop closure removes the first matching listener via
indexOf, which breaks when the same listener was registered multiple times;
change the registration to capture the cleanup return and have the stopper
remove the most recently added matching entries using lastIndexOf: when pushing
to atom._watchedSubs, capture the pushed listener (or its index) and when you
push the cleanup (listener()) capture that cleanup value into a local (e.g.,
cleanupRef), then in the returned stop() use
atom._watchedSubs.lastIndexOf(listener) to find and splice the correct (most
recent) subscriber and use atom._watchedCleanups.lastIndexOf(cleanupRef) to
remove the corresponding cleanup entry. Ensure you only attempt to remove when
the found index !== -1.
- Around line 66-79: The watched/unwatched lifecycle must be made
exception-safe: in watched(InternalAtom) initialize atom._watchedCleanups = []
before running any setup and then iterate atom._watchedSubs (not using map)
calling each setup in a try block and pushing its returned cleanup (if any) into
atom._watchedCleanups; if a setup throws, ensure atom._watchedCleanups remains
set with already-collected cleanup functions and rethrow the error. In
unwatched(InternalAtom) iterate atom._watchedCleanups and invoke each cleanup
inside its own try/catch so one throwing cleanup does not prevent others from
running, then always clear atom._watchedCleanups.length = 0 and set
atom._watched = false.
---
Nitpick comments:
In `@packages/store/src/atom.ts`:
- Around line 41-47: The docstring for whileWatched (type: (effect:
WatchedEffect) => () => void) is misleading: change the return description to
state that the returned stop() only prevents the provided effect from being
re-registered in future watch cycles and does not invoke the current cycle’s
cleanup or remove the active listener immediately; reference the whileWatched
signature and WatchedEffect type, and explicitly note that existing active
effect cleanup will still run only when the current watch cycle ends.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7e509ae-343a-43a4-812a-b08512fff464
📒 Files selected for processing (1)
packages/store/src/atom.ts
Comment on lines
+66
to
+79
| watched(atom: InternalAtom<any>): void { | ||
| atom._watched = true | ||
| if (atom._watchedSubs?.length) { | ||
| atom._watchedCleanups = atom._watchedSubs.map((sub) => sub()) | ||
| } | ||
| }, | ||
| unwatched(atom: InternalAtom<any>): void { | ||
| atom._watched = false | ||
| // not sure if this should go above or below the deps purge | ||
| // preorder vs postorder | ||
| if (atom._watchedCleanups?.length) { | ||
| atom._watchedCleanups.forEach((cleanup) => cleanup?.()) | ||
| atom._watchedCleanups.length = 0 | ||
| } |
There was a problem hiding this comment.
Make watched lifecycle callbacks exception-safe.
Line 69 assigns _watchedCleanups only after every setup succeeds, so one throwing setup loses the cleanups returned by earlier setups. Line 77 also stops on the first throwing cleanup, which skips later teardowns and can leave external subscriptions/resources behind.
Suggested hardening
watched(atom: InternalAtom<any>): void {
atom._watched = true
if (atom._watchedSubs?.length) {
- atom._watchedCleanups = atom._watchedSubs.map((sub) => sub())
+ const cleanups: Array<(() => void) | void | undefined> = []
+ try {
+ for (const sub of atom._watchedSubs) {
+ cleanups.push(sub())
+ }
+ atom._watchedCleanups = cleanups
+ } catch (error) {
+ atom._watched = false
+ for (const cleanup of cleanups) {
+ cleanup?.()
+ }
+ throw error
+ }
}
},
unwatched(atom: InternalAtom<any>): void {
atom._watched = false
- if (atom._watchedCleanups?.length) {
- atom._watchedCleanups.forEach((cleanup) => cleanup?.())
- atom._watchedCleanups.length = 0
+ const cleanups = atom._watchedCleanups
+ atom._watchedCleanups = []
+ let firstError: unknown
+ if (cleanups?.length) {
+ for (const cleanup of cleanups) {
+ try {
+ cleanup?.()
+ } catch (error) {
+ firstError ??= error
+ }
+ }
}
if (atom.depsTail !== undefined) {
atom.depsTail = undefined
atom.flags = ReactiveFlags.Mutable | ReactiveFlags.Dirty
purgeDeps(atom)
}
+ if (firstError) {
+ throw firstError
+ }
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/store/src/atom.ts` around lines 66 - 79, The watched/unwatched
lifecycle must be made exception-safe: in watched(InternalAtom) initialize
atom._watchedCleanups = [] before running any setup and then iterate
atom._watchedSubs (not using map) calling each setup in a try block and pushing
its returned cleanup (if any) into atom._watchedCleanups; if a setup throws,
ensure atom._watchedCleanups remains set with already-collected cleanup
functions and rethrow the error. In unwatched(InternalAtom) iterate
atom._watchedCleanups and invoke each cleanup inside its own try/catch so one
throwing cleanup does not prevent others from running, then always clear
atom._watchedCleanups.length = 0 and set atom._watched = false.
Comment on lines
+259
to
+273
| whileWatched(listener: WatchedEffect): () => void { | ||
| atom._watchedSubs ??= [] | ||
| atom._watchedSubs.push(listener) | ||
| if (atom._watched) { | ||
| atom._watchedCleanups ??= [] | ||
| atom._watchedCleanups.push(listener()) | ||
| } | ||
| return () => { | ||
| if (!atom._watchedSubs) { | ||
| return | ||
| } | ||
| const index = atom._watchedSubs.indexOf(listener) | ||
| if (index !== -1) { | ||
| atom._watchedSubs.splice(index, 1) | ||
| } |
There was a problem hiding this comment.
stop() removes the wrong entry for duplicate registrations.
If the same function reference is passed to whileWatched() more than once, both returned stop closures use indexOf(listener) and target the first match. Calling the second stopper first removes the earlier registration instead of its own one, so future watch cycles can keep one listener active unexpectedly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/store/src/atom.ts` around lines 259 - 273, The stop closure removes
the first matching listener via indexOf, which breaks when the same listener was
registered multiple times; change the registration to capture the cleanup return
and have the stopper remove the most recently added matching entries using
lastIndexOf: when pushing to atom._watchedSubs, capture the pushed listener (or
its index) and when you push the cleanup (listener()) capture that cleanup value
into a local (e.g., cleanupRef), then in the returned stop() use
atom._watchedSubs.lastIndexOf(listener) to find and splice the correct (most
recent) subscriber and use atom._watchedCleanups.lastIndexOf(cleanupRef) to
remove the corresponding cleanup entry. Ensure you only attempt to remove when
the found index !== -1.
|
View your CI Pipeline Execution ↗ for commit 768b8b5
☁️ Nx Cloud last updated this comment at |
@tanstack/angular-store
@tanstack/preact-store
@tanstack/react-store
@tanstack/solid-store
@tanstack/store
@tanstack/svelte-store
@tanstack/vue-store
commit: |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
docs/reference/classes/ReadonlyStore.md (2)
136-137: Update terminology to match the documented entity.The description references "atom" but this is
ReadonlyStoredocumentation. Consider updating to "while the store is watched" for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/classes/ReadonlyStore.md` around lines 136 - 137, The docs mention "atom" in the ReadonlyStore API description; update the wording to refer to the documented entity by changing phrases like "while the atom is watched" and "when the atom is unwatched" to "while the store is watched" and "when the store is unwatched" (refer to ReadonlyStore and the effect callback) so the terminology matches the ReadonlyStore documentation.
136-156: Consider using the more detailed description from BaseAtom docs.The Atom/BaseAtom documentation provides a more precise description that clarifies when the effect runs ("when the first subscriber is added") and includes a helpful example. For consistency and completeness, consider:
- Using the detailed description: "effect will be called when the first subscriber is added. effect may return a cleanup function, which will be called when the store is unwatched."
- Adding the
createTickerexample (adapted for stores if appropriate)- Using consistent terminology ("cleanup function" instead of "stop function")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/classes/ReadonlyStore.md` around lines 136 - 156, Update the ReadonlyStore docs to match BaseAtom wording: change the effect description to "effect will be called when the first subscriber is added. effect may return a cleanup function, which will be called when the store is unwatched." Replace the "stop function" wording with "cleanup function" everywhere in this doc and ensure the parameter type reference still points to WatchedEffect; additionally add the adapted createTicker example (from BaseAtom) illustrating effect usage for stores and showing returning a cleanup function, and keep the Returns section consistent with the new terminology for the returned cleanup function.docs/reference/classes/Store.md (2)
170-171: Update terminology to match the documented entity.The description references "atom" but this is
Storedocumentation. Consider updating to "while the store is watched" for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/classes/Store.md` around lines 170 - 171, Summary: The docs use "atom" where they should reference "Store". Update the wording to match the documented entity by changing references from "atom" to "store" in the sentence describing effect. Specifically, in the Store documentation entry that mentions `effect`, replace phrases like "while the atom is watched" and "when the atom is unwatched" with "while the store is watched" and "when the store is unwatched" so the terminology matches the `Store` concept and the `effect` description.
170-189: Consider using the more detailed description from BaseAtom docs.The Atom/BaseAtom documentation provides a more precise description that clarifies when the effect runs ("when the first subscriber is added") and includes a helpful example. For consistency and completeness, consider:
- Using the detailed description: "effect will be called when the first subscriber is added. effect may return a cleanup function, which will be called when the store is unwatched."
- Adding the
createTickerexample (adapted for stores if appropriate)- Using consistent terminology ("cleanup function" instead of "stop function")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/classes/Store.md` around lines 170 - 189, Update the documentation for the Store watch/effect section to match BaseAtom's more precise wording: say that `effect` (type `WatchedEffect`) "will be called when the first subscriber is added" and that `effect` may return a "cleanup function" which is called when the store is unwatched; replace the ambiguous "stop" wording with "cleanup function" everywhere in this section; also add the adapted `createTicker` example (or a short store-specific example) showing subscription start/cleanup to demonstrate lifecycle semantics. Ensure references to `effect`, `WatchedEffect`, and the example function name (e.g., `createTicker`) are included so reviewers can locate and verify the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/reference/classes/ReadonlyStore.md`:
- Around line 136-137: The docs mention "atom" in the ReadonlyStore API
description; update the wording to refer to the documented entity by changing
phrases like "while the atom is watched" and "when the atom is unwatched" to
"while the store is watched" and "when the store is unwatched" (refer to
ReadonlyStore and the effect callback) so the terminology matches the
ReadonlyStore documentation.
- Around line 136-156: Update the ReadonlyStore docs to match BaseAtom wording:
change the effect description to "effect will be called when the first
subscriber is added. effect may return a cleanup function, which will be called
when the store is unwatched." Replace the "stop function" wording with "cleanup
function" everywhere in this doc and ensure the parameter type reference still
points to WatchedEffect; additionally add the adapted createTicker example (from
BaseAtom) illustrating effect usage for stores and showing returning a cleanup
function, and keep the Returns section consistent with the new terminology for
the returned cleanup function.
In `@docs/reference/classes/Store.md`:
- Around line 170-171: Summary: The docs use "atom" where they should reference
"Store". Update the wording to match the documented entity by changing
references from "atom" to "store" in the sentence describing effect.
Specifically, in the Store documentation entry that mentions `effect`, replace
phrases like "while the atom is watched" and "when the atom is unwatched" with
"while the store is watched" and "when the store is unwatched" so the
terminology matches the `Store` concept and the `effect` description.
- Around line 170-189: Update the documentation for the Store watch/effect
section to match BaseAtom's more precise wording: say that `effect` (type
`WatchedEffect`) "will be called when the first subscriber is added" and that
`effect` may return a "cleanup function" which is called when the store is
unwatched; replace the ambiguous "stop" wording with "cleanup function"
everywhere in this section; also add the adapted `createTicker` example (or a
short store-specific example) showing subscription start/cleanup to demonstrate
lifecycle semantics. Ensure references to `effect`, `WatchedEffect`, and the
example function name (e.g., `createTicker`) are included so reviewers can
locate and verify the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2809a279-054d-4237-8a98-2fc756a7f36e
📒 Files selected for processing (26)
docs/reference/classes/ReadonlyStore.mddocs/reference/classes/Store.mddocs/reference/functions/batch.mddocs/reference/functions/createAsyncAtom.mddocs/reference/functions/createAtom.mddocs/reference/functions/createExternalStoreAtom.mddocs/reference/functions/createStore.mddocs/reference/functions/flush.mddocs/reference/index.mddocs/reference/interfaces/Atom.mddocs/reference/interfaces/AtomOptions.mddocs/reference/interfaces/BaseAtom.mddocs/reference/interfaces/InternalBaseAtom.mddocs/reference/interfaces/InternalReadonlyAtom.mddocs/reference/interfaces/InteropSubscribable.mddocs/reference/interfaces/Readable.mddocs/reference/interfaces/ReadonlyAtom.mddocs/reference/interfaces/Subscribable.mddocs/reference/interfaces/Subscription.mddocs/reference/type-aliases/AnyAtom.mddocs/reference/type-aliases/Observer.mddocs/reference/type-aliases/Selection.mddocs/reference/type-aliases/StoreAction.mddocs/reference/type-aliases/StoreActionMap.mddocs/reference/type-aliases/StoreActionsFactory.mddocs/reference/type-aliases/WatchedEffect.md
✅ Files skipped from review due to trivial changes (21)
- docs/reference/type-aliases/Selection.md
- docs/reference/functions/batch.md
- docs/reference/type-aliases/StoreActionMap.md
- docs/reference/type-aliases/StoreAction.md
- docs/reference/interfaces/AtomOptions.md
- docs/reference/type-aliases/StoreActionsFactory.md
- docs/reference/interfaces/Subscription.md
- docs/reference/interfaces/InteropSubscribable.md
- docs/reference/functions/createAtom.md
- docs/reference/type-aliases/WatchedEffect.md
- docs/reference/interfaces/InternalBaseAtom.md
- docs/reference/type-aliases/Observer.md
- docs/reference/interfaces/InternalReadonlyAtom.md
- docs/reference/index.md
- docs/reference/functions/createExternalStoreAtom.md
- docs/reference/functions/createStore.md
- docs/reference/interfaces/Subscribable.md
- docs/reference/functions/flush.md
- docs/reference/functions/createAsyncAtom.md
- docs/reference/type-aliases/AnyAtom.md
- docs/reference/interfaces/Readable.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Changes
Add
atom.whileWatched(cb),store.whileWatched(cb), andcreateExternaStoreAtom.atom.whileWatchedallows running asetupfunction when the atom gains its first subscriber, and optionally running thecleanupfunction returned bysetupwhen the atom loses its last subscriber.This gives us a
useLayoutEffect-like primitive that we buildcreateExternalStoreAtomon top of.Example use case of these features is to implement a
createQueryAtomfunction to pull @tanstack/query-core state into the @tanstack/store reactivity graph. We use this primitive at Notion Labs Inc for this purpose.You can also use this primitive to build atoms that automatically sync their state across tabs via
localStorage, etc✅ Checklist
pnpm test:pr.🚀 Release Impact
Summary by CodeRabbit
New Features
Documentation
Tests