fix: remove last mounted component upon subsequent mount calls #24470
Conversation
|
Thanks for taking the time to open a PR!
|
npm/angular/src/mount.ts
Outdated
| function cleanup () { | ||
| activeFixture = null | ||
| // Not public, we need to call this to remove the last component from the DOM | ||
| getTestBed()['tearDownTestingModule']() |
There was a problem hiding this comment.
Do you think we need to be a bit defensive here? Eg
const teardown = getTestBed()['tearDownTestingModule']
if (!teardown) {
throw Error('Could not teardown component. The version of Angular you are using may not be officially supported').
// OR just return early?
return
}
teardown()There was a problem hiding this comment.
Surprised TS isn't erroring here with "might be undefined" - maybe we should make the TS setting more strict.
There was a problem hiding this comment.
I like the idea of throwing an error here. Since this is something that might break in a minor, it's best to fail early and loud. The logic in tearDownTestingModule is important to guarantee clean state for the next tests so I'm ok with erroring in this case.
There was a problem hiding this comment.
Addressed in ce2c587, also made the types more strict for npm/angular
| it("should mount", () => { | ||
| cy.mount(Counter); | ||
| }); |
There was a problem hiding this comment.
| it("should mount", () => { | |
| cy.mount(Counter); | |
| }); | |
| it('should mount', () => { | |
| cy.mount(Counter); | |
| }); |
There was a problem hiding this comment.
Addressed in ce2c587
…e-last-mounted-component
npm/angular/src/mount.ts
Outdated
| try { | ||
| (getTestBed() as any).tearDownTestingModule() | ||
| } catch (e) { | ||
| throw new Error(`Failed to teardown component. The version of Angular you are using may not be officially supported, visit https://docs.cypress.io/guides/component-testing/component-framework-configuration for current Angular version support.`) |
There was a problem hiding this comment.
Should this use an on link?
There was a problem hiding this comment.
Damn you onLinks!!!
|
@rockindahizzy added the onlinks here: https://github.com/cypress-io/cypress-services/pull/4937 Error thrown |
There was a problem hiding this comment.
Did manual testing Vue, works great.
Looking at Percy, we have 3 snapshots that rely on mount not being cleaned up, to catch all the variations at once. I actually like this behavior from a billing perspective lol.
Not blocking, I'll make a PR into with suggested updates to these tests into this branch if it is still open at the time, or into the freature branch if we've already merged this.
A few other small observations/questions that don't block merging.
| @@ -33,6 +33,9 @@ export function mount (jsx: React.ReactNode, options: MountOptions = {}, rerende | |||
| Cypress.log({ name: 'warning', message }) | |||
| } | |||
|
|
|||
| // Remove last mounted component if cy.mount is called more than once in a test | |||
There was a problem hiding this comment.
Is this comment only applicable in React 18 or should it be here as well?
// React by default removes the last component when calling render, but we should remove the root
// to wipe away any state
There was a problem hiding this comment.
Only applicable to 18, react17 doesn't have the concept of root but just a rerender into the same element.
| Cypress.vueWrapper?.unmount() | ||
| const el = getContainerEl() | ||
| const cleanup = () => { | ||
| Cypress.vueWrapper?.unmount() |
There was a problem hiding this comment.
If there's no Cypress.vueWrapper that would indicate it's the first mount, so possibly we just return here instead of touching the DOM?
Actually wondered, as part of cleanup do we want to unset Cypress.vueWrapper and Cypress.vue? Probably pointless since mount will overwrite them & cleanup is always called before mount.
There was a problem hiding this comment.
I'm going to remove the innerHtml = '', more consistent with other frameworks. I'll add the cleanup as well.
| @@ -266,6 +265,10 @@ declare global { | |||
| } | |||
| } | |||
|
|
|||
| const cleanup = () => { | |||
| Cypress.vueWrapper?.destroy() | |||
There was a problem hiding this comment.
Any need to do innerHTML = '' for the root el here? Curious why Vue 3 does it but Vue 2 does not
There was a problem hiding this comment.
See #24470 (comment)
User facing changelog
Subsequent
mountcalls within the same test will remove the last mounted component from the DOMAdditional details
Calling
mountmultiple times in a single test had different behavior between frameworks. Some (properly) removed the element, some mounted multiple components and another threw an error.This PR makes all of the frameworks behave consistently by destroying/removing the last component mounted if
mountis called again within a test.Steps to test
I relied on system-tests to test these changes, specifically the tests in
component_testing_spec.ts. I've added/tweaked themount.cy.{ts,js,tsx,jsx}tests forreact,vue,svelte,angularto verify the behavior.How has the user experience changed?
Screen.Recording.2022-10-31.at.11.35.40.AM.mov
PR Tasks
cypress-documentation?type definitions?