-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add promised oneEvent
listener
#41
Conversation
const spy = sinon.spy(); | ||
const controller = new AbortController(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was duplicate
delegate.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was moved without many changes. Just castAddEventListenerOptions
probably
options?: DelegateOptions | ||
): Promise<DelegateEvent<GlobalEventHandlersEventMap[TEventType], TElement>>; | ||
|
||
async function oneEvent< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the benefit of having this overload over the selector inference ? In delegate
, this was necessary for BC reasons with older versions in case of explicit generic calls (instead of relying on inference based on the selector), but this only weakens the type checks by allowing a mismatch between the element type expected by the callable and the selector in case of explicit typing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copy-pasted the types so if they're correct there they're correct here too, the output is identical (both "generate" a DelegateEvent). If you think the overload should be dropped, please do so. It will help with the implementation of #35
It'd be good to to add some type testing since it's expectType
included in vitest.
*/ | ||
async function oneEvent< | ||
Selector extends string, | ||
TElement extends Element = ParseSelector<Selector, HTMLElement>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have removed this generic type, using ParseSelector<Selector, HTMLElement>
directly in the return type. This would make explicit generic calls easier to write (only 2 template types instead of 3) and type checking more strict (by not accepting a callback that wants a HTMLTextareaElement
when the selector is not that specific)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you send a PR? I didn't really try to optimize the types here at this time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too bad I haven't seen this before the 6.0 release, as changing the template types is a BC break for Typescript users not relying on type inference.
However, if you are fine doing a new major version for that, I could indeed send a PR making type checks stricter (which would probably also remove the need for exporting overloads)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too bad I haven't seen this before the 6.0 release
Yeah I kinda rushed a few breaking changes in.
if you are fine doing a new major version for that
No problem, the earlier the better so no one actually updates to v6, if a new version is really breaking (e.g. with #48 )
once
#40await delegate(document, 'a', 'click')
#34