From 9571ede84bb6949e13c25807cc8f016ace29dc8a Mon Sep 17 00:00:00 2001 From: Evan You Date: Mon, 17 Feb 2020 23:14:19 -0500 Subject: [PATCH] refactor(watch): adjsut watch API behavior BREAKING CHANGE: `watch` behavior has been adjusted. - When using the `watch(source, callback, options?)` signature, the callback now fires lazily by default (consistent with 2.x behavior). Note that the `watch(effect, options?)` signature is still eager, since it must invoke the `effect` immediately to collect dependencies. - The `lazy` option has been replaced by the opposite `immediate` option, which defaults to `false`. (It's ignored when using the effect signature) - Due to the above changes, the `watch` option in Options API now behaves exactly the same as 2.x. - When using the effect signature or `{ immediate: true }`, the intital execution is now performed synchronously instead of deferred until the component is mounted. This is necessary for certain use cases to work properly with `async setup()` and Suspense. The side effect of this is the immediate watcher invocation will no longer have access to the mounted DOM. However, the watcher can be initiated inside `onMounted` to retain previous behavior. --- .../runtime-core/__tests__/apiOptions.spec.ts | 40 +++---- .../runtime-core/__tests__/apiWatch.spec.ts | 112 +++++++----------- .../__tests__/components/Suspense.spec.ts | 30 ++++- .../__tests__/errorHandling.spec.ts | 12 +- packages/runtime-core/src/apiWatch.ts | 91 ++++++++------ .../runtime-core/src/components/KeepAlive.ts | 3 +- test-dts/watch.test-d.ts | 31 ++++- 7 files changed, 176 insertions(+), 143 deletions(-) diff --git a/packages/runtime-core/__tests__/apiOptions.spec.ts b/packages/runtime-core/__tests__/apiOptions.spec.ts index 7d915eb0ff2..c55c197e358 100644 --- a/packages/runtime-core/__tests__/apiOptions.spec.ts +++ b/packages/runtime-core/__tests__/apiOptions.spec.ts @@ -149,30 +149,24 @@ describe('api: options', () => { function assertCall(spy: jest.Mock, callIndex: number, args: any[]) { expect(spy.mock.calls[callIndex].slice(0, 2)).toMatchObject(args) + expect(spy).toHaveReturnedWith(ctx) } - assertCall(spyA, 0, [1, undefined]) - assertCall(spyB, 0, [2, undefined]) - assertCall(spyC, 0, [{ qux: 3 }, undefined]) - expect(spyA).toHaveReturnedWith(ctx) - expect(spyB).toHaveReturnedWith(ctx) - expect(spyC).toHaveReturnedWith(ctx) - ctx.foo++ await nextTick() - expect(spyA).toHaveBeenCalledTimes(2) - assertCall(spyA, 1, [2, 1]) + expect(spyA).toHaveBeenCalledTimes(1) + assertCall(spyA, 0, [2, 1]) ctx.bar++ await nextTick() - expect(spyB).toHaveBeenCalledTimes(2) - assertCall(spyB, 1, [3, 2]) + expect(spyB).toHaveBeenCalledTimes(1) + assertCall(spyB, 0, [3, 2]) ctx.baz.qux++ await nextTick() - expect(spyC).toHaveBeenCalledTimes(2) + expect(spyC).toHaveBeenCalledTimes(1) // new and old objects have same identity - assertCall(spyC, 1, [{ qux: 4 }, { qux: 4 }]) + assertCall(spyC, 0, [{ qux: 4 }, { qux: 4 }]) }) test('watch array', async () => { @@ -218,30 +212,24 @@ describe('api: options', () => { function assertCall(spy: jest.Mock, callIndex: number, args: any[]) { expect(spy.mock.calls[callIndex].slice(0, 2)).toMatchObject(args) + expect(spy).toHaveReturnedWith(ctx) } - assertCall(spyA, 0, [1, undefined]) - assertCall(spyB, 0, [2, undefined]) - assertCall(spyC, 0, [{ qux: 3 }, undefined]) - expect(spyA).toHaveReturnedWith(ctx) - expect(spyB).toHaveReturnedWith(ctx) - expect(spyC).toHaveReturnedWith(ctx) - ctx.foo++ await nextTick() - expect(spyA).toHaveBeenCalledTimes(2) - assertCall(spyA, 1, [2, 1]) + expect(spyA).toHaveBeenCalledTimes(1) + assertCall(spyA, 0, [2, 1]) ctx.bar++ await nextTick() - expect(spyB).toHaveBeenCalledTimes(2) - assertCall(spyB, 1, [3, 2]) + expect(spyB).toHaveBeenCalledTimes(1) + assertCall(spyB, 0, [3, 2]) ctx.baz.qux++ await nextTick() - expect(spyC).toHaveBeenCalledTimes(2) + expect(spyC).toHaveBeenCalledTimes(1) // new and old objects have same identity - assertCall(spyC, 1, [{ qux: 4 }, { qux: 4 }]) + assertCall(spyC, 0, [{ qux: 4 }, { qux: 4 }]) }) test('provide/inject', () => { diff --git a/packages/runtime-core/__tests__/apiWatch.spec.ts b/packages/runtime-core/__tests__/apiWatch.spec.ts index 9b1d35a21f0..ef05452fd33 100644 --- a/packages/runtime-core/__tests__/apiWatch.spec.ts +++ b/packages/runtime-core/__tests__/apiWatch.spec.ts @@ -13,13 +13,12 @@ import { mockWarn } from '@vue/shared' describe('api: watch', () => { mockWarn() - it('basic usage', async () => { + it('watch(effect)', async () => { const state = reactive({ count: 0 }) let dummy watch(() => { dummy = state.count }) - await nextTick() expect(dummy).toBe(0) state.count++ @@ -27,33 +26,6 @@ describe('api: watch', () => { expect(dummy).toBe(1) }) - it('triggers when initial value is null', async () => { - const state = ref(null) - const spy = jest.fn() - watch(() => state.value, spy) - await nextTick() - expect(spy).toHaveBeenCalled() - }) - - it('triggers when initial value is undefined', async () => { - const state = ref() - const spy = jest.fn() - watch(() => state.value, spy) - await nextTick() - expect(spy).toHaveBeenCalled() - state.value = 3 - await nextTick() - expect(spy).toHaveBeenCalledTimes(2) - // testing if undefined can trigger the watcher - state.value = undefined - await nextTick() - expect(spy).toHaveBeenCalledTimes(3) - // it shouldn't trigger if the same value is set - state.value = undefined - await nextTick() - expect(spy).toHaveBeenCalledTimes(3) - }) - it('watching single source: getter', async () => { const state = reactive({ count: 0 }) let dummy @@ -68,9 +40,6 @@ describe('api: watch', () => { } } ) - await nextTick() - expect(dummy).toMatchObject([0, undefined]) - state.count++ await nextTick() expect(dummy).toMatchObject([1, 0]) @@ -87,9 +56,6 @@ describe('api: watch', () => { prevCount + 1 } }) - await nextTick() - expect(dummy).toMatchObject([0, undefined]) - count.value++ await nextTick() expect(dummy).toMatchObject([1, 0]) @@ -107,9 +73,6 @@ describe('api: watch', () => { prevCount + 1 } }) - await nextTick() - expect(dummy).toMatchObject([1, undefined]) - count.value++ await nextTick() expect(dummy).toMatchObject([2, 1]) @@ -127,8 +90,6 @@ describe('api: watch', () => { vals.concat(1) oldVals.concat(1) }) - await nextTick() - expect(dummy).toMatchObject([[1, 1, 2], []]) state.count++ count.value++ @@ -149,8 +110,6 @@ describe('api: watch', () => { count + 1 oldStatus === true }) - await nextTick() - expect(dummy).toMatchObject([[1, false], []]) state.count++ status.value = true @@ -164,7 +123,6 @@ describe('api: watch', () => { const stop = watch(() => { dummy = state.count }) - await nextTick() expect(dummy).toBe(0) stop() @@ -174,7 +132,7 @@ describe('api: watch', () => { expect(dummy).toBe(0) }) - it('cleanup registration (basic)', async () => { + it('cleanup registration (effect)', async () => { const state = reactive({ count: 0 }) const cleanup = jest.fn() let dummy @@ -182,7 +140,6 @@ describe('api: watch', () => { onCleanup(cleanup) dummy = state.count }) - await nextTick() expect(dummy).toBe(0) state.count++ @@ -202,22 +159,30 @@ describe('api: watch', () => { onCleanup(cleanup) dummy = count }) + + count.value++ await nextTick() - expect(dummy).toBe(0) + expect(cleanup).toHaveBeenCalledTimes(0) + expect(dummy).toBe(1) count.value++ await nextTick() expect(cleanup).toHaveBeenCalledTimes(1) - expect(dummy).toBe(1) + expect(dummy).toBe(2) stop() expect(cleanup).toHaveBeenCalledTimes(2) }) - it('flush timing: post', async () => { + it('flush timing: post (default)', async () => { const count = ref(0) + let callCount = 0 const assertion = jest.fn(count => { - expect(serializeInner(root)).toBe(`${count}`) + callCount++ + // on mount, the watcher callback should be called before DOM render + // on update, should be called after the count is updated + const expectedDOM = callCount === 1 ? `` : `${count}` + expect(serializeInner(root)).toBe(expectedDOM) }) const Comp = { @@ -230,7 +195,6 @@ describe('api: watch', () => { } const root = nodeOps.createElement('div') render(h(Comp), root) - await nextTick() expect(assertion).toHaveBeenCalledTimes(1) count.value++ @@ -270,7 +234,6 @@ describe('api: watch', () => { } const root = nodeOps.createElement('div') render(h(Comp), root) - await nextTick() expect(assertion).toHaveBeenCalledTimes(1) count.value++ @@ -313,7 +276,6 @@ describe('api: watch', () => { } const root = nodeOps.createElement('div') render(h(Comp), root) - await nextTick() expect(assertion).toHaveBeenCalledTimes(1) count.value++ @@ -346,9 +308,6 @@ describe('api: watch', () => { { deep: true } ) - await nextTick() - expect(dummy).toEqual([0, 1, 1, true]) - state.nested.count++ await nextTick() expect(dummy).toEqual([1, 1, 1, true]) @@ -369,18 +328,42 @@ describe('api: watch', () => { expect(dummy).toEqual([1, 2, 2, false]) }) - it('lazy', async () => { + it('immediate', async () => { const count = ref(0) const cb = jest.fn() - watch(count, cb, { lazy: true }) - await nextTick() - expect(cb).not.toHaveBeenCalled() + watch(count, cb, { immediate: true }) + expect(cb).toHaveBeenCalledTimes(1) count.value++ await nextTick() - expect(cb).toHaveBeenCalled() + expect(cb).toHaveBeenCalledTimes(2) }) - it('ignore lazy option when using simple callback', async () => { + it('immediate: triggers when initial value is null', async () => { + const state = ref(null) + const spy = jest.fn() + watch(() => state.value, spy, { immediate: true }) + expect(spy).toHaveBeenCalled() + }) + + it('immediate: triggers when initial value is undefined', async () => { + const state = ref() + const spy = jest.fn() + watch(() => state.value, spy, { immediate: true }) + expect(spy).toHaveBeenCalled() + state.value = 3 + await nextTick() + expect(spy).toHaveBeenCalledTimes(2) + // testing if undefined can trigger the watcher + state.value = undefined + await nextTick() + expect(spy).toHaveBeenCalledTimes(3) + // it shouldn't trigger if the same value is set + state.value = undefined + await nextTick() + expect(spy).toHaveBeenCalledTimes(3) + }) + + it('warn immediate option when using effect signature', async () => { const count = ref(0) let dummy // @ts-ignore @@ -388,13 +371,10 @@ describe('api: watch', () => { () => { dummy = count.value }, - { lazy: true } + { immediate: false } ) - expect(dummy).toBeUndefined() - expect(`lazy option is only respected`).toHaveBeenWarned() - - await nextTick() expect(dummy).toBe(0) + expect(`"immediate" option is only respected`).toHaveBeenWarned() count.value++ await nextTick() diff --git a/packages/runtime-core/__tests__/components/Suspense.spec.ts b/packages/runtime-core/__tests__/components/Suspense.spec.ts index 264bd7ca4b1..a530532f077 100644 --- a/packages/runtime-core/__tests__/components/Suspense.spec.ts +++ b/packages/runtime-core/__tests__/components/Suspense.spec.ts @@ -164,8 +164,14 @@ describe('Suspense', () => { deps.push(p.then(() => Promise.resolve())) watch(() => { + calls.push('immediate effect') + }) + + const count = ref(0) + watch(count, v => { calls.push('watch callback') }) + count.value++ // trigger the watcher now onMounted(() => { calls.push('mounted') @@ -193,18 +199,24 @@ describe('Suspense', () => { const root = nodeOps.createElement('div') render(h(Comp), root) expect(serializeInner(root)).toBe(`
fallback
`) - expect(calls).toEqual([]) + expect(calls).toEqual([`immediate effect`]) await Promise.all(deps) await nextTick() expect(serializeInner(root)).toBe(`
async
`) - expect(calls).toEqual([`watch callback`, `mounted`]) + expect(calls).toEqual([`immediate effect`, `watch callback`, `mounted`]) // effects inside an already resolved suspense should happen at normal timing toggle.value = false await nextTick() + await nextTick() expect(serializeInner(root)).toBe(``) - expect(calls).toEqual([`watch callback`, `mounted`, 'unmounted']) + expect(calls).toEqual([ + `immediate effect`, + `watch callback`, + `mounted`, + 'unmounted' + ]) }) test('content update before suspense resolve', async () => { @@ -254,8 +266,14 @@ describe('Suspense', () => { deps.push(p) watch(() => { + calls.push('immediate effect') + }) + + const count = ref(0) + watch(count, () => { calls.push('watch callback') }) + count.value++ // trigger the watcher now onMounted(() => { calls.push('mounted') @@ -283,7 +301,7 @@ describe('Suspense', () => { const root = nodeOps.createElement('div') render(h(Comp), root) expect(serializeInner(root)).toBe(`
fallback
`) - expect(calls).toEqual([]) + expect(calls).toEqual(['immediate effect']) // remove the async dep before it's resolved toggle.value = false @@ -294,8 +312,8 @@ describe('Suspense', () => { await Promise.all(deps) await nextTick() expect(serializeInner(root)).toBe(``) - // should discard effects - expect(calls).toEqual([]) + // should discard effects (except for immediate ones) + expect(calls).toEqual(['immediate effect']) }) test('unmount suspense after resolve', async () => { diff --git a/packages/runtime-core/__tests__/errorHandling.spec.ts b/packages/runtime-core/__tests__/errorHandling.spec.ts index d9f753f9330..6396c6b35a4 100644 --- a/packages/runtime-core/__tests__/errorHandling.spec.ts +++ b/packages/runtime-core/__tests__/errorHandling.spec.ts @@ -241,7 +241,7 @@ describe('error handling', () => { expect(fn).toHaveBeenCalledWith(err, 'ref function') }) - test('in watch (simple usage)', () => { + test('in watch (effect)', () => { const err = new Error('foo') const fn = jest.fn() @@ -268,7 +268,7 @@ describe('error handling', () => { expect(fn).toHaveBeenCalledWith(err, 'watcher callback') }) - test('in watch getter', () => { + test('in watch (getter)', () => { const err = new Error('foo') const fn = jest.fn() @@ -298,7 +298,7 @@ describe('error handling', () => { expect(fn).toHaveBeenCalledWith(err, 'watcher getter') }) - test('in watch callback', () => { + test('in watch (callback)', async () => { const err = new Error('foo') const fn = jest.fn() @@ -312,10 +312,11 @@ describe('error handling', () => { } } + const count = ref(0) const Child = { setup() { watch( - () => 1, + () => count.value, () => { throw err } @@ -325,6 +326,9 @@ describe('error handling', () => { } render(h(Comp), nodeOps.createElement('div')) + + count.value++ + await nextTick() expect(fn).toHaveBeenCalledWith(err, 'watcher callback') }) diff --git a/packages/runtime-core/src/apiWatch.ts b/packages/runtime-core/src/apiWatch.ts index b44fbfc64ec..b3c92239faf 100644 --- a/packages/runtime-core/src/apiWatch.ts +++ b/packages/runtime-core/src/apiWatch.ts @@ -47,22 +47,25 @@ type MapSources = { [K in keyof T]: T[K] extends WatchSource ? V : never } -type MapOldSources = { +type MapOldSources = { [K in keyof T]: T[K] extends WatchSource - ? Lazy extends true ? V : (V | undefined) + ? Immediate extends true ? (V | undefined) : V : never } export type CleanupRegistrator = (invalidate: () => void) => void -export interface WatchOptions { - lazy?: Lazy +export interface BaseWatchOptions { flush?: 'pre' | 'post' | 'sync' - deep?: boolean onTrack?: ReactiveEffectOptions['onTrack'] onTrigger?: ReactiveEffectOptions['onTrigger'] } +export interface WatchOptions extends BaseWatchOptions { + immediate?: Immediate + deep?: boolean +} + export type StopHandle = () => void const invoke = (fn: Function) => fn() @@ -73,14 +76,14 @@ const INITIAL_WATCHER_VALUE = {} // overload #1: simple effect export function watch( effect: WatchEffect, - options?: WatchOptions + options?: BaseWatchOptions ): StopHandle // overload #2: single source + cb -export function watch = false>( +export function watch = false>( source: WatchSource, - cb: WatchCallback, - options?: WatchOptions + cb: WatchCallback, + options?: WatchOptions ): StopHandle // overload #3: array of multiple sources + cb @@ -89,11 +92,11 @@ export function watch = false>( // of all possible value types. export function watch< T extends Readonly[]>, - Lazy extends Readonly = false + Immediate extends Readonly = false >( sources: T, - cb: WatchCallback, MapOldSources>, - options?: WatchOptions + cb: WatchCallback, MapOldSources>, + options?: WatchOptions ): StopHandle // implementation @@ -102,15 +105,11 @@ export function watch( cbOrOptions?: WatchCallback | WatchOptions, options?: WatchOptions ): StopHandle { - if (isInSSRComponentSetup && !(options && options.flush === 'sync')) { - // component watchers during SSR are no-op - return NOOP - } else if (isFunction(cbOrOptions)) { - // effect callback as 2nd argument - this is a source watcher + if (isFunction(cbOrOptions)) { + // watch(source, cb) return doWatch(effectOrSource, cbOrOptions, options) } else { - // 2nd argument is either missing or an options object - // - this is a simple effect watcher + // watch(effect) return doWatch(effectOrSource, null, cbOrOptions) } } @@ -118,8 +117,23 @@ export function watch( function doWatch( source: WatchSource | WatchSource[] | WatchEffect, cb: WatchCallback | null, - { lazy, deep, flush, onTrack, onTrigger }: WatchOptions = EMPTY_OBJ + { immediate, deep, flush, onTrack, onTrigger }: WatchOptions = EMPTY_OBJ ): StopHandle { + if (__DEV__ && !cb) { + if (immediate !== undefined) { + warn( + `watch() "immediate" option is only respected when using the ` + + `watch(source, callback) signature.` + ) + } + if (deep !== undefined) { + warn( + `watch() "deep" option is only respected when using the ` + + `watch(source, callback) signature.` + ) + } + } + const instance = currentInstance const suspense = currentSuspense @@ -168,6 +182,21 @@ function doWatch( } } + // in SSR there is no need to setup an actual effect, and it should be noop + // unless it's eager + if (__NODE_JS__ && isInSSRComponentSetup) { + if (!cb) { + getter() + } else if (immediate) { + callWithAsyncErrorHandling(cb, instance, ErrorCodes.WATCH_CALLBACK, [ + getter(), + undefined, + registerCleanup + ]) + } + return NOOP + } + let oldValue = isArray(source) ? [] : INITIAL_WATCHER_VALUE const applyCb = cb ? () => { @@ -219,23 +248,19 @@ function doWatch( scheduler: applyCb ? () => scheduler(applyCb) : scheduler }) - if (lazy && cb) { - oldValue = runner() - } else { - if (__DEV__ && lazy && !cb) { - warn( - `watch() lazy option is only respected when using the ` + - `watch(getter, callback) signature.` - ) - } - if (applyCb) { - scheduler(applyCb) + recordInstanceBoundEffect(runner) + + // initial run + if (applyCb) { + if (immediate) { + applyCb() } else { - scheduler(runner) + oldValue = runner() } + } else { + runner() } - recordInstanceBoundEffect(runner) return () => { stop(runner) if (instance) { diff --git a/packages/runtime-core/src/components/KeepAlive.ts b/packages/runtime-core/src/components/KeepAlive.ts index 6587e91122c..d5750d5aacf 100644 --- a/packages/runtime-core/src/components/KeepAlive.ts +++ b/packages/runtime-core/src/components/KeepAlive.ts @@ -135,8 +135,7 @@ const KeepAliveImpl = { ([include, exclude]) => { include && pruneCache(name => matches(include, name)) exclude && pruneCache(name => matches(exclude, name)) - }, - { lazy: true } + } ) onBeforeUnmount(() => { diff --git a/test-dts/watch.test-d.ts b/test-dts/watch.test-d.ts index d32313cc406..72a38e8d02d 100644 --- a/test-dts/watch.test-d.ts +++ b/test-dts/watch.test-d.ts @@ -25,22 +25,39 @@ watch([source, source2, source3] as const, (values, oldValues) => { }) // lazy watcher will have consistent types for oldValue. +watch(source, (value, oldValue) => { + expectType(value) + expectType(oldValue) +}) + +watch([source, source2, source3], (values, oldValues) => { + expectType<(string | number)[]>(values) + expectType<(string | number)[]>(oldValues) +}) + +// const array +watch([source, source2, source3] as const, (values, oldValues) => { + expectType>(values) + expectType>(oldValues) +}) + +// source + immediate: true watch( source, (value, oldValue) => { expectType(value) - expectType(oldValue) + expectType(oldValue) }, - { lazy: true } + { immediate: true } ) watch( [source, source2, source3], (values, oldValues) => { expectType<(string | number)[]>(values) - expectType<(string | number)[]>(oldValues) + expectType<(string | number | undefined)[]>(oldValues) }, - { lazy: true } + { immediate: true } ) // const array @@ -48,7 +65,9 @@ watch( [source, source2, source3] as const, (values, oldValues) => { expectType>(values) - expectType>(oldValues) + expectType< + Readonly<[string | undefined, string | undefined, number | undefined]> + >(oldValues) }, - { lazy: true } + { immediate: true } )