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

feat: warn when operating on destroyed Vue component, fix #1685 #1706

Merged
merged 1 commit into from
Oct 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 56 additions & 0 deletions packages/test-utils/src/wrapper.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
getCheckedEvent,
isPhantomJS,
nextTick,
warn,
warnDeprecated
} from 'shared/util'
import { isElementVisible } from 'shared/is-visible'
Expand Down Expand Up @@ -82,14 +83,27 @@ export default class Wrapper implements BaseWrapper {
}
}

/**
* Prints warning if component is destroyed
*/
__warnIfDestroyed() {
if (!this.exists()) {
warn('Operations on destroyed component are discouraged')
}
}

at(): void {
this.__warnIfDestroyed()

throwError('at() must be called on a WrapperArray')
}

/**
* Returns an Object containing all the attribute/value pairs on the element.
*/
attributes(key?: string): { [name: string]: string } | string {
this.__warnIfDestroyed()

const attributes = this.element.attributes
const attributeMap = {}
for (let i = 0; i < attributes.length; i++) {
Expand All @@ -104,6 +118,8 @@ export default class Wrapper implements BaseWrapper {
* Returns an Array containing all the classes on the element
*/
classes(className?: string): Array<string> | boolean {
this.__warnIfDestroyed()

const classAttribute = this.element.getAttribute('class')
let classes = classAttribute ? classAttribute.split(' ') : []
// Handle converting cssmodules identifiers back to the original class name
Expand Down Expand Up @@ -134,6 +150,9 @@ export default class Wrapper implements BaseWrapper {
'contains',
'Use `wrapper.find`, `wrapper.findComponent` or `wrapper.get` instead'
)

this.__warnIfDestroyed()

const selector = getSelector(rawSelector, 'contains')
const nodes = find(this.rootNode, this.vm, selector)
return nodes.length > 0
Expand Down Expand Up @@ -209,6 +228,8 @@ export default class Wrapper implements BaseWrapper {
* matches the provided selector.
*/
get(rawSelector: Selector): Wrapper {
this.__warnIfDestroyed()

const found = this.find(rawSelector)
if (found instanceof ErrorWrapper) {
throw new Error(`Unable to find ${rawSelector} within: ${this.html()}`)
Expand All @@ -221,6 +242,8 @@ export default class Wrapper implements BaseWrapper {
* matches the provided selector.
*/
find(rawSelector: Selector): Wrapper | ErrorWrapper {
this.__warnIfDestroyed()

const selector = getSelector(rawSelector, 'find')
if (selector.type !== DOM_SELECTOR) {
warnDeprecated(
Expand All @@ -237,6 +260,8 @@ export default class Wrapper implements BaseWrapper {
* matches the provided selector.
*/
findComponent(rawSelector: Selector): Wrapper | ErrorWrapper {
this.__warnIfDestroyed()

const selector = getSelector(rawSelector, 'findComponent')
if (!this.vm && !this.isFunctionalComponent) {
throwError(
Expand Down Expand Up @@ -270,6 +295,8 @@ export default class Wrapper implements BaseWrapper {
* the provided selector.
*/
findAll(rawSelector: Selector): WrapperArray {
this.__warnIfDestroyed()

const selector = getSelector(rawSelector, 'findAll')
if (selector.type !== DOM_SELECTOR) {
warnDeprecated(
Expand All @@ -285,6 +312,8 @@ export default class Wrapper implements BaseWrapper {
* the provided selector.
*/
findAllComponents(rawSelector: Selector): WrapperArray {
this.__warnIfDestroyed()

const selector = getSelector(rawSelector, 'findAll')
if (!this.vm) {
throwError(
Expand Down Expand Up @@ -318,13 +347,17 @@ export default class Wrapper implements BaseWrapper {
* Returns HTML of element as a string
*/
html(): string {
this.__warnIfDestroyed()

return pretty(this.element.outerHTML)
}

/**
* Checks if node matches selector or component definition
*/
is(rawSelector: Selector): boolean {
this.__warnIfDestroyed()

const selector = getSelector(rawSelector, 'is')

if (selector.type === DOM_SELECTOR) {
Expand All @@ -351,6 +384,8 @@ export default class Wrapper implements BaseWrapper {
'Consider a custom matcher such as those provided in jest-dom: https://github.com/testing-library/jest-dom#tobeempty. ' +
'When using with findComponent, access the DOM element with findComponent(Comp).element'
)
this.__warnIfDestroyed()

if (!this.vnode) {
return this.element.innerHTML === ''
}
Expand All @@ -375,6 +410,8 @@ export default class Wrapper implements BaseWrapper {
* Checks if node is visible
*/
isVisible(): boolean {
this.__warnIfDestroyed()

return isElementVisible(this.element)
}

Expand All @@ -384,6 +421,8 @@ export default class Wrapper implements BaseWrapper {
*/
isVueInstance(): boolean {
warnDeprecated(`isVueInstance`)
this.__warnIfDestroyed()

return !!this.vm
}

Expand All @@ -393,6 +432,7 @@ export default class Wrapper implements BaseWrapper {
*/
name(): string {
warnDeprecated(`name`)
this.__warnIfDestroyed()

if (this.vm) {
return (
Expand All @@ -416,6 +456,7 @@ export default class Wrapper implements BaseWrapper {
*/
overview(): void {
warnDeprecated(`overview`)
this.__warnIfDestroyed()

if (!this.vm) {
throwError(`wrapper.overview() can only be called on a Vue instance`)
Expand Down Expand Up @@ -495,6 +536,7 @@ export default class Wrapper implements BaseWrapper {
if (!this.vm) {
throwError('wrapper.props() must be called on a Vue instance')
}
this.__warnIfDestroyed()

const props = {}
const keys = this.vm && this.vm.$options._propKeys
Expand All @@ -519,6 +561,8 @@ export default class Wrapper implements BaseWrapper {
* @deprecated
*/
setChecked(checked: boolean = true): Promise<*> {
this.__warnIfDestroyed()

if (typeof checked !== 'boolean') {
throwError('wrapper.setChecked() must be passed a boolean')
}
Expand Down Expand Up @@ -568,6 +612,8 @@ export default class Wrapper implements BaseWrapper {
* @deprecated
*/
setSelected(): Promise<void> {
this.__warnIfDestroyed()

const tagName = this.element.tagName

if (tagName === 'SELECT') {
Expand Down Expand Up @@ -613,6 +659,8 @@ export default class Wrapper implements BaseWrapper {
throwError(`wrapper.setData() can only be called on a Vue instance`)
}

this.__warnIfDestroyed()

recursivelySetData(this.vm, this.vm, data)
return nextTick()
}
Expand All @@ -630,6 +678,8 @@ export default class Wrapper implements BaseWrapper {
if (!this.vm) {
throwError(`wrapper.setMethods() can only be called on a Vue instance`)
}
this.__warnIfDestroyed()

Object.keys(methods).forEach(key => {
// $FlowIgnore : Problem with possibly null this.vm
this.vm[key] = methods[key]
Expand Down Expand Up @@ -657,6 +707,7 @@ export default class Wrapper implements BaseWrapper {
if (!this.vm) {
throwError(`wrapper.setProps() can only be called on a Vue instance`)
}
this.__warnIfDestroyed()

// Save the original "silent" config so that we can directly mutate props
const originalConfig = Vue.config.silent
Expand Down Expand Up @@ -730,6 +781,7 @@ export default class Wrapper implements BaseWrapper {
const tagName = this.element.tagName
// $FlowIgnore
const type = this.attributes().type
this.__warnIfDestroyed()

if (tagName === 'OPTION') {
throwError(
Expand Down Expand Up @@ -782,13 +834,17 @@ export default class Wrapper implements BaseWrapper {
* Return text of wrapper element
*/
text(): string {
this.__warnIfDestroyed()

return this.element.textContent.trim()
}

/**
* Dispatches a DOM event on wrapper
*/
trigger(type: string, options: Object = {}): Promise<void> {
this.__warnIfDestroyed()

if (typeof type !== 'string') {
throwError('wrapper.trigger() must be passed a string')
}
Expand Down
58 changes: 58 additions & 0 deletions test/specs/wrapper/destroy.spec.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,19 @@
import { describeWithShallowAndMount } from '~resources/utils'
import { config } from 'packages/test-utils/src'

describeWithShallowAndMount('destroy', mountingMethod => {
let originalConsoleError

beforeEach(() => {
config.showDeprecationWarnings = true
originalConsoleError = console.error
console.error = jest.fn()
})

afterEach(() => {
console.error = originalConsoleError
})

it('triggers beforeDestroy ', () => {
const stub = jest.fn()
mountingMethod({
Expand Down Expand Up @@ -61,4 +74,49 @@ describeWithShallowAndMount('destroy', mountingMethod => {
const wrapper = mountingMethod(TestComponent)
expect(() => wrapper.destroy()).toThrow()
})

const StubComponent = { props: ['a'], template: '<div><p></p></div>' }

;[
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be great to use jest.each here, but that will fail in Karma :(

Copy link
Member

Choose a reason for hiding this comment

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

A lot of cool jest things fail in karma... all good.

['attributes'],
['classes'],
['isEmpty'],
['isVisible'],
['isVueInstance'],
['name'],
['overview'],
['props'],
['text'],
['html'],
['contains', ['p']],
['get', ['p']],
['find', ['p']],
['findComponent', [StubComponent]],
['findAll', [StubComponent]],
['findAllComponents', [StubComponent]],
['is', [StubComponent]],
['setProps', [{ a: 1 }]],
['setData', [{}]],
['setMethods', [{}]],
['trigger', ['test-event']]
].forEach(([method, args = []]) => {
it(`displays warning when ${method} is called on destroyed wrapper`, () => {
config.showDeprecationWarnings = false
const wrapper = mountingMethod(StubComponent)
wrapper.destroy()
wrapper[method](...args)

expect(console.error).toHaveBeenCalled()
})
})
;['emitted', 'emittedByOrder', 'exists'].forEach(method => {
it(`does not display warning when ${method} is called on destroyed wrapper`, () => {
config.showDeprecationWarnings = false
const wrapper = mountingMethod(StubComponent)
wrapper.destroy()
wrapper[method]()

expect(console.error).not.toHaveBeenCalled()
})
})
})