-
Notifications
You must be signed in to change notification settings - Fork 272
fix: return correct html depending if outer element is root Vue app #174
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
Conversation
html() { | ||
return this.parentElement.innerHTML | ||
// cover cases like <Suspense>, multiple root nodes. | ||
if (this.parentElement['__vue_app__']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__vue_app__
is only present on the root component no? Does it still work if the suspense or multi-root node component is not directly under the app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea if this will work with nested <Suspense>
, we should add a test and find out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine fc045f7#diff-7906b5e8a0a656a314ef4e609e16f76dR54
Is this what you meant @cexbrayat ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about an extra layer of component, something like mount(App)
with App
just displaying the Component
you have in your test. Because as is in your test, Component
is the root comp, so it has the __vue_app_
attribute I guess? But maybe I'm misunderstanding something: if you're confident that's OK for me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not that confident, better have a test. Can you give me an example of what you mean (doesn't need to be working). The way I read is it something like this:
const Comp = {
template: `
<ComponentA>
<ComponentB>1</ComponentB>
<ComponentB>2</ComponentB>
<ComponentB>3</ComponentB>
</ComponentA>
`
}
const App = {
template: `
<ComponentA />
`
}
mount(App)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this does NOT work
it.only('works', () => {
const ComponentA = {
name: 'ComponentA',
template: `<div class="comp-a"><slot /></div>`
}
const ComponentB = {
name: 'ComponentB',
template: `<div class="comp-b"><slot /></div>`
}
const Main = {
components: { ComponentA, ComponentB },
template: `
<ComponentA>
<ComponentB>1</ComponentB>
<ComponentB>2</ComponentB>
<ComponentB>3</ComponentB>
</ComponentA>
`
}
const App = {
components: { Main },
template: '<Main />'
}
const wrapper = mount(App)
expect(wrapper.getComponent(ComponentB).html()).toBe('<div class="comp-b">1</div>')
It cannot find ComponentB
at all. I don't know why (nothing to do this with PR though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made issue... #180
I still don't fully get how Vue stores the component hierarchy internally. Hm.
I think you can see this example, why |
@bastarder I am not sure, seems it's incorrect. Thanks for the extra tests - will try it out in the next day or two and find out 👍 Or you can investigate if you get time before me :D |
@bastarder I added your test case: fc045f7#diff-3e22d2badc1a61e2e80330efea26ef91R227 Turns out the problem is not VTU here, but |
thk! i will use |
Great, I will release this when we fix #180. Please wait a bit. |
This fixes one class of bug, but not another. For now I will merge this, and deal with the other problem in another PR. |
Feels a little hackish, but resolves #173. I just tried things until all the tests passed. TDD at it's finest!
@bastarder might want to give this a review - can you think of any other cases we missed?