Could we replace find().exists() with exists()?

[afontcu]

Hi, folks! 👋

The main goal of find() is to assert something exists. However, we should be aware that

expect(wrapper.find('#whatever')).toBeTruthy()

is dangerous, as it might lead to false negatives. This is happening right now, for instance, here https://github.com/vuejs/vue-test-utils-next/blob/master/tests/features/transition.spec.ts#L9.

Just to prove the point, replace line 7 with expect(wrapper.find('#message')).toBeTruthy() and the test still passes (even though "it shouldn't"). btw, I just submitted a PR to fix improve the assertion: #139.

State of the art

• find() + toBeTruthy() is dangerous.
• In docs, we suggest using find() + exists() to assert whether if it does exist (or not). get() is the way to go to get an existing element.
• thus, AFAICT find() does only make sense when bounded to exists().

Suggestion

Given that scenario, would it be sensible to ditch find() and make exists() behave like find().exists()?

To maintain some consistency, what if findComponent became getComponent? So:

• get()/getAll(): gets you an (or an array of) element or throws.
• getComponent(): gets you a component or throws.
• exists(): returns whether if the query for an element succeeded.
• find/findAll/findComponent would disappear from the public API.

Doubts? Suggestions? Am I missing some use case for find? 🤔

[dobromir-hristov]

Removing find is a pretty steep breaking change. Do you mean ditch from codebase or docs? What about findComponent.

Also according to this, exists will work like 'contains', if passed parameters. Would this work on querySelector type DOM queries only, or can we assume that findComponent type queries would also work?

[afontcu]

Thanks for sharing your thoughts! 🙌

Removing find is a pretty steep breaking change

It is def a breaking change. The migration would be find() -> get() or find() -> exists(). Not sure if there's a way to narrow the "algorithm" down, but: if find() is followed by exists(), then exists(). If followed by any other method, then get().

What about findComponent

Good question. getComponent?

Would this work on querySelector type DOM queries only, or can we assume that findComponent type queries would also work?

to be honest, findComponent is not something I spend much brain power on 😅 as mentioned above, it could be renamed to getComponent if we want to keep a method that returns a VueWrapper, and even a existsComponent would make sense to match exists. Just thinking out loud, here.

[lmiller1990]

I have noticed that ErrorWrapper is still a truthy value. I can see why this leads to bugs like the one you described.

I agree in theory - with get, find it not very useful except for specifying an element does not exist. We also had a contains method in VTU too, which is similar to the exists you described 🤔

The main problem I foresee is what @dobromir-hristov described - some codebases exclusively interact with components (via findComponent). Eg findComponent(Foo).vm.$emit. Good or bad, this feels like too big a breaking change.

I'm not sure it makes sense to add an entire new API for just one use case. VTU v1 had a pretty large API, mainly due to adding methods for specific use-cases.

I will keep thinking about this.

[afontcu]

some codebases exclusively interact with components (via findComponent). Eg findComponent(Foo).vm.$emit. Good or bad, this feels like too big a breaking change.

To maintain some consistency, what if findComponent became getComponent? So:

• get(): gets you an element or throws. Same thing with getAll.
• getComponent(): gets you a component or throws.
• exists(): returns whether if the query for an element succeeded.
• find/findAll/findComponent would disappear from the public API.

Adding existsComponent() would also make sense in terms of completeness, even though I'm not sure it is that useful 🤔

I'm not sure it makes sense to add an entire new API for just one use case

Totally agree! That's why I suggest merging find().exists() into exists(), and remove find() :)

[lmiller1990]

This makes a lot of sense to me. I think this is still a very significant breaking change, and should be considered very carefully. As someone looking to improve VTU, I love it. As someone with a large Vue app using VTU heavily, I'm not sure. @dobromir-hristov probably has a better perspective, I know he works on a very large Vue codebase.

Another option is extending get: wrapper.get('#foo', { error: false }). You could use it like find. 🤷‍♂️

Finally, we could continue to have find, to help people upgrade. If we did make this breaking change. Or just leave them, but don't document them.

I will think about this a bit more. I definitely agree the new API you suggested is better, but I feel like people did not respond well to the breaking changes with v1.

I would like to get other opinions and perspectives. Getting feedback about this is quite challenging!

[dobromir-hristov]

One thing i see allot is in beforeEach ppl mount the wrapper and find 3 4 main elements they work with and save to vars.

