Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better API to trigger custom events #1588

Closed
ianlet opened this issue Jun 21, 2020 · 12 comments
Closed

Better API to trigger custom events #1588

ianlet opened this issue Jun 21, 2020 · 12 comments

Comments

@ianlet
Copy link

ianlet commented Jun 21, 2020

What problem does this feature solve?

Currently, when you want to emit an event on a custom component, you have to access the vm of that component and manually trigger the event. But when it is a DOM event, you can simply trigger that event.

For example:

<template>
  <div>
    <a @click="onLinkClick" data-test-id="link">link</a>
    <MyComponent @do-something="onDoSomething" data-test-id="my-component" />
  </div>
</template>
// the test

it('should do something and click on link', () => {
  const link = wrapper.find('[data-test-id="link"]')
  const myComponent = wrapper.find('[data-test-id="my-component"]')
  
  link.trigger('click')
  myComponent.vm.$emit('do-something')
  
  expect(wrapper.emitted('link-click')).toBe(true)
  expect(wrapper.emitted('do-something')).toBe(true)
}

It is confusing (especially for beginners) to have two different ways of triggering an event.
And also, as the test should not know implementation details, it makes the test more fragile.

In my opinion, as the expected outcome is the same in both cases, the end user should not have to think about that and should be able to use the same API.

What does the proposed API look like?

it('should do something and click on link', () => {
  const link = wrapper.find('[data-test-id="link"]')
  const myComponent = wrapper.find('[data-test-id="my-component"]')
  
  link.trigger('click')
  myComponent.trigger('do-something') // <--- same API as the link
  
  expect(wrapper.emitted('link-click')).toBe(true)
  expect(wrapper.emitted('do-something')).toBe(true)
}

If it's ok for you, I can open a PR to fix this.

@ianlet
Copy link
Author

ianlet commented Jun 21, 2020

FYI @arsenaultk9

@dobromir-hristov
Copy link
Contributor

While it is possible to do this on the VueWrapper, I am not 100% sure it is correct.

If you list to a @click.native on a component, $emit('click') will not catch it, and you wont be able to trigger it, unless you trigger click` on some of it's HTML elements.

In Vue 3 and hence VTU 2, we can do this, as there is no .native anymore. You are however encouraged to test those scenarios using the real component's elements, unless using shallowMount, in which case I understand your requirements.

@lmiller1990 any opinions?

@ianlet
Copy link
Author

ianlet commented Jun 21, 2020

Oh right, I forgot to mention I use shallowMount in this case. So the suggestion is related to shallowed components.

@lmiller1990
Copy link
Member

lmiller1990 commented Jun 22, 2020

I think the main problem we will run into is trigger currently just a wrapper for creating a DOM event: see here for how we create a DOM event and here for trigger. I think the original idea was trigger is supposed to simulate a user interacting with the component (click, keyup etc). A user cannot do a doSomething event.

I can see why this is confusing, but a component emitting an event is pretty difficult to a DOM event getting triggered; I am not sure how much sense this change makes 🤔

Regarding not testing implementation details; that means you should not do vm.$emit at all, but make the event get emitted by doing trigger on some element in <MyComponent>.

In this case of shallowMount, understandably, you cannot do this (thus the test is fragile, like you said). Using emit to emit and event seems less confusing, to me, than trigger in this case (you literally call this emit function to emit an event, that seems more explicit than using another method with a different name).

@ianlet
Copy link
Author

ianlet commented Jun 22, 2020

I think the original idea was trigger is supposed to simulate a user interacting with the component (click, keyup etc). A user cannot do a doSomething event.

Interesting! To me, as soon as an event is triggered on a component it means the user interacted with your app. No matter if this is something as low level as a click or high level as a <Tweet @like /> for instance.

And as a user of that Tweet component, I should not have to know how to trigger that click that will in turn emit like. All I want is a like so I can collaborate with it adequately.

It is even more true if the component is from an external library that I should not know how it is implemented so it can evolve without breaking my app. As long as the contract is not broken.

About the fragile test problem, here is another example: let's say I have a component using a button and I want to emit a custom event on the click of that button (or any other reaction):

<template>
  <div>
    <button @click="$emit('rainbows')" data-test-id="button">Click me</button>
  </div>
</template>

With the following test (with the wrapper being shallow mounted):

it('emits rainbows', () => {
  const button = wrapper.find('[data-test-id="button"]')
  
  const button.trigger('click')

  expect(wrapper.emitted('rainbows')).toBeTruthy()
}

It will work and you'll get 🌈. But as soon as you change the button, let's say, for a v-btn with a click event, the test will break. Even though the behavior did not change.

In this case, I know I could use mount instead of shallowMount because it is pretty trivial but with higher level components I prefer stubbing its dependencies (the other components) and rely on a strong contracts (the components API). But maybe this is a mistake?

@lmiller1990
Copy link
Member

lmiller1990 commented Jun 22, 2020

I see what you mean. I have one comment, and then we can move on and discuss the original issue (improving the API):

As long as the contract is not broken.

Sure, but what if v-btn changes to emit as clicked even instead of click, breaking the contract? Your app will be broken, but your tests won't tell you, because you are assuming that v-btn is emitting a click event (which might change).

mount will almost always be better for this reason - you are testing something that much closely resembles the actual application you are building.


Anyway, opinions aside, I assume you have some kind of extremely complex component tree where you cannot use mount. What is your proposed API? The problem I foresee is the additional arguments to trigger. For example if you would like to simulate a like event getting trigged with a payload of { likedByUserId: 1, totalLikes: 5 } you can currently do:

wrapper.find(Tweet).vm.$emit('like', { likedByUserId: 1, totalLikes: 5 })

trigger takes two arguments: the event, and the modifiers. See the docs. For example:

await wrapper.trigger('click', {
  ctrlKey: true // For testing @click.ctrl handlers
})

It may be difficult to combine these, since they both take an object of options as the second argument, but the usage is very different.

One alternative could be wrapper.find(Tweet).emit, as a short-hand for .vm.$emit.

@dobromir-hristov
Copy link
Contributor

I think we should not combine the two, as they serve different purpose in Vue 2, as I explained earlier.

But I also think adding emit is probably better.

@lmiller1990
Copy link
Member

I like wrapper.find(Foo).emit. I think we should add this. The less we need to use .vm, the better!

@ianlet
Copy link
Author

ianlet commented Jul 3, 2020

So wrapper.find(Foo).emit would be an alias for wrapper.find(Foo).vm.$emit? Should it also substitute wrapper.trigger and offer a way to support modifiers (such as your example with @click.ctrl)?

@dobromir-hristov
Copy link
Contributor

You would just call emit('click', payload).

As long as your component emits that, should be fine

@afontcu
Copy link
Member

afontcu commented Jul 13, 2020

I like wrapper.find(Foo).emit. I think we should add this. The less we need to use .vm, the better!

Hm, I feel introducing this API would set the direction for people to trigger events programatically, instead of interacting with the component "manually".

While it is true that some cases w/ shallow mount may require a test to trigger a custom event, I'd say wrapper.find(Foo).vm.$emit is a valid alternative for these cases (not that hidden after all!).

anyway, my two cents 😃

@lmiller1990
Copy link
Member

You could write this in VTU next with the plugin system. Maybe we should just backport this feature to this codebase as well.

Another alternative is add it yourself:

import { VueWrapper } from '@vue/test-utils'

VueWrapper.prototype.emit = (evt, ...rest) => {
  this.vm.$emit(evt, ...rest)
}

(not tested). Considering how trivial this (should) be to add, I think we just let the people who want it add it. I personally have a lot of util methods I use for my various Vue projects. Lean core + build your own abstractions seems like the way to go for maximum flexibility.

I thought about this a little more and am leaning towards @afontcu's side - although you save a few keystrokes, I think mostly it's better to be more explicit - vm.$emit is in the official docs, so I think using this is more clear and understandable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants