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

fix(findComponent): return root instance if it matches #834

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

xanf
Copy link
Collaborator

@xanf xanf commented Aug 6, 2021

When we're using findComponent on recursive component, the result is pretty unexpected - if our selector matches root - it will return first nested instance. Considering #211 I've made sure that if root matches - it always takes priority

This resulted in failing test which literally expects other behavior. After some consideration and verifying this is behavior of vue-test-utils v1 CodeSandBox - I've dropped offending test intentionally (@dobromir-hristov will be cool if you could look and approve that)

@dobromir-hristov
Copy link
Contributor

dobromir-hristov commented Aug 6, 2021

Wait, this is a breaking change right? If I want to find the child, not get the same component I am querying from, right?

So if we have a Child that is the root of Parent, if we search for Child, we would get the Parent back instead?

const parentWrapper = mount(Parent)
const childWrapper = parentWrapper.findComponent(Child) // what would `childWrapper` be? The Parent or the Child?

@xanf
Copy link
Collaborator Author

xanf commented Aug 6, 2021

@dobromir-hristov could you please explain a bit. If Parent !== Child nothing changes. if Parent === Child - yes, you will receive wrapper around same component

@dobromir-hristov
Copy link
Contributor

Ah ok, then why did that test fail? The child was different than the parent.

Also, how should someone fetch the child component if it's the same as the parent? Like in your recursion example? Just curious.

@xanf
Copy link
Collaborator Author

xanf commented Aug 6, 2021

it('finds a component when root of mounted component', async () => {
    const wrapper = mount(compD)
    // make sure it finds the component, not its root
    expect(wrapper.findComponent('.c-as-root-on-d').vm).toHaveProperty(
      '$options.name',
      'ComponentC'
    )
  })

Here we were mounting component D, which render componentC as it's root:

const compD = defineComponent({
  name: 'ComponentD',
  template: '<comp-c class="c-as-root-on-d" />',
  components: { compC }
})

These two obviously share same element and both of them match .c-as-root-on-d selector. Previously we will return child component as match, now we return parent component (which also matches)

Regarding your question - how to reach child component - well, if there are no other ways - by calling findAllComponents for example.

To be honest, I'm not happy with current behavior - mostly because querySelector / querySelectorAll never include root, but:

  • this is how VTU v1 works. I can leave with removing this behavior, but removal needs to be consistent (basically that means reverting fix: allow finding root component #214
  • wrapper.find(Foo).vm === wrapper.vm is poor-man's replacement for removed .is() from VTU v2. This could be addressed by backporting .is (only for checking component type) in the same way I've done when reviving it for v1

@lmiller1990
Copy link
Member

I agree we should match the V1 behavior - it looks like that's the goal of this PR. Seems ✅ to me - @dobromir-hristov do you have any other concerns/questions? I know you use VTU on some pretty big code bases, do you see any problems? Keep in mind the goal should be V1 compat where-ever possible (even if it doesn't really make sense).

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.

Seems fine to me - will wait on @dobromir-hristov to see if he has any further feedback.

@lmiller1990 lmiller1990 merged commit 20f08d4 into vuejs:master Aug 12, 2021
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.

None yet

3 participants