From 4f55febdade37117844826ebe65fb9a5a4c911ca Mon Sep 17 00:00:00 2001 From: lidlanca <8693091+lidlanca@users.noreply.github.com> Date: Sat, 12 Feb 2022 18:08:09 -0500 Subject: [PATCH 1/6] fix(runtime-core): allow spying on proxy methods regression #5415 #4216 --- .../__tests__/componentPublicInstance.spec.ts | 38 +++++++++++++++++-- .../src/componentPublicInstance.ts | 2 +- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/packages/runtime-core/__tests__/componentPublicInstance.spec.ts b/packages/runtime-core/__tests__/componentPublicInstance.spec.ts index fe588115d31..5bcfc91e76c 100644 --- a/packages/runtime-core/__tests__/componentPublicInstance.spec.ts +++ b/packages/runtime-core/__tests__/componentPublicInstance.spec.ts @@ -257,13 +257,13 @@ describe('component: proxy', () => { expect(instanceProxy.isDisplayed).toBe(true) }) - test('allow spying on proxy methods', () => { + test('allow jest spying on proxy methods with Object.defineProperty', () => { let instanceProxy: any const Comp = { render() {}, setup() { return { - toggle() {} + toggle() { return 'a'} } }, mounted() { @@ -274,10 +274,42 @@ describe('component: proxy', () => { const app = createApp(Comp) app.mount(nodeOps.createElement('div')) + + // access 'toggle' to ensure key is cached + let v1 = instanceProxy.toggle() + + // reconfigure "toggle" to be getter based. + let getCalledTimes = 0 + Object.defineProperty(instanceProxy, 'toggle', { + get(){ + getCalledTimes++ + return ()=> 'b' + } + }) - const spy = jest.spyOn(instanceProxy, 'toggle') + // getter should not be evaluated on definition + expect(getCalledTimes).toEqual(0) + + // invoke "toggle" after "defineProperty" + let v2 = instanceProxy.toggle() + expect(getCalledTimes).toEqual(1) + // expect toggle getter not to be cached. it can't be instanceProxy.toggle() + expect(getCalledTimes).toEqual(2) + + // attaching jest spy, triggers the getter once, cache it and override the property. + // also uses Object.defineProperty + const spy = jest.spyOn(instanceProxy, 'toggle') + expect(getCalledTimes).toEqual(3) + + // toggle getter is not going to be evaluated due to jest wrapper caching it result + let v3 = instanceProxy.toggle() + expect(getCalledTimes).toEqual(3) + + expect(v1).toEqual('a') + expect(v2).toEqual('b') + expect(v3).toEqual('b') expect(spy).toHaveBeenCalled() }) diff --git a/packages/runtime-core/src/componentPublicInstance.ts b/packages/runtime-core/src/componentPublicInstance.ts index 0413730032c..53e85ef066d 100644 --- a/packages/runtime-core/src/componentPublicInstance.ts +++ b/packages/runtime-core/src/componentPublicInstance.ts @@ -455,7 +455,7 @@ export const PublicInstanceProxyHandlers: ProxyHandler = { descriptor: PropertyDescriptor ) { if (descriptor.get != null) { - this.set!(target, key, descriptor.get(), null) + target.$.accessCache[key] = 0; } else if (descriptor.value != null) { this.set!(target, key, descriptor.value, null) } From 0d6f3338702abfde40f64fd2d5596e3fc838caa5 Mon Sep 17 00:00:00 2001 From: lidlanca <8693091+lidlanca@users.noreply.github.com> Date: Sat, 12 Feb 2022 18:48:48 -0500 Subject: [PATCH 2/6] fix(runtime-core): allow spying on proxy methods --- .../__tests__/componentPublicInstance.spec.ts | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/packages/runtime-core/__tests__/componentPublicInstance.spec.ts b/packages/runtime-core/__tests__/componentPublicInstance.spec.ts index 5bcfc91e76c..2fc3acf10e8 100644 --- a/packages/runtime-core/__tests__/componentPublicInstance.spec.ts +++ b/packages/runtime-core/__tests__/componentPublicInstance.spec.ts @@ -314,6 +314,37 @@ describe('component: proxy', () => { expect(spy).toHaveBeenCalled() }) + test('defineProperty on proxy property with value descriptor', () => { + let instanceProxy: any + const Comp = { + render() {}, + setup() { + return { + toggle:'a' + } + }, + mounted() { + instanceProxy = this + } + } + + const app = createApp(Comp) + + app.mount(nodeOps.createElement('div')) + + let v1 = instanceProxy.toggle + + Object.defineProperty(instanceProxy, 'toggle', { + value:'b' + }) + + let v2 = instanceProxy.toggle + + expect(v1).toEqual('a') + expect(v2).toEqual('b') + + }) + // #864 test('should not warn declared but absent props', () => { const Comp = { From 04adf7ac79da88ea2b58af661935ddd108fc54cb Mon Sep 17 00:00:00 2001 From: lidlanca <8693091+lidlanca@users.noreply.github.com> Date: Sat, 12 Feb 2022 19:53:14 -0500 Subject: [PATCH 3/6] allow null as a value for property descriptor --- .../runtime-core/__tests__/componentPublicInstance.spec.ts | 7 ++++++- packages/runtime-core/src/componentPublicInstance.ts | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/runtime-core/__tests__/componentPublicInstance.spec.ts b/packages/runtime-core/__tests__/componentPublicInstance.spec.ts index 2fc3acf10e8..879fab0ca95 100644 --- a/packages/runtime-core/__tests__/componentPublicInstance.spec.ts +++ b/packages/runtime-core/__tests__/componentPublicInstance.spec.ts @@ -337,11 +337,16 @@ describe('component: proxy', () => { Object.defineProperty(instanceProxy, 'toggle', { value:'b' }) - let v2 = instanceProxy.toggle + Object.defineProperty(instanceProxy, 'toggle', { + value:null + }) + let v3 = instanceProxy.toggle + expect(v1).toEqual('a') expect(v2).toEqual('b') + expect(v3).toBeNull() }) diff --git a/packages/runtime-core/src/componentPublicInstance.ts b/packages/runtime-core/src/componentPublicInstance.ts index 53e85ef066d..c123aa0953d 100644 --- a/packages/runtime-core/src/componentPublicInstance.ts +++ b/packages/runtime-core/src/componentPublicInstance.ts @@ -456,7 +456,7 @@ export const PublicInstanceProxyHandlers: ProxyHandler = { ) { if (descriptor.get != null) { target.$.accessCache[key] = 0; - } else if (descriptor.value != null) { + } else if (hasOwn(descriptor,'value')) { this.set!(target, key, descriptor.value, null) } return Reflect.defineProperty(target, key, descriptor) From 3aa7932b6440da9246c0867eaffadb10ea81e75e Mon Sep 17 00:00:00 2001 From: lidlanca <8693091+lidlanca@users.noreply.github.com> Date: Sun, 13 Feb 2022 15:25:49 -0500 Subject: [PATCH 4/6] add null test and tidy --- .../__tests__/componentPublicInstance.spec.ts | 80 +++++++++++++------ .../src/componentPublicInstance.ts | 1 + 2 files changed, 56 insertions(+), 25 deletions(-) diff --git a/packages/runtime-core/__tests__/componentPublicInstance.spec.ts b/packages/runtime-core/__tests__/componentPublicInstance.spec.ts index 879fab0ca95..731612da683 100644 --- a/packages/runtime-core/__tests__/componentPublicInstance.spec.ts +++ b/packages/runtime-core/__tests__/componentPublicInstance.spec.ts @@ -257,13 +257,17 @@ describe('component: proxy', () => { expect(instanceProxy.isDisplayed).toBe(true) }) + test('allow jest spying on proxy methods with Object.defineProperty', () => { + // #5417 let instanceProxy: any const Comp = { render() {}, setup() { return { - toggle() { return 'a'} + toggle() { + return 'a' + } } }, mounted() { @@ -274,53 +278,52 @@ describe('component: proxy', () => { const app = createApp(Comp) app.mount(nodeOps.createElement('div')) - + // access 'toggle' to ensure key is cached let v1 = instanceProxy.toggle() + expect(v1).toEqual('a') // reconfigure "toggle" to be getter based. let getCalledTimes = 0 Object.defineProperty(instanceProxy, 'toggle', { - get(){ + get() { getCalledTimes++ - return ()=> 'b' + return () => 'b' } }) - // getter should not be evaluated on definition + // getter should not be evaluated on initial definition expect(getCalledTimes).toEqual(0) // invoke "toggle" after "defineProperty" - let v2 = instanceProxy.toggle() + const v2 = instanceProxy.toggle() + expect(v2).toEqual('b') expect(getCalledTimes).toEqual(1) // expect toggle getter not to be cached. it can't be instanceProxy.toggle() expect(getCalledTimes).toEqual(2) - + // attaching jest spy, triggers the getter once, cache it and override the property. - // also uses Object.defineProperty + // also uses Object.defineProperty const spy = jest.spyOn(instanceProxy, 'toggle') expect(getCalledTimes).toEqual(3) - // toggle getter is not going to be evaluated due to jest wrapper caching it result - let v3 = instanceProxy.toggle() - expect(getCalledTimes).toEqual(3) - - expect(v1).toEqual('a') - expect(v2).toEqual('b') + // expect getter to not evaluate the jest spy caches its value + const v3 = instanceProxy.toggle() expect(v3).toEqual('b') - expect(spy).toHaveBeenCalled() + expect(getCalledTimes).toEqual(3) }) test('defineProperty on proxy property with value descriptor', () => { + // #5417 let instanceProxy: any const Comp = { render() {}, setup() { return { - toggle:'a' + toggle: 'a' } }, mounted() { @@ -331,23 +334,50 @@ describe('component: proxy', () => { const app = createApp(Comp) app.mount(nodeOps.createElement('div')) - - let v1 = instanceProxy.toggle + + const v1 = instanceProxy.toggle + expect(v1).toEqual('a') Object.defineProperty(instanceProxy, 'toggle', { - value:'b' + value: 'b' }) - let v2 = instanceProxy.toggle + const v2 = instanceProxy.toggle + expect(v2).toEqual('b') Object.defineProperty(instanceProxy, 'toggle', { - value:null + value: null }) - let v3 = instanceProxy.toggle + const v3 = instanceProxy.toggle + expect(v3).toBeNull() + }) + test('defineProperty on proxy property with null value descriptor', () => { + // #5417 + let instanceProxy: any + const Comp = { + render() {}, + setup() { + return { + toggle: 'a' + } + }, + mounted() { + instanceProxy = this + } + } + + const app = createApp(Comp) + + app.mount(nodeOps.createElement('div')) + + const v1 = instanceProxy.toggle expect(v1).toEqual('a') - expect(v2).toEqual('b') - expect(v3).toBeNull() - + Object.defineProperty(instanceProxy, 'toggle', { + value: null + }) + + const v2 = instanceProxy.toggle + expect(v2).toBeNull() }) // #864 diff --git a/packages/runtime-core/src/componentPublicInstance.ts b/packages/runtime-core/src/componentPublicInstance.ts index c123aa0953d..9bfbfc10371 100644 --- a/packages/runtime-core/src/componentPublicInstance.ts +++ b/packages/runtime-core/src/componentPublicInstance.ts @@ -455,6 +455,7 @@ export const PublicInstanceProxyHandlers: ProxyHandler = { descriptor: PropertyDescriptor ) { if (descriptor.get != null) { + // invalidate key cache of a getter based property #5417 target.$.accessCache[key] = 0; } else if (hasOwn(descriptor,'value')) { this.set!(target, key, descriptor.value, null) From 2e2fdfb8c9d883c0a6d7a08cbc88e1d2b5f2b26d Mon Sep 17 00:00:00 2001 From: lidlanca <8693091+lidlanca@users.noreply.github.com> Date: Sun, 13 Feb 2022 15:27:09 -0500 Subject: [PATCH 5/6] const --- packages/runtime-core/__tests__/componentPublicInstance.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/runtime-core/__tests__/componentPublicInstance.spec.ts b/packages/runtime-core/__tests__/componentPublicInstance.spec.ts index 731612da683..64a5e4c7e9d 100644 --- a/packages/runtime-core/__tests__/componentPublicInstance.spec.ts +++ b/packages/runtime-core/__tests__/componentPublicInstance.spec.ts @@ -280,7 +280,7 @@ describe('component: proxy', () => { app.mount(nodeOps.createElement('div')) // access 'toggle' to ensure key is cached - let v1 = instanceProxy.toggle() + const v1 = instanceProxy.toggle() expect(v1).toEqual('a') // reconfigure "toggle" to be getter based. From 834965a0b829138ec7a9ec08a8002b74e8e21a1a Mon Sep 17 00:00:00 2001 From: lidlanca <8693091+lidlanca@users.noreply.github.com> Date: Sun, 13 Feb 2022 17:05:46 -0500 Subject: [PATCH 6/6] test coverage - SETUP,DATA,CONTEXT,PROPS --- .../__tests__/componentPublicInstance.spec.ts | 65 ++++++++++++++++--- 1 file changed, 56 insertions(+), 9 deletions(-) diff --git a/packages/runtime-core/__tests__/componentPublicInstance.spec.ts b/packages/runtime-core/__tests__/componentPublicInstance.spec.ts index 64a5e4c7e9d..2bea4e0eedc 100644 --- a/packages/runtime-core/__tests__/componentPublicInstance.spec.ts +++ b/packages/runtime-core/__tests__/componentPublicInstance.spec.ts @@ -344,6 +344,7 @@ describe('component: proxy', () => { const v2 = instanceProxy.toggle expect(v2).toEqual('b') + // expect null to be a settable value Object.defineProperty(instanceProxy, 'toggle', { value: null }) @@ -351,14 +352,23 @@ describe('component: proxy', () => { expect(v3).toBeNull() }) - test('defineProperty on proxy property with null value descriptor', () => { + test('defineProperty on public instance proxy should work with SETUP,DATA,CONTEXT,PROPS', () => { // #5417 let instanceProxy: any const Comp = { + props: ['fromProp'], + data() { + return { name: 'data.name' } + }, + computed: { + greet() { + return 'Hi ' + (this as any).name + } + }, render() {}, setup() { return { - toggle: 'a' + fromSetup: true } }, mounted() { @@ -366,20 +376,57 @@ describe('component: proxy', () => { } } - const app = createApp(Comp) + const app = createApp(Comp, { + fromProp: true + }) app.mount(nodeOps.createElement('div')) + expect(instanceProxy.greet).toEqual('Hi data.name') - const v1 = instanceProxy.toggle - expect(v1).toEqual('a') - Object.defineProperty(instanceProxy, 'toggle', { - value: null + // define property on data + Object.defineProperty(instanceProxy, 'name', { + get() { + return 'getter.name' + } }) - const v2 = instanceProxy.toggle - expect(v2).toBeNull() + // computed is same still cached + expect(instanceProxy.greet).toEqual('Hi data.name') + + // trigger computed + instanceProxy.name = '' + + // expect "greet" to evaluated and use name from context getter + expect(instanceProxy.greet).toEqual('Hi getter.name') + + // defineProperty on computed ( context ) + Object.defineProperty(instanceProxy, 'greet', { + get() { + return 'Hi greet.getter.computed' + } + }) + expect(instanceProxy.greet).toEqual('Hi greet.getter.computed') + + // defineProperty on setupState + expect(instanceProxy.fromSetup).toBe(true) + Object.defineProperty(instanceProxy, 'fromSetup', { + get() { + return false + } + }) + expect(instanceProxy.fromSetup).toBe(false) + + // defineProperty on Props + expect(instanceProxy.fromProp).toBe(true) + Object.defineProperty(instanceProxy, 'fromProp', { + get() { + return false + } + }) + expect(instanceProxy.fromProp).toBe(false) }) + // #864 test('should not warn declared but absent props', () => { const Comp = {