Do not allow calling `exists` on `get` and `getComponent` because it is already implied

[angelogulina]

This Issue relates to the discussion started here.

I noticed that get would introduce the same tension with find that it's currently occurring between find and at (on v1) that is, throwing an error. Maybe I am missing some context that justifies the choice and I'd be glad to know more. Further I try to explain why this would seem odd to me.

The docs example suggest a usage of get (note that the example uses getComponent but it's listed under get):

test('get', () => {
  const wrapper = mount(Component)
  expect(() => wrapper.getComponent('p')).toThrowError()
})

• the assertion does not seem to describe that a p Component is not supposed to be there (to exist)
• it is rather testing the library internal implementation or at least hardly depending on (the knowledge of) it

For sure using the library requires knowing the API but it should not require knowledge of the internal implementation and this last one should not be the target of a client test. Now,

• the signature of get suggests it returns a DomWrapper
• TypeScript would suggest the same
• similarity with find (stated in the docs) would suggest the same
• there are only two places I can find to know that get throws an error: the source code and the examples is the code.

I might be wrong and happy to review my thoughts but the source code (which is internal implementation) should not generally be the source of my tests, even less the subject of them. As for the docs, part of my point is that this example should not be there, as it suggests relying on the internal implementation of the method.

If replying to this gets too big, I'ma happy to open an Issue fo this in the v2 repo.

Thanks a lot for your time 🙏 really appreciate it.

[lmiller1990]

I agree, this example is bad. I am surprised to even see this in the docs.

I think it should read like this:

test('get', () => {
  const wrapper = mount(Component)

  wrapper.get('span') //=> found; returns DOMWrapper
  wrapper.get('[data-test="span"]') //=> found; returns DOMWrapper, fails if no matching element is found
})

Basically:

• always use get except when you as asserting something doesn't exist
• in that case use find + exists combo

Can you update the docs?

[angelogulina]

@lmiller1990 oh, just saw the reply, thanks! I'll take care of the docs 💪

[cexbrayat]

I agree with @lmiller1990 , the docs should not have this usage.

We used to have different signatures in TS that reflected more what was happening. find used to return VueWrapper | ErrorWrapper whereas get returned VueWrapper. So it was clear that when calling find you might have get an error wrapper. get was removing the need to add non-null assertions in your test code: wrapper.find('h1')!.text() vs wrapper.get('h1').text() (note the ! with find) .

It's indeed weird to have the exact same return type now 🤔 @lmiller1990 I think it was lost when we introduced the proxy createWrapperError but maybe we lost a bit too much info in the signatures? WDYT?

[afontcu]

+1 at "dunno how we did end up here", and it is possibly due to… me 😂

always use get except when you as asserting something doesn't exist. in that case use find + exists combo

This is it 👌

Thank you for rising this up! 👐

[lmiller1990]

Introducing the proxy createWrapperError does appear to make the types less specific. The createWrapperProxy definitely makes the codebase cleaner and easier to work on, though, which I think is valuable.

I wonder if we have both (using createWrapperProxy, but also better types).

[angelogulina]

@lmiller1990 @cexbrayat @afontcu thanks all for the feedback.

get was removing the need to add non-null assertions in your test code: wrapper.find('h1')!.text() vs wrapper.get('h1').text() (note the ! with find).

I see. I'm not being able to tell the difference at the moment trying few examples in the code base but I might miss something so not sure. Glad to have more insight here, if you want and have time.

Getting back to the API, the PR for the docs is out.

TL;DR

To make the docs suggestion consistent, I would also suggest to patch the return type of get with something like this:

type NonexistentDomWrapper<T extends Element> = Omit<DOMWrapper<T>, 'exist'>;

get<K extends keyof HTMLElementTagNameMap>(
  selector: K
): NonexistentDomWrapper<HTMLElementTagNameMap[K]>
get<K extends keyof SVGElementTagNameMap>(
  selector: K
): NonexistentDomWrapper<SVGElementTagNameMap[K]>
get<T extends Element>(selector: string): NonexistentDomWrapper<T>
get(selector: string): NonexistentDomWrapper<Element> {
  const result = this.find(selector)
  if (result instanceof DOMWrapper) {
    return result
  }
  throw new Error(`Unable to get ${selector} within: ${this.html()}`)
}

(naming inspired by this debate).

Note: this will still throw but it prevents from using exists, given we want to discourage its usage paired with get.

Longer explanation

A note. Introducing the suggestion (always use get except when you are asserting something doesn't exist) is a good rule of thumb but I think it stil can be misleading in three ways:

1. given how the current signature is placed, TypeScript won't complain if you use exists
2. it does not exclude that you can still use get if you want to assert something does exist
3. formally is not 100% correct, as vue-test-utils cannot actually say that something does not exist; the library has only the exists verb

Let me try elaborate with an example:

Given this code:

const wrapper = shallowMount({});

const fExist = wrapper.find('p').exists();
const gExist = wrapper.get('p').exists();

Assuming no p exist, as per point 1), TypeScript would not complain about calling exists on both though one invocation will throw an error.

Assuming p exists, as per point 2), I can actually use get and exists (which is not excluded by the rule of thumb stated above) but then comes 3): I cannot actually use find to say something does not exist. I can of course use find and exist in a test library assertion to say that exist would return false (or true).

