fix(reactivity): should not recompute if computed does not track reactive data #12341
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
|
Maybe this modification will be simpler export function triggerRef(ref: Ref): void {
// ref may be an instance of ObjectRefImpl
if ((ref as unknown as RefImpl).dep) {
// add
if ((ref as unknown as RefImpl).dep.computed) {
refreshComputed((ref as unknown as RefImpl).dep.computed!)
}
...
}
} |
edison1105
changed the title
edison1105
added
ready to merge
edison1105
added
the
🔨 p3-minor-bug
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Computed
participant ReactiveRef
User->>Computed: Access computed.value
Computed->>Computed: Evaluate getter (no reactive deps)
User->>ReactiveRef: Change ref.value
User->>Computed: Access computed.value again
Computed-->>User: Return cached value (no recomputation)
Assessment against linked issues
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/reactivity/src/effect.ts (2)
41-53: Add succinct JSDoc for the newEVALUATEDflagThe semantics of the new bit are only implied by later usage. A short inline comment (similar to the other flags) will make its purpose self-explanatory to future maintainers.
PAUSED = 1 << 6, + /** + * Computed has executed at least once and produced a value. + * Used to skip redundant evaluations when it tracks no deps. + */ EVALUATED = 1 << 7,
385-394: Early-return condition is correct but hard to read – consider extracting helpersThe triple-negated expression:
(!computed.deps && !(computed as any)._dirty) || !isDirty(computed)is cognitively heavy and mixes two separate ideas (“no deps” vs “deps unchanged”).
Creating small predicates such ashasNoDeps(computed)anddepsUnchanged(computed)(or at least splitting the check across twoifblocks) would improve clarity without affecting perf.packages/reactivity/__tests__/computed.spec.ts (1)
1015-1024:asynckeyword is unnecessary in this testThe test body contains no
await; marking itasynconly masks accidental un-awaited promises and slightly slows the runner.-test('should not recompute if computed does not track reactive data', async () => { +test('should not recompute if computed does not track reactive data', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/reactivity/__tests__/computed.spec.ts(1 hunks)packages/reactivity/src/effect.ts(3 hunks)
🔇 Additional comments (1)
packages/reactivity/src/effect.ts (1)
394-418:RUNNINGflag now set after the early return – verify re-entrancy in edge casesMoving
computed.flags |= RUNNINGbelow the new guard avoids polluting the stack for short-circuited calls, 👍.
However, if a getter with side-effects clears its ownDIRTYflag insideprepareDeps, there’s a theoretical chance of re-entrancy beforeRUNNINGis set (because external code could synchronously access the same computed in the getter).That situation is rare but not impossible in user-space (e.g. self-mutating computeds).
Consider adding a regression test or, at minimum, a comment explaining why re-entrancy beforeRUNNINGis impossible with the current control flow.
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
revert #11931 re-fix #11929
close #12337
Summary by CodeRabbit
New Features
Tests