Skip to content

Commit

Permalink
fix: Revert "fix: Revert "fix(reactivity): self-referencing computed …
Browse files Browse the repository at this point in the history
…should refresh""

This reverts commit 35c760f.
  • Loading branch information
yyx990803 committed Sep 10, 2024
1 parent 48613bb commit e596378
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 9 deletions.
7 changes: 4 additions & 3 deletions packages/reactivity/__tests__/computed.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,7 @@ describe('reactivity/computed', () => {

v.value += ' World'
await nextTick()
expect(serializeInner(root)).toBe('Hello World World World')
expect(serializeInner(root)).toBe('Hello World World World World')
// expect(COMPUTED_SIDE_EFFECT_WARN).toHaveBeenWarned()
})

Expand Down Expand Up @@ -892,7 +892,7 @@ describe('reactivity/computed', () => {
v.value += ' World'
await nextTick()
expect(serializeInner(root)).toBe(
'Hello World World World | Hello World World World',
'Hello World World World World | Hello World World World World',
)
})

Expand Down Expand Up @@ -962,6 +962,7 @@ describe('reactivity/computed', () => {
})
})

// #11797
test('should prevent endless recursion in self-referencing computed getters', async () => {
const Comp = defineComponent({
data() {
Expand Down Expand Up @@ -998,7 +999,7 @@ describe('reactivity/computed', () => {
})
const root = nodeOps.createElement('div')
render(h(Comp), root)
expect(serializeInner(root)).toBe(`<button>Step</button><p></p>`)
expect(serializeInner(root)).toBe(`<button>Step</button><p>Step 1</p>`)
triggerEvent(root.children[1] as TestElement, 'click')
await nextTick()
expect(serializeInner(root)).toBe(`<button>Step</button><p>Step 2</p>`)
Expand Down
2 changes: 1 addition & 1 deletion packages/reactivity/src/computed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,9 @@ export class ComputedRefImpl<T = any> implements Subscriber {
* @internal
*/
notify(): void {
this.flags |= EffectFlags.DIRTY
// avoid infinite self recursion
if (activeSub !== this) {
this.flags |= EffectFlags.DIRTY
this.dep.notify()
} else if (__DEV__) {
// TODO warn
Expand Down
7 changes: 2 additions & 5 deletions packages/reactivity/src/effect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ function isDirty(sub: Subscriber): boolean {
for (let link = sub.deps; link; link = link.nextDep) {
if (
link.dep.version !== link.version ||
(link.dep.computed && refreshComputed(link.dep.computed) === false) ||
(link.dep.computed && refreshComputed(link.dep.computed)) ||

This comment has been minimized.

Copy link
@lehni

lehni Sep 12, 2024

Contributor

@yyx990803 this check looks dubious to me. refreshComputed() returns only undefined now, so this condition will never be met, but calling it will have side-effects? Is this the intention here?

This comment has been minimized.

Copy link
@yyx990803

yyx990803 Sep 12, 2024

Author Member

This is intended - in a way. Rewriting this logic would result in three ifs with return statements which is more code, and may not be minified to the same amount of bytes as the code here.

This comment has been minimized.

Copy link
@lehni

lehni Sep 12, 2024

Contributor

Understood! I just wanted to point it out since it looks a bit like a possible source of an error.

link.dep.version !== link.version
) {
return true
Expand All @@ -344,10 +344,7 @@ function isDirty(sub: Subscriber): boolean {
* Returning false indicates the refresh failed
* @internal
*/
export function refreshComputed(computed: ComputedRefImpl): false | undefined {
if (computed.flags & EffectFlags.RUNNING) {
return false
}
export function refreshComputed(computed: ComputedRefImpl): undefined {
if (
computed.flags & EffectFlags.TRACKING &&
!(computed.flags & EffectFlags.DIRTY)
Expand Down

0 comments on commit e596378

Please sign in to comment.