Behavior of find and findAll

[eddyerburgh]

Should find and findAll return all nodes including the root node, or excluding the root node

Including root node:

const compiled = compileToFunctions('<div><div /></div>')
const wrapper = mount(compiled).findAll('div').length // 2

excluding root node:

const compiled = compileToFunctions('<div><div /></div>')
const wrapper = mount(compiled).findAll('div').length // 1

I think it should exclude the root node, but would be interested to hear others thoughts

[Austio]

First glance, would expect it to include the root node (count of 2). What are thoughts for reason to exclude?

[eddyerburgh]

querySelector and querySelectorAll exclude the root node

[yyx990803]

I think including the root node would make sense, because .find() and .findAll are not called on the root node but the "wrapper"

[eddyerburgh]

Although methods are called on the wrapper, they refer to the root node:

const compiled = compileToFunctions('<div class="class" id="id" />')
const wrapper = mount(compiled)

wrapper.is('div') // returns true
wrapper.hasClass('class') // returns true
wrapper.hasAttribute('id', 'id') // returns true

[yyx990803]

Hmm, I still find inclusive match more intuitive, since the root node is also part of the template. More importantly, is there actually a case where omitting the root node would be useful?

[pearofducks]

I think this depends on whether your headspace considers the method being called on the 'wrapper' or the node itself.

Since we have wrapper.is('div') that suggests to me the user should think of the wrapper as a node, not containing a node.

Are there other places where the user would think of the wrapper object as a node? Or is it typically treated as a wrapper that contains the node?

(Not arguing one way or another, but suggesting the method names align to imply behavior the user should expect) :)

[eddyerburgh]

As @pearofducks says, this is a case of how we conceive of a wrapper.

I think we should treat it as a node, and not as containing a node. In my eyes, almost all methods treat the wrapper as a node:

wrapper.is('div') // checks node matches selector
wrapper.isEmpty() // checks node is empty
wrapper.hasAttribute('id', 'id') // checks node attribute
wrapper.trigger('click') // calls on the node

I can't imagine a use case where omitting the root node would be useful. But it seems strange to me to return the same div each time you call find:

wrapper.find('div').find('div').find('div') // would return the same div

Especially since we the find methods are so similar to querySelector and querySelectorAll

I think this would confuse users:

const compiled = compileToFunctions('<header><div><div></div></div></header>')
const rootDiv = wrapper.find('div')
rootDiv.findAll('div').length // 2

Maybe the answer is to include the root node when called on a vm wrapper, and exclude it when called on a DOM node wrapper.

[Austio]

Maybe the answer is to include the root node when called on a vm wrapper, and exclude it when called on a DOM node wrapper.

I agree, here is what i would expect while using find/findAll.

const compiled = compileToFunctions('<div id='foo'><div></div></div>')

wrapper.findAll('div').length // would expect 2

const rootDiv = wrapper.find('#foo')
rootDiv.findAll('div').length // Would expect 1

Something else to consider is have a separate method so that we don't have to context switch.

find
findAll
findChildren

[eddyerburgh]

@Austio I don't think we should add an extra find method. Two is enough.

I thought about it this weekend and I agree with @yyx990803 . We should include the root node, in every find and findAll call.

[callumacrae]

+1 to including the root node: if the root node is returned and the user wasn't expecting it to, it's clear what happened and easy to work around, while it's less obvious the other way round.

[rowanu]

Another vote for including the root node - as per @callumacrae's comment it's the least surprising thing to do. Anything else is getting a bit magical and will result in many GitHub issues being opened... 😬

While @eddyerburgh's chained find() example is at first a little confusing, it will be easy for a user to work it out for themselves.

[eddyerburgh]

I'm closing this again. We'll keep the behavior as it is

[rowanu]

Thanks for closing.

FWIW I ended up changing my tests so I didn't need this - It felt too much like testing Vue's functionality to be worth the change.