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.

Sign up for GitHub

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

fix(component-testing): Correctly unmount react components #15250

Merged
merged 2 commits into from
Feb 26, 2021
Merged

fix(component-testing): Correctly unmount react components #15250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d152c31
Correctly unmount react components
dmtrKovalenko Feb 26, 2021
c80514d
Merge branch 'develop' into fix/correctly-unmount-react-components
dmtrKovalenko Feb 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 0 additions & 17 deletions npm/react/src/hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,6 @@ export function setupHooks (rootId: string) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Though this is not part of the fix, this clean-up is a very good idea.
I am going to check for the same issues in the vue side.

})

/** This function stays here only for old experimental component-testing */
function renderTestingPlatform () {
if (document.getElementById(rootId)) {
return
}

const rootNode = document.createElement('div')

rootNode.setAttribute('id', rootId)
document.getElementsByTagName('body')[0].prepend(rootNode)

const selector = `#${rootId}`

return cy.get(selector, { log: false })
}

/**
* Remove any style or extra link elements from the iframe placeholder
* left from any previous test
Expand Down Expand Up @@ -64,7 +48,6 @@ export function setupHooks (rootId: string) {
return
}

renderTestingPlatform()
cleanupStyles()
})
}
13 changes: 8 additions & 5 deletions npm/react/src/mount.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as React from 'react'
import ReactDOM, { unmountComponentAtNode } from 'react-dom'
import * as ReactDOM from 'react-dom'
import getDisplayName from './getDisplayName'
import { injectStylesBeforeElement } from './utils'
import { setupHooks } from './hooks'
Expand Down Expand Up @@ -50,7 +50,7 @@ export const mount = (jsx: React.ReactNode, options: MountOptions = {}) => {
// @ts-ignore
let logInstance: Cypress.Log

return cy
return unmount({ log: false })
.then(() => {
if (options.log !== false) {
logInstance = Cypress.log({
Expand Down Expand Up @@ -154,13 +154,16 @@ export const mount = (jsx: React.ReactNode, options: MountOptions = {}) => {
})
```
*/
export const unmount = () => {
export const unmount = (options = { log: true }) => {
return cy.then(() => {
cy.log('unmounting...')
const selector = `#${ROOT_ID}`

return cy.get(selector, { log: false }).then(($el) => {
unmountComponentAtNode($el[0])
const wasUnmounted = ReactDOM.unmountComponentAtNode($el[0])

if (wasUnmounted && options.log) {
cy.log('Unmounted component at', $el)
}
})
})
}
Expand Down
8 changes: 5 additions & 3 deletions npm/webpack-dev-server/src/aut-runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@ export function init (importPromises, parent = (window.opener || window.parent))
Cypress.onSpecWindow(window, importPromises)
Cypress.action('app:window:before:load', window)

beforeEach(() => {
// This really have no sense for @cypress/react and @cypress/vue
// It is anyway required to mount a new
// Before all tests we are mounting the root element, **not beforeEach**
// Cleaning up platform between tests is the responsibility of the specific adapter
// because unmounting react/vue component should be done using specific framework API
// (for devtools and to get rid of global event listeners from previous tests.)
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is a little confusing to me - Just for my understanding, unmounting the component won't actually get rid of the global listener (REACT_DEVTOOLS_GLOBAL_HOOK) though, right? It will just make sure the component is no longer present in the devtools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean if you are doing document.addEventListener inside the useEffect and unsubscribing in return

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, sure. Makes sense. One thing we might need to be careful is if the devtools do window.addEventListener('postMessage'). Vue DevTools do this - we need to clean it up because window.addEventListener` will not replace the previous one but duplicate it.

before(() => {
appendTargetIfNotExists('__cy_root')
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need to fix vue with using this root too. Right now, @cypress/vue's mount function creates it's very own root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did change this so it is matching the old react behavior (that I deleted here) – but yes, I do want to test this in scope of devtools PR

})

Expand Down