Then they assert either existence or attributes in each test, sometimes calling setProps at the beginning of the test. It makes for some very minimal tests, that ofc have their limitations.

Using get would make this approach instantly broken, as it will throw in the beforeEach.

[dobromir-hristov]

I like this idea tbh, we can always emphasize this is the recommended way to do things from now in, and leave find as an advanced use case, lets say.

[lmiller1990]

If we decide to go ahead with this, we should make an RFC.

One thing i see allot is in beforeEach ppl mount the wrapper and find 3 4 main elements they work with and save to vars.

I didn't think of this. I have seen these kinds of tests (mainly in VTU bug reports and #testing in Vue-land):

let foo
let bar
let wrapper

beforeEach(() => {
  wrapper = mount...
  a = wrapper.find('a')
  b = wrapper.find('b')
})

This would no longer be possible with the above API. This seems like a blocker... have both find and get, which do similar things but solve different problems is a code smell, to me.

[afontcu]

Thanks all for the input!

This would no longer be possible with the above API

Yep. However, it is not that VTU fails to cover a use case. It is true, though, that migrating from this kind of setup would not be as straightforward as search&replace.

If we decide to go ahead with this, we should make an RFC.

Do you want me to go ahead and write a proper RFC? I feel we've shared some concerns/ideas and iterated the initial proposal a bit, so I'm OK with writing that up even if it ends up being rejected :)

[lmiller1990]

Hm... how can you accomplish the above with the new API? You would need to change the tests to redeclare foo and bar locally for each test. This really forces people to write their tests in a certain style. While I personally don't mind opinionated libraries, I'm not sure that is the direction an official Vue library has the luxury of taking.

An RFC is definitely the next logical step - not many people will see this discussion, but an RFC will definitely gain more attention.

I would recommend drafting the RFC somewhere (here, notion, where-ever) and getting feedback on it first - that way we can try to anticipate and prepare some responses. What do you think?

[afontcu]

While I'm not entirely sure this isn't a direction VTU should not take (it is as "mandatory" as any other decision we made) I understand why this would trouble some users and make it harder for them to upgrade to v2.

I'm still up for this change, but I agree we could explore other alternatives that would led to a more ergonomic API.

Would something along the lines of wrapper.get('#foo', { error: false }) (whatever the option name would be), as @lmiller1990 mentioned, work?

let wrapper, a, b

beforeEach(() => {
  wrapper = mount...
  a = wrapper.get('a', { error: false })
  b = wrapper.get('b', { error: false })
})

This would make this option way less relevant (it's way more hidden than having find()), so people and docs could focus on get() while also providing the ability to use the beforeEach strategy.

---

re RFC, totally agree that having a draft here is wise 👍 . I'll take care of it once we've decided which version (if any) we're more comfortable with, and which are the alternatives :)

[cexbrayat]

The tests could still be fine with something like the following, no?

beforeEach(() => {
  wrapper = mount...
-  a = wrapper.find('a')
+  a = () => wrapper.get('a')
-  b = wrapper.find('b')
+  b = () => wrapper.get('b')
})

it('should get a, () => {
- expect(a.text).toBe('Hello');
+ expect(a().text).toBe('Hello');

[afontcu]

The tests could still be fine with something like the following, no?

beforeEach(() => {
  wrapper = mount...
-  a = wrapper.find('a')
+  a = () => wrapper.get('a')
-  b = wrapper.find('b')
+  b = () => wrapper.get('b')
})

it('should get a, () => {
- expect(a.text).toBe('Hello');
+ expect(a().text).toBe('Hello');

Thought about that, too! (this is similar to how I write tests with Testing Library)

However, I'm afraid expect(a().text).toBe('Hello') would throw anyway if the element doesn't exist. You could write something like expect(a()).toThrow(), but the API looks a bit off.

[lmiller1990]

Small update! Evan mentioned that it would great to get all the core libs to move to beta sooner rather than later.

I am thinking we should probably keep breaking changes to a minimum for VTU v2. While the APIs here seem like they might be better, I don't know if we have a lot of time to make breaking changes. I would be much more in favor of additional, optional improvements where possible, instead of breaking changes.

Did anything come from this? Seems like a lot of cool ideas, does anyone have a specific one they would like to peruse?

[lmiller1990]

With all the other core packages going to beta (Router, Vuex)... we should also be heading to beta soon. I think we should not making any more API changes for the time being; we can add additional methods, and deprecating existing ones, but that will not be prior to 2.0.0.

[afontcu]

Agree!