But here's te thing, why should I be able to do:

expect(wrapper.get('p').exists()).toBe(true);

and not:

expect(wrapper.get('p').exists()).toBe(false);

If I'm not able to do the last one, I shouldn't be able to do the former one as well, thence my suggestion above.

WDYT? 😄

[cexbrayat]

@angelogulina I completely agree that the typings are misleading here and should be better 👍
Your proposition makes sense, maybe I would built it the other way, with get returning DomWrapper and find returning DomWrapper & { exists } or a better name, but that's a minor details. Anyway I like your proposition 👌

We are kind of circling back to the proposition of @afontcu of just having get('p') and exists('p') (and remove find that doesn't bring much except for compat with VTU v1) :)

[lmiller1990]

Should we just add exists then? If it lets us have better TS integration that'd be great, and it might simplify things overall.

We need to keep find for a few reasons:

1. you might want to assert 3 of some element are rendered. We don't have a getAllComponents like we have findAll and findAllComponents (should we have this?)
2. backwards compat (that's fine though, we could just recommend against using it in docs)

I think now is probably a good time to sort this out once and for all. I see 3 cases we need to solve, and it'd be nice to have 1 "official" way to do things:

1. getting a DOM element or component you know exists and doing something with it
2. asserting something exists
3. asserting something does not exist

If we add exists, we can solve all three very easily:

1. wrapper.get('#el').trigger(...)
2. wrapper.get('#el') (implicitly asserting that the element exists)
3. expect(wrapper.exists('#doesnt-exist')).toBe(false) (you could also use exists for the above assertion if you like to be explitic)

I wonder if ErrorWrapper is a good ideal still. It let's us have very nice error messages, but also makes some things a little complex (like find never returning a falsy value).

I would welcome improvements here; we should definitely be clear on what we want to change before we do so, though.

[angelogulina]

Thanks all for this great discussion.

TL;DR

• find should not be replaced by exists
• ErrorWrapper should be kept (maybe renamed, for find to EmptyWrapper)
• get should throw in the non-existing case but should not admit calling exist in the existing one
• as a last resort, if introducing a verb for the wrapper for asserting existence, I would suggest to use contains
• only in the last case, remove exist from everywhere else

The longer explanation

I see 3 cases we need to solve...

I agree this is a compelling reason for having an API that makes the user capable of asserting (non-)existence.

... having get('p') and exists('p') (and remove find... )

Though I see no compelling reasons to have exists replacing find for a very naive argument: it forces the user to a counter-intuitive API, that is reversed in comparison to how the verb exist is used in ordinary language.

... find never returning a falsy value

I agree that find never returning falsy values can be odd but I'd say that get as well does not return them: when something is not there, it throws. But there's a (justified) difference between the two...

I wonder if ErrorWrapper is a good ideal still...

It's funny that I started this discussion thinking get should be more similar to find but I'm now convinced of the contrary 😄 I'm not a native English speaker so I might be wrong but I'll try:

1. in ordinary language we use find and get differently: if I ask you to find me X in the closet, there's an implication that you might not find X; if I ask you to get me X in the closet, there's an implication that X should be there or we might say that it corresponds to if you find X, get X;
2. the docs already reflect this by stating the rule only use find when you want to assert things do not exist;
3. the code already reflects this rule by having find and get behaving differently;

What I propose:

• in respect to 1): get correctly implies an element should exist so that if we cannot get something (because it's not found), we should signal an error occurred;
• in respect to 2): we should generalise it by saying only use find for asserting existence of things;
• in respect to 3): we should enforce the behaviour by removing the possibility to call exists on the result of get (either by using the Omit strategy or the augmentative one) and maybe in the future by adding a throw annotation for the type;
• in respect to ErrorWrapper, I do think it is a good idea; it somehow resembles a (Maybe) Monad, a great abstraction to avoid breaking the flow. I might only suggest that find could return an Empty- and not an Error- Wrapper as not finding something (point 1)) is actually not an error. But this is minimal as what's exposed to the user is the behaviour and not the naming.

