Remove WrapperArray

[38elements]

What problem does this feature solve?

IMHO,
Since WrapperArray has only methods which is the small wrapper method of Array,
it is not absolutly necessary.
I think that the Array of Wrapper is easier to understand than WrapperArray.
I do not think the methods of WrapperArray are often used.
By removing WrapperArray, it become simple and small maintenance cost.

What does the proposed API look like?

findAll() returns Array of Wrapper and WrapperArray is removed.

[eddyerburgh]

I think we should keep WrapperArray as part of the library.

The reason for a WrapperArray is that it improves error handling, which makes the library more user friendly.

With an array, if you try to access a wrapper that doesn't exist:

findAll('div')[1].props() // throws error Cannot read property 'props' of undefined 

With a WrapperArray:

findAll('div').at(1).props() // throws error [vue-test-utils]: no item exists at 1

We could improve the error message to include the selector that wasn't found:

findAll('div').at(1).props() // throws error [vue-test-utils]: no div exists at index 1

I think the WrapperArray is useful, just for this reason.

[38elements]

Thank you for reply.
I understood the intent to exist WrapperArray.

IMHO,
Mistaking findAll() and find() may be few.
Since most people often use Array, They may be used to error of Array.

Since there is WrapperArray.wrappers, findAll() returning an array of Wrapper is not absolutely necessary.
I think if moderate number of people need WrapperArray, WrapperArray is necessary.

I think it is difficult to change this, after version 1.0 released.

[phanan]

I tend to agree with @38elements. Personally, I don't find much use of the WrapperArray. The fact that it's named as Array when actually an object containing an array or Wrapper is pretty confused, too. That it throws a descriptive error message being the only benefit doesn't really justify the maintenance IMO.

[eddyerburgh]

An error message thrown by vue-test-utils lets you know there isn't an error in your test code.

This makes the process of writing tests much quicker. Instead of wondering if you used findAll incorrectly, you know the component you're testing didn't render the node.

[phanan]

I'm not sure if

findAll('div')[1].props() // throws error Cannot read property 'props' of undefined

is not clear enough. findAll('div')[1] being undefined would immediately suggest that the item doesn't exist at position 1, which is exactly what WrapperArray would throw: no item exists at 1. Anyways, having a wrapper only to throw a (subjectively) more meaningful error message isn't really worth it IMO.

Looking at the current implementation of WrapperArray, I see two main issues:

