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.

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

fix(reactivity): no longer allows stop computed effect #10290

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 0 additions & 21 deletions packages/reactivity/__tests__/computed.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,21 +123,6 @@ describe('reactivity/computed', () => {
expect(getter2).toHaveBeenCalledTimes(2)
})

it('should no longer update when stopped', () => {
const value = reactive<{ foo?: number }>({})
const cValue = computed(() => value.foo)
let dummy
effect(() => {
dummy = cValue.value
})
expect(dummy).toBe(undefined)
value.foo = 1
expect(dummy).toBe(1)
cValue.effect.stop()
value.foo = 2
expect(dummy).toBe(1)
})

it('should support setter', () => {
const n = ref(1)
const plusOne = computed({
Expand Down Expand Up @@ -219,12 +204,6 @@ describe('reactivity/computed', () => {
expect(isReadonly(z.value.a)).toBe(false)
})

it('should expose value when stopped', () => {
const x = computed(() => 1)
x.effect.stop()
expect(x.value).toBe(1)
})

it('debug: onTrack', () => {
let events: DebuggerEvent[] = []
const onTrack = vi.fn((e: DebuggerEvent) => {
Expand Down
16 changes: 0 additions & 16 deletions packages/reactivity/__tests__/deferredComputed.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,20 +136,4 @@ describe('deferred computed', () => {
c2.value
expect(effectSpy).toHaveBeenCalledTimes(2)
})

test('should not compute if deactivated before scheduler is called', () => {
const c1Spy = vi.fn()
const src = ref(0)
const c1 = computed(() => {
c1Spy()
return src.value % 2
})
effect(() => c1.value)
expect(c1Spy).toHaveBeenCalledTimes(1)

c1.effect.stop()
// trigger
src.value++
expect(c1Spy).toHaveBeenCalledTimes(1)
})
})
2 changes: 1 addition & 1 deletion packages/reactivity/__tests__/effectScope.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,8 @@ describe('reactivity/effect/scope', () => {
r.value++
c!.value
await nextTick()
expect(computedSpy).toHaveBeenCalledTimes(3)
// should not trigger anymore
expect(computedSpy).toHaveBeenCalledTimes(2)
expect(watchSpy).toHaveBeenCalledTimes(1)
expect(watchEffectSpy).toHaveBeenCalledTimes(2)
})
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 @@ -59,7 +59,7 @@ export class ComputedRefImpl<T> {
),
)
this.effect.computed = this
this.effect.active = this._cacheable = !isSSR
this._cacheable = !isSSR
this[ReactiveFlags.IS_READONLY] = isReadonly
}

Expand Down
22 changes: 15 additions & 7 deletions packages/reactivity/src/effect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ export type DebuggerEventExtraInfo = {
export let activeEffect: ReactiveEffect | undefined

export class ReactiveEffect<T = any> {
active = true
deps: Dep[] = []

/**
Expand All @@ -39,7 +38,6 @@ export class ReactiveEffect<T = any> {
*/
allowRecurse?: boolean

onStop?: () => void
// dev only
onTrack?: (event: DebuggerEvent) => void
// dev only
Expand Down Expand Up @@ -105,9 +103,6 @@ export class ReactiveEffect<T = any> {

run() {
this._dirtyLevel = DirtyLevels.NotDirty
if (!this.active) {
return this.fn()
}
let lastShouldTrack = shouldTrack
let lastEffect = activeEffect
try {
Expand All @@ -123,6 +118,19 @@ export class ReactiveEffect<T = any> {
shouldTrack = lastShouldTrack
}
}
}

export class ReactiveSideEffect<T = any> extends ReactiveEffect<T> {
active = true

onStop?: () => void

run() {
if (!this.active) {
return this.fn()
}
return super.run()
}

stop() {
if (this.active) {
Expand Down Expand Up @@ -177,7 +185,7 @@ export interface ReactiveEffectOptions extends DebuggerOptions {

export interface ReactiveEffectRunner<T = any> {
(): T
effect: ReactiveEffect
effect: ReactiveSideEffect
}

/**
Expand All @@ -198,7 +206,7 @@ export function effect<T = any>(
fn = (fn as ReactiveEffectRunner).effect.fn
}

const _effect = new ReactiveEffect(fn, NOOP, () => {
const _effect = new ReactiveSideEffect(fn, NOOP, () => {
if (_effect.dirty) {
_effect.run()
}
Expand Down
9 changes: 6 additions & 3 deletions packages/reactivity/src/effectScope.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { ReactiveEffect } from './effect'
import type { ReactiveEffect, ReactiveSideEffect } from './effect'
import { warn } from './warning'

let activeEffectScope: EffectScope | undefined
Expand All @@ -11,7 +11,7 @@ export class EffectScope {
/**
* @internal
*/
effects: ReactiveEffect[] = []
effects: (ReactiveEffect | ReactiveSideEffect)[] = []
/**
* @internal
*/
Expand Down Expand Up @@ -82,7 +82,10 @@ export class EffectScope {
if (this._active) {
let i, l
for (i = 0, l = this.effects.length; i < l; i++) {
this.effects[i].stop()
const effect = this.effects[i]
if ('stop' in effect) {
effect.stop()
}
}
for (i = 0, l = this.cleanups.length; i < l; i++) {
this.cleanups[i]()
Expand Down
1 change: 1 addition & 0 deletions packages/reactivity/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ export {
pauseScheduling,
resetScheduling,
ReactiveEffect,
ReactiveSideEffect,
type ReactiveEffectRunner,
type ReactiveEffectOptions,
type EffectScheduler,
Expand Down
9 changes: 7 additions & 2 deletions packages/runtime-core/__tests__/apiSetupHelpers.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {
type ComponentInternalInstance,
type ComputedRef,
type ReactiveSideEffect,
type SetupContext,
Suspense,
computed,
Expand All @@ -13,6 +14,7 @@ import {
render,
serializeInner,
shallowReactive,
watchEffect,
} from '@vue/runtime-test'
import {
createPropsRestProxy,
Expand Down Expand Up @@ -427,6 +429,7 @@ describe('SFC <script setup> helpers', () => {
})

let c: ComputedRef
let effect: ReactiveSideEffect

const Comp = defineComponent({
async setup() {
Expand All @@ -436,6 +439,8 @@ describe('SFC <script setup> helpers', () => {
__restore()

c = computed(() => {})
watchEffect(() => c.value)
effect = [...(c! as any).dep!.keys()][0]
// register the lifecycle after an await statement
onMounted(resolve)
return () => ''
Expand All @@ -447,10 +452,10 @@ describe('SFC <script setup> helpers', () => {
app.mount(root)

await ready
expect(c!.effect.active).toBe(true)
expect(effect!.active).toBe(true)

app.unmount()
expect(c!.effect.active).toBe(false)
expect(effect!.active).toBe(false)
})
})
})
6 changes: 5 additions & 1 deletion packages/runtime-core/__tests__/apiWatch.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1185,7 +1185,11 @@ describe('api: watch', () => {
await nextTick()
await nextTick()

expect(instance!.scope.effects[0].active).toBe(false)
for (const effect of instance!.scope.effects) {
if ('active' in effect) {
expect(effect.active).toBe(false)
}
}
})

test('this.$watch should pass `this.proxy` to watch source as the first argument ', () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/runtime-core/src/apiWatch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import {
type ComputedRef,
type DebuggerOptions,
type EffectScheduler,
ReactiveEffect,
ReactiveFlags,
ReactiveSideEffect,
type Ref,
getCurrentScope,
isReactive,
Expand Down Expand Up @@ -392,7 +392,7 @@ function doWatch(
scheduler = () => queueJob(job)
}

const effect = new ReactiveEffect(getter, NOOP, scheduler)
const effect = new ReactiveSideEffect(getter, NOOP, scheduler)

const scope = getCurrentScope()
const unwatch = () => {
Expand Down
1 change: 1 addition & 0 deletions packages/runtime-core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export {
effect,
stop,
ReactiveEffect,
ReactiveSideEffect,
// effect scope
effectScope,
EffectScope,
Expand Down
Loading