Skip to content

Commit

Permalink
fix(runtime-core): allow spying on proxy methods regression (#5417)
Browse files Browse the repository at this point in the history
fix #5415 (regression by #4216)
  • Loading branch information
lidlanca authored Feb 14, 2022
1 parent cea82cf commit 1574edd
Show file tree
Hide file tree
Showing 2 changed files with 151 additions and 5 deletions.
151 changes: 148 additions & 3 deletions packages/runtime-core/__tests__/componentPublicInstance.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -257,13 +257,17 @@ describe('component: proxy', () => {
expect(instanceProxy.isDisplayed).toBe(true)
})

test('allow spying on proxy methods', () => {

test('allow jest spying on proxy methods with Object.defineProperty', () => {
// #5417
let instanceProxy: any
const Comp = {
render() {},
setup() {
return {
toggle() {}
toggle() {
return 'a'
}
}
},
mounted() {
Expand All @@ -275,13 +279,154 @@ describe('component: proxy', () => {

app.mount(nodeOps.createElement('div'))

const spy = jest.spyOn(instanceProxy, 'toggle')
// access 'toggle' to ensure key is cached
const v1 = instanceProxy.toggle()
expect(v1).toEqual('a')

// reconfigure "toggle" to be getter based.
let getCalledTimes = 0
Object.defineProperty(instanceProxy, 'toggle', {
get() {
getCalledTimes++
return () => 'b'
}
})

// getter should not be evaluated on initial definition
expect(getCalledTimes).toEqual(0)

// invoke "toggle" after "defineProperty"
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
const spy = jest.spyOn(instanceProxy, 'toggle')
expect(getCalledTimes).toEqual(3)

// 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'
}
},
mounted() {
instanceProxy = this
}
}

const app = createApp(Comp)

app.mount(nodeOps.createElement('div'))

const v1 = instanceProxy.toggle
expect(v1).toEqual('a')

Object.defineProperty(instanceProxy, 'toggle', {
value: 'b'
})
const v2 = instanceProxy.toggle
expect(v2).toEqual('b')

// expect null to be a settable value
Object.defineProperty(instanceProxy, 'toggle', {
value: null
})
const v3 = instanceProxy.toggle
expect(v3).toBeNull()
})

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 {
fromSetup: true
}
},
mounted() {
instanceProxy = this
}
}

const app = createApp(Comp, {
fromProp: true
})

app.mount(nodeOps.createElement('div'))
expect(instanceProxy.greet).toEqual('Hi data.name')

// define property on data
Object.defineProperty(instanceProxy, 'name', {
get() {
return 'getter.name'
}
})

// 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 = {
Expand Down
5 changes: 3 additions & 2 deletions packages/runtime-core/src/componentPublicInstance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -455,8 +455,9 @@ export const PublicInstanceProxyHandlers: ProxyHandler<any> = {
descriptor: PropertyDescriptor
) {
if (descriptor.get != null) {
this.set!(target, key, descriptor.get(), null)
} else if (descriptor.value != 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)
}
return Reflect.defineProperty(target, key, descriptor)
Expand Down

1 comment on commit 1574edd

@mbrodala
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this has been done more than a month ago: are there plans for a new release (3.2.32)?

Our acceptance tests fail with the update to 3.2.31 and we would like to see if this already fixes the issue.

Please sign in to comment.