1. Real methods (those that don't throw an error) like contains() and exists() are just very thin (one line of actual code) wrappers around Wrapper, which can be achieved easily without the need of the WrapperArray itself. This goes against the philosophy of not adding helpers or aliases.
2. On the other hand, there are actually quite a few of "fake" methods (those simply throw an error saying "use at(i) to… instead"). These IMHO don't even need to be there and are adding maintenance debt as well as creating inconsistency.

Where I come from (backend frameworks), find() always returns an object, and findAll a collection. When I call findAll() I'd naturally expect a collection that allows me to traverse around and call the methods of the contained items. This goes back to @38elements's suggestion which I'd tend to agree: findAll should just return an array of Wrappers. Now I'm aware of WrapperArray.wrappers which is the actual array of wrappers (isn't that confusing btw?), but this fact only makes WrapperArray less important and more a candidate for removal.

[38elements]

If the return value of findAll() is changed,
Changing from Array to WrapperArray is easier than changing from WrapperArray to Array.
I think it is better to select Array of Wrapper before releasing version 1.0.

[eddyerburgh]

I'm strongly against returning an array from findAll.

Let me put down my thoughts.

Benefits of WrapperArray

• Improved error handling

I'll go into this in more detail.

Downsides of WrapperArray

• Unfamiliar API
• Difficult to filter

The API can be a problem for new users.

The second issue, that it's difficult to filter, can be fixed by adding a filter method. #336

Note an array exists on a WrapperArray object for users who want to use arrays rather than the WrapperArray object

Improved error handling

When you use at to access Wrappers in the WrapperArray, you stay in vue-test-utils. This means we can handle common errors, like findAll not returning any matching nodes.

If you follow TDD, you write the test before you right the code. Say I'm testing that all the second <p> tag in my component is hidden. I would write a test:

const pTags = wrapper.findAll('p')
expect(pTags.at(1).visible()).toBe(true)

When I run my test I'll get the error [vue-test-utils]: No <p> tag exists at index 1. Great, I know the test is failing because I haven't added any

tags to my component.

If we returned an array from findAll, the error message is not clear:

const pTags = wrapper.findAll('p')
expect(pTags[1].visible()).toBe(true)

When I run my test I'll get the error Cannot read property 'visible' of undefined. I might need to go look at the code to make sure I didn't add a typo to findAll.

That custom error saves me from having to check my code is correct after I run the test. When the custom error is thrown, I know it's failing because the component code is incorrect, not because the test code is incorrect.

Imagine you're refactoring a component. You remove an tag without realizing, and you get some failing tests with a message of [vue-test-utils]: No <a> tag exists at index 1. Without needing to look at the test, you realize that the tag you removed was necessary for the component to behave correctly.

If findAll returns an array, you'll get a TypeError Cannot read property 'setProps' of undefined. You need to dig into the test code to understand what went wrong.

Similar APIs

enzyme find returns an array-like object. To access individual nodes you use the at method. If only one node exists, the method is called against that node. If multiple nodes exist, it will throw an error, telling you to use at to access the node.

wrapper.find('div').at(2).hasClass('some-class')

jQuery find returns an array-like object. You can access nodes using the index.

$wrapper.find('div')[3].hasClass('some-class')

document.querySelectorAll returns a NodeList. A NodeList is an array-like object that includes methods like item to access an item. You can access individual items using [i]. You can also access items using an index.

element.querySelectorAll('div').item(3).classList.contains('some-class')
element.querySelectorAll('div')[3].classList.contains('some-class')

Methods that throw errors

The methods on a WrapperArray that throw errors are there to make users aware of methods that can't be called on a WrapperArray, and to show them how to access a node using the at method.

A user might be using findAll and assume they can call props on the WrapperArray. If the method threw an error like pTags.props is not a function, the user might think there's a problem in their test code. When we throw an error—props must be called on a single wrapper, use at(i) to access a wrapper— the user can see straight away what they did wrong, and how to solve it.

Utility methods

Apart from at, and methods that throw errors, a WrapperArray contains a number of utility methods that call the corresponding method on each of the child nodes.

These are methods like visible, exists, and is.

I'm not sure how useful these methods are, and we can discuss whether we should keep them or not. The reasoning behind adding them, instead of throwing an error that told users to access the wrapper with at was that the API is similar to jQuery, which calls a method on each of the items when you call a method on a jQuery object.

Conclusion

In my opinion, we should continue to make at the recommended way to access selectors, for the error handling benefits.

We should add more details to the docs to make the reasoning behind a WrapperArray clear.

To improve confusion with the API we could rename WrapperArray to WrapperCollection.

To improve usability we could add Wrapper methods like filter, and forEach.

[phanan]

Thanks for the very comprehensive argument @eddyerburgh. I'll go through each of them and add my counter-argument 😄

Improved error handling

The only benefit of WrapperArray as you listed, "improved error handling", is subjective. In fact, your examples are not really convincing, at least to me. Let's take a look at the first example:

const pTags = wrapper.findAll('p')
expect(pTags[1].visible()).toBe(true)

You're saying:

When I run my test I'll get the error Cannot read property 'visible' of undefined. I might need to go look at the code to make sure I didn't add a typo to findAll.

Not really. To me it suggests the same thing as that thrown by .at(): there's no such element found, which can only mean I haven't added either any element or the component itself if we're following strict TDD. Btw I don't think TDD has any value in this argument.

Similar APIs

I'm not familiar with enzyme, but from what I see, it only has find, which return a collection of nodes, and no such findAll(). This is IMO very consistent. The same goes with jQuery – $() rules it all. You have one API to remember, not two. This is the core difference from what we have here in vue-test-utils. Not to mention, jQuery's collection object is designed to behave pretty much like an array, with almost every higher-order function implemented – each, filter, map and more.

The same can still be said about document.querySelectorAll. While with querySelectorAll you have one more API to remember (which personally I almost always fail to remember and use querySelector mistakenly instead, and which I do hate), the returned object, as you said yourself, is very array-like. Which, again, isn't true with vue-test-utils.

Methods that throw errors

I don't think the purpose of these methods needs any explanation. The problems here are API chaos, inconsistency, and most importantly, maintenance, which remain untackled.

Utility methods

Again, these are few, and mostly only one-liner wrappers around the wrappers items. Their behavior can be implemented easily in userland, or via an additional helper library.

Conclusion

If we decide to keep WrapperArray, or WrapperCollection, we'll need to make it much, much more useful than it currently is. I'm talking about adding the load of array methods, to make it more array-like, and maybe to merge its API with Wrapper's. In other words, the benefits should overweigh the costs. Still, IMO it's not worth it, and should better go.

[eddyerburgh]

My preference—apart from keeping findAll— would be to remove findAll, keep find, but have it behave in the same way as find in enzyme.

That means it always returns a collection. If there's one node in the collection, the methods will be called on that node. If there are multiple nodes, it will throw an error and tell you to access the node with the at method.

[phanan]

My preference—apart from keeping findAll— would be to remove findAll, keep find, but have it behave in the same way as find in enzyme.

Mine as well. In short, let's make it behave similarly to other proven solutions, or don't have it at all.

[38elements]

IMHO, I seem collection makes it complex.
I think Wrapper and Array are simple.
I like Array.
I think if Array.at() is useful, it is adopted ECMAScript.
Since this is my subjective opinion,
I think if moderate number of people think collection is good, collection should be adopted.

I think changing wrapper.find() to wrapper.findOne() is better, since it is explicit.

[38elements]

wrapper.findAll() returns the following WrapperArray.
How about this ?

class WrapperArray extends Array {
  at() {
    // ...
  }
}

let wrapperArray = WrapperArray.from(wrappers)

[lusarz]

I agree with @38elements and @phanan that it would be nice to remove WrapperArray and return Array.
I think methods like hasClass, hasProp, hasAttribute may be confused for developers, because they use Array.prototype.every method - someone may expect that any of child has class, not every (Array.prototype.some) - why don't allow users to decide whether he want:

wrapper.findAll('p').some(item => item.hasClass(className))

or

wrapper.findAll('p').every(item => item.hasClass(className))

?

It's also not possible to chain, like:

wrapper.findAll('p')
  .filter(predicate)
  .forEach(doSomething)