refactor(store): improve tree-shaking of alien-signals implementation#297
Merged
KevinVandy merged 2 commits intoTanStack:mainfrom Mar 25, 2026
Merged
Conversation
📝 WalkthroughWalkthroughReorganizes reactive system implementations from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
|
View your CI Pipeline Execution ↗ for commit 4a10f8f
☁️ 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.
Actionable comments posted: 3
🤖 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/signal.ts`:
- Around line 147-167: The effect() function links the new EffectNode into the
reactive graph before running e.fn(), so if e.fn() throws the caller never gets
the disposer and the half-constructed node remains subscribed; change the setup
so that any partial construction is cleaned up on throw: wrap the e.fn()
invocation in try/catch, and in the catch remove the node from its
parent/subscription (undo the link call referencing setActiveSub/link and the
prevSub variable), remove any deps/subs added to e (clear
deps/depsTail/subs/subsTail and clear flags like ReactiveFlags.Watching),
restore activeSub, then rethrow the error so the caller sees it; ensure
effectOper.bind(e) is only returned for fully-initialized nodes (or still
returned after cleanup if you want the same contract) and reference effect,
setActiveSub, link, effectOper, and ReactiveFlags in your changes so the undo
logic targets the correct symbols.
- Around line 92-95: endBatch currently decrements the shared batchDepth
unguarded which allows it to become negative; change endBatch so it only
decrements when batchDepth > 0 (or capture the previous value and clamp to
zero), and only call flush when batchDepth transitions from 1 to 0; use the
existing symbols batchDepth and flush (and function endBatch) to implement this
guard, and optionally log or noop on extra calls instead of letting batchDepth
go negative.
- Around line 223-224: Replace strict inequality checks that currently use !==
with Object.is to correctly compare previous and new signal values (handling NaN
and -0). Specifically, in updateComputed() change the comparison that returns
oldValue !== (c.value = c.getter(oldValue)) to use Object.is(oldValue, newValue)
logic (compute newValue from c.getter(oldValue) then use !Object.is(oldValue,
newValue) for invalidation); apply the same change in updateSignal() where the
code compares previous and assigned values, and in signalOper() at the
comparison around signal value assignment—compute the new value into a
temporary, then use Object.is to decide whether to invalidate/update. Ensure you
reference the same variables c.value, c.getter(oldValue), and the signal
value/temp newValue when making these replacements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57513166-d3b1-4f6a-accf-96b12c16578e
📒 Files selected for processing (11)
.changeset/curvy-apes-matter.mddocs/reference/functions/batch.mddocs/reference/functions/createAsyncAtom.mddocs/reference/functions/createAtom.mddocs/reference/functions/flush.mdknip.jsonpackages/store/src/alien.tspackages/store/src/atom.tspackages/store/src/batch.tspackages/store/src/index.tspackages/store/src/signal.ts
💤 Files with no reviewable changes (2)
- packages/store/src/index.ts
- packages/store/src/batch.ts
Comment on lines
+92
to
+95
| export function endBatch() { | ||
| if (!--batchDepth) { | ||
| flush() | ||
| } |
There was a problem hiding this comment.
Prevent endBatch() from corrupting the scheduler state.
An extra endBatch() call pushes the shared batchDepth negative here, and from that point later writes stop flushing on the right boundary because the module never gets back to the expected 0 → flush transition.
🐛 Proposed fix
export function endBatch() {
+ if (batchDepth <= 0) {
+ throw new Error('endBatch() called without a matching startBatch()')
+ }
if (!--batchDepth) {
flush()
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/store/src/signal.ts` around lines 92 - 95, endBatch currently
decrements the shared batchDepth unguarded which allows it to become negative;
change endBatch so it only decrements when batchDepth > 0 (or capture the
previous value and clamp to zero), and only call flush when batchDepth
transitions from 1 to 0; use the existing symbols batchDepth and flush (and
function endBatch) to implement this guard, and optionally log or noop on extra
calls instead of letting batchDepth go negative.
Comment on lines
+147
to
+167
| export function effect(fn: () => void): () => void { | ||
| const e: EffectNode = { | ||
| fn, | ||
| subs: undefined, | ||
| subsTail: undefined, | ||
| deps: undefined, | ||
| depsTail: undefined, | ||
| flags: ReactiveFlags.Watching | ReactiveFlags.RecursedCheck, | ||
| } | ||
| const prevSub = setActiveSub(e) | ||
| if (prevSub !== undefined) { | ||
| link(e, prevSub, 0) | ||
| } | ||
| try { | ||
| e.fn() | ||
| } finally { | ||
| activeSub = prevSub | ||
| e.flags &= ~ReactiveFlags.RecursedCheck | ||
| } | ||
| return effectOper.bind(e) | ||
| } |
There was a problem hiding this comment.
Dispose partially constructed effects/scopes when setup throws.
Both factories link the new node into the graph before running user code. If fn() throws during that first run, the caller never receives the disposer, but the half-built node stays subscribed through any deps/parent links created before the throw.
🐛 Proposed fix
export function effect(fn: () => void): () => void {
const e: EffectNode = {
fn,
subs: undefined,
subsTail: undefined,
deps: undefined,
depsTail: undefined,
flags: ReactiveFlags.Watching | ReactiveFlags.RecursedCheck,
}
const prevSub = setActiveSub(e)
if (prevSub !== undefined) {
link(e, prevSub, 0)
}
+ let didThrow = true
try {
e.fn()
+ didThrow = false
} finally {
activeSub = prevSub
e.flags &= ~ReactiveFlags.RecursedCheck
+ if (didThrow) {
+ effectScopeOper.call(e)
+ }
}
return effectOper.bind(e)
}
export function effectScope(fn: () => void): () => void {
const e: ReactiveNode = {
deps: undefined,
depsTail: undefined,
subs: undefined,
subsTail: undefined,
flags: ReactiveFlags.None,
}
const prevSub = setActiveSub(e)
if (prevSub !== undefined) {
link(e, prevSub, 0)
}
+ let didThrow = true
try {
fn()
+ didThrow = false
} finally {
activeSub = prevSub
+ if (didThrow) {
+ effectScopeOper.call(e)
+ }
}
return effectScopeOper.bind(e)
}Also applies to: 169-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/store/src/signal.ts` around lines 147 - 167, The effect() function
links the new EffectNode into the reactive graph before running e.fn(), so if
e.fn() throws the caller never gets the disposer and the half-constructed node
remains subscribed; change the setup so that any partial construction is cleaned
up on throw: wrap the e.fn() invocation in try/catch, and in the catch remove
the node from its parent/subscription (undo the link call referencing
setActiveSub/link and the prevSub variable), remove any deps/subs added to e
(clear deps/depsTail/subs/subsTail and clear flags like ReactiveFlags.Watching),
restore activeSub, then rethrow the error so the caller sees it; ensure
effectOper.bind(e) is only returned for fully-initialized nodes (or still
returned after cleanup if you want the same contract) and reference effect,
setActiveSub, link, effectOper, and ReactiveFlags in your changes so the undo
logic targets the correct symbols.
Comment on lines
+223
to
+224
| const oldValue = c.value | ||
| return oldValue !== (c.value = c.getter(oldValue)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/store/src/signal.ts | sed -n '220,240p'Repository: TanStack/store
Length of output: 749
🏁 Script executed:
cat -n packages/store/src/signal.ts | sed -n '305,315p'Repository: TanStack/store
Length of output: 490
🏁 Script executed:
wc -l packages/store/src/signal.tsRepository: TanStack/store
Length of output: 90
🏁 Script executed:
rg "Object\.is" packages/store/src/ -A 2 -B 2Repository: TanStack/store
Length of output: 385
🏁 Script executed:
rg "NaN" packages/store/src/signal.tsRepository: TanStack/store
Length of output: 40
🏁 Script executed:
# Search for any tests related to cache invalidation with NaN values
rg "NaN" packages/store --type-list | head -20Repository: TanStack/store
Length of output: 447
🏁 Script executed:
# Search for tests related to NaN or signals
find packages/store -name "*.test.*" -o -name "*.spec.*" | head -10Repository: TanStack/store
Length of output: 247
🏁 Script executed:
# Check what the compare function defaults to in the context
cat -n packages/store/src/atom.ts | sed -n '1,50p'Repository: TanStack/store
Length of output: 1912
🏁 Script executed:
# Look at the broader context of the three locations mentioned
cat -n packages/store/src/signal.ts | sed -n '218,240p'
cat -n packages/store/src/signal.ts | sed -n '230,240p'
cat -n packages/store/src/signal.ts | sed -n '305,320p'Repository: TanStack/store
Length of output: 1648
🏁 Script executed:
# Check if there's a custom compare function already used elsewhere
rg "compare" packages/store/src/signal.ts -B 2 -A 2Repository: TanStack/store
Length of output: 40
🏁 Script executed:
# Look for the complete function signatures to understand context better
cat -n packages/store/src/signal.ts | sed -n '210,230p'Repository: TanStack/store
Length of output: 642
🏁 Script executed:
# Check if there are any TypeScript type annotations that might restrict values
rg "SignalNode|ComputedNode" packages/store/src/signal.ts -A 5 | head -50Repository: TanStack/store
Length of output: 1489
🏁 Script executed:
# Search test files for NaN-related tests
grep -r "NaN" packages/store/tests/Repository: TanStack/store
Length of output: 40
🏁 Script executed:
# Look at how atom.ts implements the compare function
cat -n packages/store/src/atom.ts | sed -n '50,120p'Repository: TanStack/store
Length of output: 2341
🏁 Script executed:
# Check if there's any documentation or comments about equality comparisons
rg "equality|compare|SameValue|Object.is" packages/store/src/ -B 1 -A 1Repository: TanStack/store
Length of output: 627
🏁 Script executed:
# Look for any existing issues or patterns around NaN handling
git log --oneline --all -S "NaN" packages/store/ 2>/dev/null | head -5Repository: TanStack/store
Length of output: 40
Replace !== with Object.is for cache invalidation to handle NaN and -0 correctly.
The !== comparisons treat NaN as changed every time (since NaN !== NaN is true), causing unnecessary downstream invalidation. Object.is correctly treats NaN values as equal and handles -0 properly, aligning with the approach already used by default in atom.ts.
Apply to:
- Line 224 in
updateComputed() - Line 234 in
updateSignal() - Line 310 in
signalOper()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/store/src/signal.ts` around lines 223 - 224, Replace strict
inequality checks that currently use !== with Object.is to correctly compare
previous and new signal values (handling NaN and -0). Specifically, in
updateComputed() change the comparison that returns oldValue !== (c.value =
c.getter(oldValue)) to use Object.is(oldValue, newValue) logic (compute newValue
from c.getter(oldValue) then use !Object.is(oldValue, newValue) for
invalidation); apply the same change in updateSignal() where the code compares
previous and assigned values, and in signalOper() at the comparison around
signal value assignment—compute the new value into a temporary, then use
Object.is to decide whether to invalidate/update. Ensure you reference the same
variables c.value, c.getter(oldValue), and the signal value/temp newValue when
making these replacements.
davidkpiano
approved these changes
Mar 25, 2026
KevinVandy
approved these changes
Mar 25, 2026
Merged
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
3 participants
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
Ensure nothing from the
signalsimplementation is used by theatomsimplementationflush()method ofsignalsanymoresignalsto their own file. This file is not exported from the lib, and ignored byknip.In Router, these changes translate into bundle-size savings
✅ Checklist
pnpm test:pr.🚀 Release Impact
Summary by CodeRabbit