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

bugfix/unmounting-on-multi-root #182

Merged
merged 3 commits into from
Aug 21, 2020

Conversation

jw-foss
Copy link
Contributor

@jw-foss jw-foss commented Aug 20, 2020

- Fix the logic which causes unmounting fail when it's multi-root component
@jw-foss
Copy link
Contributor Author

jw-foss commented Aug 20, 2020

I went through the mounting function and found out that the whole part of parentElement.removeChild(child) is kind of unnecessary when you eventually calling the app.unmount(element) method, so I simply removed the code here
https://github.com/vuejs/vue-test-utils-next/blob/e5aee88ec6437a7d28dd0a4a9d0e84bbe3a8df40/src/vueWrapper.ts#L219
and use app.unmount(this.parentNode) to remove mounted DOM.

@jw-foss
Copy link
Contributor Author

jw-foss commented Aug 20, 2020

@lmiller1990

Comment on lines +15 to +22
const wrapper = mount(Component, {
props: {},
global: {
config: {
errorHandler
}
}
} as any) // The type checking keeps complaning about unmatched type which might be a bug
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I tried it with

mount(Component, {
  global: {
    config: ...
  }
}

Type system seems not agree with this syntax when custom component is mounting with global options

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, will deal with the types later, it's outside the scope of this ticket. Great job!

Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Code looks good, just a small comment - let's keep the codebase boring and simple :)

Comment on lines +15 to +22
const wrapper = mount(Component, {
props: {},
global: {
config: {
errorHandler
}
}
} as any) // The type checking keeps complaning about unmatched type which might be a bug
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, will deal with the types later, it's outside the scope of this ticket. Great job!

const errorHandler = jest.fn()
const Component = defineComponent({
template: `
<div>${AXIOM}</div>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need AXIOM, it doesn't really add much to the test. I am not sure who Rem is.

Can we just have <div /><div /> or something?

Copy link
Contributor Author

@jw-foss jw-foss Aug 21, 2020

Choose a reason for hiding this comment

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

Yep, sure. I'll remove the text later

Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Great, I will do a release in the next few days with this feature.

@jw-foss
Copy link
Contributor Author

jw-foss commented Aug 21, 2020

Great, I will do a release in the next few days with this feature.

Nice! Thanks for the quick response though

@lmiller1990 lmiller1990 merged commit e3daca6 into vuejs:master Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unmount on multi-root component causes JSDOM error
2 participants