Skip to content

Commit

Permalink
fix(runtime-core): allow spying on proxy methods (#4216)
Browse files Browse the repository at this point in the history
Since Jest v26.6.1, the mock method changed (see this commit jestjs/jest@30e8020)  to rely on `Object.defineProperty` in some cases.

This breaks spying on proxy's methods, because even if Jest is properly calling `Object.defineProperty`, the cached value in the `get` section of the proxy is never updated, and the spy is in fact never used.
This is easily reproducible as vue-next already uses a version of jest with these changes.

This is blocking projects (like vue-test-utils-next and vue-cli) to update to recent Jest versions.

This commit adds a `defineProperty` method to the proxy handler, that properly updates the defined value in the cache.
  • Loading branch information
cexbrayat committed Feb 12, 2022
1 parent 436c500 commit 8457d8b
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 0 deletions.
68 changes: 68 additions & 0 deletions packages/runtime-core/__tests__/componentPublicInstance.spec.ts
Expand Up @@ -214,6 +214,74 @@ describe('component: proxy', () => {
])
})

test('allow updating proxy with Object.defineProperty', () => {
let instanceProxy: any
const Comp = {
render() {},
setup() {
return {
isDisplayed: true
}
},
mounted() {
instanceProxy = this
}
}

const app = createApp(Comp)

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

Object.defineProperty(instanceProxy, 'isDisplayed', { value: false })

expect(instanceProxy.isDisplayed).toBe(false)

Object.defineProperty(instanceProxy, 'isDisplayed', { value: true })

expect(instanceProxy.isDisplayed).toBe(true)

Object.defineProperty(instanceProxy, 'isDisplayed', {
get() {
return false
}
})

expect(instanceProxy.isDisplayed).toBe(false)

Object.defineProperty(instanceProxy, 'isDisplayed', {
get() {
return true
}
})

expect(instanceProxy.isDisplayed).toBe(true)
})

test('allow spying on proxy methods', () => {
let instanceProxy: any
const Comp = {
render() {},
setup() {
return {
toggle() {}
}
},
mounted() {
instanceProxy = this
}
}

const app = createApp(Comp)

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

const spy = jest.spyOn(instanceProxy, 'toggle')

instanceProxy.toggle()

expect(spy).toHaveBeenCalled()
})

// #864
test('should not warn declared but absent props', () => {
const Comp = {
Expand Down
15 changes: 15 additions & 0 deletions packages/runtime-core/src/componentPublicInstance.ts
Expand Up @@ -397,8 +397,10 @@ export const PublicInstanceProxyHandlers: ProxyHandler<any> = {
const { data, setupState, ctx } = instance
if (setupState !== EMPTY_OBJ && hasOwn(setupState, key)) {
setupState[key] = value
return true
} else if (data !== EMPTY_OBJ && hasOwn(data, key)) {
data[key] = value
return true
} else if (hasOwn(instance.props, key)) {
__DEV__ &&
warn(
Expand Down Expand Up @@ -445,6 +447,19 @@ export const PublicInstanceProxyHandlers: ProxyHandler<any> = {
hasOwn(publicPropertiesMap, key) ||
hasOwn(appContext.config.globalProperties, key)
)
},

defineProperty(
target: ComponentRenderContext,
key: string,
descriptor: PropertyDescriptor
) {
if (descriptor.get != null) {
this.set!(target, key, descriptor.get(), null)
} else if (descriptor.value != null) {
this.set!(target, key, descriptor.value, null)
}
return Reflect.defineProperty(target, key, descriptor)
}
}

Expand Down

1 comment on commit 8457d8b

@juddy-star
Copy link

@juddy-star juddy-star commented on 8457d8b Feb 16, 2022

Choose a reason for hiding this comment

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

Will lines 400 and 403 affect the logic between lines 421 and 429 @cexbrayat

Please sign in to comment.