As a last point, if there's an interest in making a method available at the wrapper level to assert that something exists, this method should be called something like contains (wrapper.contains('#non-existent-thing')) and returning only true or false. This API would be more intuitive and similar to ordinary language. In this case, and this case only, this should be the only method for asserting existence of something (so exists should be removed).

I hope I was capable to explain myself enough.

[lmiller1990]

I thought about this a little more and I am worried things are going to get more complicated. I'll keep my comment as short as possible. My TL;DR is while the API could probably be more intuitive (as explained very well above by @angelogulina), my main priority will always been a stable API and as few breaking changes as possible.

find should not be replaced by exists

Agreed. Intuitive or not, this is how it has worked for years and now is not the time to change it.

ErrorWrapper should be kept (maybe renamed, for find to EmptyWrapper)

Agreed again for the same reason as above. I don't see very much benefit in adding more complexity with an additional wrapper, though.

get should throw in the non-existing case but should not admit calling exist in the existing one

Are you saying we cannot do wrapper.get('#blah').exists? I guess it there is no real point to call exists on a get call - I am not sure we should prevent this though. get returns a wrapper so why not let people call all the usual wrapper things (we could have a useful warning here, explaining they do not need to).

as a last resort, if introducing a verb for the wrapper for asserting existence, I would suggest to use contains
only in the last case, remove exist from everywhere else

We actually has contains until recently and deprecated it with the reasoning as "just use find or get". I wonder if this was a bad decision. The goal was to have "one way of doing things" and since you can accomplish this with find + exists, contains was removed. Maybe this was a bad decision? 🤷‍♂️

---

Good discussion. I don't see a clear actionable (yet) other than considering bringing back contains and maybe some useful warnings when calling get + exists.

I realized I never noticed the unintuitive TS typings since I never use get, but always use find + exists (out of habit, I guess). But since you can do this with find and exists, I am still not sure it makes sense (since the initial goal of removing it was to keep things lean and simple).

[angelogulina]

@lmiller1990 as I agree on all the rest you said, I'll just reply to:

Are you saying we cannot do wrapper.get('#blah').exists? I guess it there is no real point to call exists on a get call...

Exactly, that's what I'm proposing.

get returns a wrapper so why not let people call all the usual wrapper things...

Because get does not always return a wrapper (find does even in the falsy cases) and there's no explicit hint for this in the code. That's why, unless opting for a Either-like strategy, I would just prevent it use in the get cases (which would also comply to the recommendation in the docs).

We actually had contains until recently and deprecated it with the reasoning as "just use find or get"...

I didn't mean to propose reintroducing contains. I think find + exists is enough.

Ultimately, I don't want to push this so hard so I'm fine if you don't see any actionable point here. Just wanted to make clear the reason I proposed to change get signature.

[lmiller1990]

I understand now that I took day away and came back. I think it makes sense that get should not allow exists. Non TS users might find this surprising (we need a good warning to let people know). Man, I wish we didn't have so many ways to find/get components, lol.

Ok, so we agree that getComponent/get should not have exists. I see two things we need to consider:

• TS users. They should not see get in the suggestions.
• non TS users.

Is there a actually implement this without making the codebase a lot more complex? I am not super strong in terms of TS, I am guessing we need to just change the return type of get to be some kind of wrapper with all the other methods except get? Can we use a fancy Omit or Pick or something (I think I saw those used for this kind of thing before).

I changed the title of the issue (please confirm we are on the same page). :)