feat: added typehint to the trigger method

[nandi95]

closes #415

[nandi95]

There's probably more I just added the missing properties and saw that it was depricated.

[cexbrayat]

I like the idea 👍

[lmiller1990]

Do we need tsd tests? This is a pretty nice PR. I'm looking forward to having type hints for events.

[nandi95]

Do we need tsd tests? This is a pretty nice PR. I'm looking forward to having type hints for events.

What is there to test? The return value doesn't change depending on the argument.

[cexbrayat]

I think there are 3 options:

• we do not care about user defined events, remove this support, and change the signature to accept only DomEventName (and we can a tsd test to check that a simple string is not accepted)
• we want to keep the support but improve the typechecking, and we might want to split the signature to have two overloads (and maybe deprecate the one with string only?)
• we want to keep the support and not change much to the typechecking, and we merge as is (as DomEventName | string is accepted, TS can't really help).

[cexbrayat]

I just realized this is the signature in the base wrapper!
So maybe user defined events are useful when you want to trigger a custom event of a child component (VueWrapper), but not on a DOM element (DomWrapper).

Maybe we should have different trigger signatures in both wrappers then:

• one which accepts only DomEventName in DomWrapper
• one which accepts strings (or even better, only the events defined in emits by the component) on the VueWrapper

[nandi95]

That would be great actually (maybe stick an abstract signature into the base wrapper too).
However, It's a valid thing to do to emit your own event so dom wrapper should probably still be DomeEventName | string no?

As for the vueWrapper I'm not entirely sure how to get the emits: [] type information into the trigger method.

[cexbrayat]

I still think t would be better with overloads:

async trigger(eventString: DomEventName, options?: TriggerOptions): Promise<void>
async trigger(eventString: string, options?: TriggerOptions): Promise<void>
async trigger(eventString: DomEventName | string, options?: TriggerOptions): Promise<void> {

because then TS will pick the first signature when typing trigger('cli and offer click as the autocomplete.
But it will still allow a custom event as the signature falls back to string.
So that would be great for the DOMWrapper (and maybe for both)

For the VueWrapper, I think it's doable to get the emits and fall back to string as well, but I don't know exactly how of the top of my head. It's one of the generic parameters of ComponentPublicInstance. It's not a big deal if we don't do this, I just thought it would be ideal 😉

[lmiller1990]

I personally don't mind what we go for, I have never had a problem with the existing typings, but always good to have better IDE hints. This sounds pretty good to me:

one which accepts only DomEventName in DomWrapper
one which accepts strings (or even better, only the events defined in emits by the component) on the VueWrapper

Are we planning to make any of these changes here, or shall we merge this as-is, and create another PR with more improvements?

[nandi95]

Please carefully consider the latest commit 9bba382
To get the vue triggers to only accept the emits: [] events I had to update couple of tests so it uses the dom wrapper
I imagine this could also be the case in the wild. If I were to add | string to the vue wrapper it seems my ide cannot type hint anymore.
Also the vue wrapper typehinting only seems to work when using the defineComponent otherwise the trigger method accepts anything
As for the deprecation, of string is there a consensus on that?

Currently in summary:
DOMWrapper takes DomEventName | string
VueWrapper takes the event defined in the emits only if defineComponent is used otherwise anything

[cexbrayat]

Thank you for giving this a try. I'm sadly not sure this works in every cases, and not convinced by having to update tests to get the DomWrapper and trigger a DomEvent.

Two possibilities IMO:

• you find a way to handle all emits cases, and remove the need to get the DomWrapper
• or we keep your previous version for now

[lmiller1990]

I have not kept up with this thread - I was away for a few days. VTU v1 needs attention, so I will focus on that for a bit.

I will leave the review/decision on this PR to @nandi95 and @cexbrayat 🙏

[cexbrayat]

This is starting to look good now 👍

One last thing: as both trigger methods are the same, can't we move it back to the BaseWrapper?
Once that's solved, I think we should merge and cut a release to give it a try.

[nandi95]

This is starting to look good now 👍

One last thing: as both trigger methods are the same, can't we move it back to the BaseWrapper?
Once that's solved, I think we should merge and cut a release to give it a try.

I could move the one from dom wrapper, but not the vue wrapper one as the base wrapper doesn't have access to T extends ComponentPublicInstance generic type

[cexbrayat]

@nandi95 for the signatures, I agree. but I'm talking about the body of the methods. You can keep the overloaded signatures in both wrappers and simply do super.trigger(eventString, options); with all the code in the base trigger method no?

[nandi95]

Ah! didn't think of that 😅

Once this is merged, I'll create an issue for the modifiers

[cexbrayat]

👍 Did you try pikax suggestion for emits? https://github.com/vuejs/vue-test-utils-next/pull/551/files#r626442106

[nandi95]

👍 Did you try pikax suggestion for emits? https://github.com/vuejs/vue-test-utils-next/pull/551/files#r626442106

It was resolved, responded now

[lmiller1990]

Anything else needed here, or we can merge once CI is green?

Also no breaking change - right - just types (from what I can see, just checking).

[cexbrayat]

@lmiller1990 I think @pikax made a suggestion here https://github.com/vuejs/vue-test-utils-next/pull/551/files#r627168878 that @nandi95 may want to try.

If it helps, great. If it doesn't then @nandi95 will remove the attempt of having a typed version of trigger in VueWrapper, and I think all the code can go back to the Base Wrapper with the slight improvement to have overloads (one for DomEvent and a fallback to string).

TL;DR: we're waiting on @nandi95 update, and there is no breaking change, just a type update 😉

[lmiller1990]

Great, thanks for the update @cexbrayat!

[nandi95]

After discussion with @pikax I'm reverting the changes concerning the vue and dom wrapper and keep everthing in the base wrapper for now.

I think a discussion or issue can be raised if this comes up again whether should we typehing the event names from the components and its argument

My reasoning for this is:
I think it's hardly the case that in tests you manually emit a component event as its implementation of a behaviour not an organic user event (perhaps if you writing a component library with an odd design and the developer have to trigger an event?)

[cexbrayat]

Awesome, thanks @nandi95 for exploring this 👍

PR is in good shape, I'm going to merge so it can be part of the next RC.