Fix #1633 – feat(array wrapper): return ErrorWrapper when index not found with .at()

[angelogulina]

What kind of change does this PR introduce? (check at least one)

• Bugfix
• Feature
• Code style update
• Refactor
• Build-related changes
• Other, please describe:

Does this PR introduce a breaking change? (check one)

• Yes
• No

(probabaly?)

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

• It's submitted to the dev branch.
• When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
• All tests are passing: https://github.com/vuejs/vue-test-utils/blob/dev/.github/CONTRIBUTING.md#development-setup
• New/updated tests are included

If adding a new feature, the PR's description includes:

• A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

[lmiller1990]

Hm, this is probably a breaking change. What do we do at the moment? Just undefined? Is there a strong reason for this breaking change?

[angelogulina]

Hi @lmiller1990 and thanks for your kind reply. Yes, I think you're correct that it is a breaking change.

My mistake not to have linked explicitly the Issue where I develop the case with the compelling (at least to me 😂) reasons for wanting that.

Please, have a look here and feel free to ping me for any doubt or question!

[lmiller1990]

I see. The issue says "The API of at() would not change, instead its internal implementation would". Actually, I think this is a change in the public API (behavior is different).

This is probably a sensible change to make though. I have no idea if/how many people rely on this existing behavior. I would like some more opinions (extremely hard to get those, though). @afontcu @cexbrayat any opinion?

In VTU v2, we don't even have WrapperArray.at - it just returns an array of Wrapper. Another significant breaking change we should document. @angelogulina do you find this API good? Do you have an opinion on just returning an Array of Wrapper instead of a WrapperArray type with at accessors?

[afontcu]

This definitely looks like a (small) breaking change, and even though it makes sense, I'm not 100% it is worth the hassle 🤔

Could we make it opt-in through a config parameter? Call the option throwAtError (or whatever), and maybe other methods could benefit from it

[angelogulina]

Thanks again for the reply @lmiller1990.

v1

I see. The issue says "The API of at() would not change, instead its internal implementation would". Actually, I think this is a change in the public API (behavior is different).

The current signature of WrapperArray.at is defined as at(index: number): Wrapper<V>; so the API (in this strict sense) would not change although I agree with you that there could be people relying on the current behaviour of it and I do not wish to break this lovely library for anyone 😄

We might have two alternatives,

1. introducing a new method that is consistent with how other methods of WrapperArray work, namely returning the ErrorWrapper when a search is not successful. nth is what is generally used but I fear it might be confusing – why having two same methods that behave differently? take would be nicer but I the same fear applies, adding the fact that some libraries have a different API for it.

2. I'd be happy to reduce the scope of the issue/change. Conventionally, libraries that operates on Array have a head method, which I think could cover many cases. I'd propose first as it sounds nicer 🙈 and just for the sake of it, a last one.

3. add a config param to at as suggested here

I'd be happy to update the relevant issue and the MR if we find an agreement.

v2 and at

Do you have an opinion on just returning an Array of Wrapper instead of a WrapperArray type with at accessors?

Thanks for asking that!

Moving from WrapperArray to Array could bring some benefits, that is one could simply use Array methods and/or utils to describe their test cases. In this scenario, at doesn't really find a place.

(As a note, I found the introduction of BaseWrapper.exists to be a very useful concept with an eminent ancestry – but I'm fine with giving up on it for some situations, if removing WrapperArray brings simplification.)

v2 and get

Related to v2, I have a question about the introduction of get.

Move to here because it was getting too big.

[angelogulina]

Thanks for the feedback @afontcu!

I just edited my reply which contains also some alternative proposals. I like yours as well, so I'm gonna add it to the list.

[lmiller1990]

I am not a fan of an API like head or tail. It feels like it just adds more to learn: imo, ideally, we should just return an array of wrappers - any JS developer knows you can do arr[0], arr[1] etc - even at is confusing, does it start at 0? 1?

If I had to choose I would go with an additive feature, suggested above... that said, the concept of WrapperArray doesn't exist in V2 (yet, and I don't think it should for the reasons outlined above) so I am not sure there is any real merit introducing it here for a small number of codebases that would benefit from it.

Not going to lie, I do feel a little bad to turn down a feature request that is so well thought about and delivered, but it's really hard to justify any kind of breaking change when the rest of the core libs have been so stable for so long, and with Vue 3 coming out, I don't expect people will want to deal with any kind of break change at this point.

On the flip side, if you feel strongly about something, now is the time to get in done in V2, before we move to a release candidate!

[angelogulina]

@lmiller1990 thanks so much for the answer again 🎉

I see your point and there's no problem in turning down the feature – I understand the concerns.
I filled an Issue for the v2 on which we could maybe elaborate a bit more on the throwing error part (if you feel like).

Gonna close this one.