Improve TS autocompletion for the type option in `bind` #44

Andarist · 2021-11-19T09:30:16Z

Before

After

Can be tested on this TS playground

The same thing could be done for bindAll but the code for this function is way more complex and I didn't have time to dive into this right now. I would also recommend dropping overloads from there and handle everything in a single signature.

alexreardon · 2021-11-19T10:30:08Z

This looks great! I'll take a look on Monday ❤️

Andarist · 2021-11-19T18:04:34Z

@devanshj is it possible to get bindAll typed using a single signature in a way that the listener for each item would be typed based on the type property of that item AND the type property would work with autocomplete? I've tried to do this but I've failed, this probably needs to use Self technique and Tuple constraint right? but I kinda cant figure out how to process each item separately

My latest, poor, stab at this can be found here. I can't make the Bindings to be inferred as tuple, so the key in the mapped type gets served to me as number, instead of the concrete index. The autocomplete is also completely broken here.

devanshj · 2021-11-19T22:43:54Z

Yep it's possible but you need none of that, not even the tuple inference trick.

The thing is these existing types are unnecessarily complex, in particular the bindAll type (and hence yours too are so). You see they are making binding.type generic, but it's not! We already have a static union of possible binding.type, namely all event names of the passed target. So only one thing needs to be generic ie the target, that's it.

I'm about to rewrite the types and send a PR, here's a preview.

The key here is to realize the events and their names¹ are not existentially generic, that is to say (for a given target) we have a fixed set of values, we can represent it like so...

type HtmlEvent<N extends "click" | "copy" | ...> = 
  { click: MouseEvent
  , copy: ClipboardEvent
  , ...
  }[N]

It would have been existentially generic if were something like this...

type MyCustomEvent<N extends string> =
  N extends `on${infer X}Changed`
    ? { type: N } & { [_ in X]: number }
    : never

...here MyCustomEvent is a function whose domain ie string (and hence the image too) is infinite, it can be anything, so you can't create a map of it.

So your bind type which loosely is...

declare const bind:
  <T extends EventTarget, N extends EventName<T>>
    (target: T, binding: { type: N, listener: (e: Event<T, N>) => void }) =>
      () => void

...can be written as...

declare const bind:
  <T extends EventTarget>
    ( target: T
    , binding:
        EventName<T> extends infer N ? N extends unknown ?
          { type: N, listener: (e: Event<T, N>) => void }
        : never : never
    ) =>
      () => void

Now for bind even I would write the former (for simplicity, convenience, maybe performance, etc) but in case of bindAll we would have to write latter because the former approach simply won't work without fixed number of overloads or some complex types which you were attempting.

And now with the latter approach the bindAll's type becomes really straightforward:

declare const bindAll:
  <T extends EventTarget>
    ( target: T
    , bindings:
        ( EventName<T> extends infer N ? N extends unknown ?
            { type: N, listener: (e: Event<T, N>) => void }
          : never : never
        )[]
    ) =>
      () => void

So if I were to give a takeaway (because reading so much must be rewarded :P), if you have a generic U which is a function of T, and making T concrete also makes U concrete, then U is not a generic in principle. So it should be made a generic as long as it makes the types simple (like in bind) but not when it makes types complex (like in bindAll).

Thanks for reading my blog, written in a github comment :P

Calling event types (click, copy, etc) "name" because "event type" can mean the names or their typescript type (MouseEvent, ClipboardEvent, etc), the code does the same. ↩

devanshj · 2021-11-20T04:05:53Z

Opened #45.

It turns out that this approach has one downside when supporting untyped events ("supporting" meaning fallbacking to Event for events that were not found on the target as on${eventName}). I'll point out the downside in the PR, but that being said this PR also does not support untyped events (which is a breaking change), so the question is do we want to support it?

If yes, then we'll have to figure out how to make the types and autocomplete work, especially autocomplete because "click" | string becomes string and you'd have to employ tricks like "click" | (string & {}) which only work in some cases. In this case it's probably not possible to make the autocomplete work so.

If no, then my approach would work and the types become simple.

And remember no means that there will be no typescript support, the runtime would obviously work, so they'd have to make assertions. Or we could provide them with an opaque type { [$$isUnknownEventType]: true } (which would not interfere with autcomplete) so they could do "foo" as UnknownEventType and then we'd fallback to Event. Edit: Actually we could even support something like "foo" as any or "foo" as never which is much easier.

alexreardon · 2021-12-03T04:45:39Z

This is looking great

alexreardon · 2022-01-27T22:52:18Z

I think this PR is a good starting point towards improving the types. Would you be okay if i merged it as is @Andarist?

devanshj · 2022-01-27T22:58:29Z

(I don't have any opinions, just giving a heads up that this PR as it is will be a breaking change)

alexreardon · 2022-01-27T23:10:51Z

src/bind.ts


-export function bind<Target extends EventTarget, Type extends string>(
+export function bind<Target extends EventTarget, Type extends PossibleEventType<keyof Target>>(


this is magic 👏

A true magic was probably here 😅 : https://github.com/alexreardon/bind-event-listener/pull/45/files

alexreardon · 2022-01-27T23:15:24Z

src/types.ts

+export type PossibleEventType<K> = K extends any
+  ? K extends `on${infer Type}`
+    ? Type
+    : never


what would happen for an event that did not have a on* property (eg DOMContentLoaded). Would that be allowed, or would it be an error now?

Should it be string rather than never?

Ideally we get auto complete + inference goodness. But when the event has no on* it just gets an Event

Should it be string rather than never?

This might turn off the autocomplete etc as we basically distribute a union of literal strings here and narrow it down to a union of literal strings (the operation here acts as a .filter()). We can't remap this union to smth like 'foo' | string because that's basically string (types reduce themselves to a common denominator).

But when the event has no on* it just gets an Event

Could you give me a practical example of when this would be useful?

what would happen for an event that did not have a on* property (eg DOMContentLoaded). Would that be allowed, or would it be an error now?

Most likely an error now - but since this special case is known upfront I could try to introduce support for it.

There is also a chance that maybe this could be actually implemented with an additional overload. This is how window.addEventListener is implemented:

addEventListener<K extends keyof WindowEventMap>(type: K, listener: (this: Window, ev: WindowEventMap[K]) => any, options?: boolean | AddEventListenerOptions): void; addEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions): void;

And it comes with autocomplete and allows u to use a random string.

As mentioned elsewhere - I'm not a huge fan of overloads as they don't play that well with some TS features (for instance, when using helpers like ReturnType or a conditional type with infer then only the very last overload is taken into a consideration and all the previous ones are ignored). This could work OK in this context here though.

Fwiw I liked #10 that handled all event targets. And alternatively you can also have the users pass additional event maps via module augmentation.

(I guess eventually I'll probably make an events package in sthir that would only have types required to deal with event emitters, it's a common problem eg even RxJS's fromEvent could leverage it)

Andarist · 2022-12-30T14:08:49Z

@alexreardon I've pushed out improvements for both bind and bindAll. Everything should be autocompletable and you no longer need the huge list of overloads to cover bindAll. It is a breaking change for bindAll though

alexreardon · 2023-01-16T04:12:26Z

Epic!

alexreardon · 2023-01-16T04:15:12Z

It is a breaking change for bindAll though

What are the breaking change(s)?

alexreardon

This is looking great

alexreardon · 2023-01-16T04:13:14Z

src/bind-all.ts

+          TTarget,
+          InferEvent<
+            TTarget,
+            // `& string` "cast" is not needed since TS 4.7 (but the repo is using TS 4.6 atm)


We can update to TS@latest to remove this

the question is - what should be the minimum TS requirement for the end users of the package?

If we are doing a breaking change anyway for the type change, I am thinking we can move to a relatively high TS target. What do you think?

4.7 has been released smth like 8 months ago. DefinitelyTyped officially supports the last 2 years of TS releases - that doesn't necessarily mean that you have to have the same policy though. It's not exactly like you have millions of downloads of this package - so I think that it's reasonable to prioritize your own use cases, ease of maintenance, etc.

That being said - this cast here is completely harmless. I just put this comment here to give a little bit of context behind this cast and also to leave the information on when it could be removed in the future (when you drop support for TS 4.6).

The more important change for this lib is coming in TS 5.0 - but the difference between the version of the code in this PR and the potential version of the code that would be compatible with TS 5.0 is also really subtle and you don't even have to follow up on that one immediately either.

However, it still might be worthwhile to act on it when TS 5.0 gets released (March 14th). That would remove the autocomplete capabilities for <5.0 users but the types would become slightly better (conceptually and all). So this is kinda up to you - I think that it's a fair game that autocompletion would behave differently between those TS versions. The code/types themselves would still work - the only difference would be the DX in the IDE.

To confirm my understanding:

The current version would work on TS 4.6+ (and 5.0)

After 5.0 is released we could do some internal cleanup of the code and still have good auto complete

Questions:

After 5.0 could we keep the currently proposed change and maintain good auto-complete for everybody? (Could eventually move to 5.0 later)

cc @Andarist

The current version would work on TS 4.6+ (and 5.0)

Yes.

After 5.0 is released we could do some internal cleanup of the code and still have good auto complete

Yes, but if we make that cleanup then TS 4.x would lose autocomplete

After 5.0 could we keep the currently proposed change and maintain good auto-complete for everybody? (Could eventually move to 5.0 later)

Yes, we don't have to do this cleanup immediately. We are free to leave it as is for a foreseeable future. We could also introduce said the cleanup, copy the current types into a separate file, and use typesVersions to point TS 4.x users to that other file. This arguably complicated the setup and stuff - so it's up to you if you would be comfortable with this or not

alexreardon · 2023-01-16T04:13:43Z

src/bind-all.ts

-  const unbinds: UnbindFn[] = bindings.map((original: Binding) => {
-    const binding: Binding = getBinding(original, sharedOptions);
+  const unbinds: UnbindFn[] = bindings.map((original) => {
+    const binding: Binding = getBinding(original as never, sharedOptions);


@Andarist can you please add a comment to the code explaining why as never is needed? I suspect that would be helpful information long term

It's just a "fancy" as any - I can just change it to as any to avoid confusing people.

as never works because never is assignable to just everything (but nothing, except never, is assignable to it). Since the cast happens in the argument position - it truly doesn't matter which one we use here.

You can go for whichever you think is best (and most maintainable)

There is no real difference between both here. When types flow through layers you might end up with the casted type in the return type or something, so then as never might be a little bit safer than as any - because you won't accidentally start accessing properties on any type (as that would be never and you wouldn't be able to access any properties on it).

I decided to keep as never - as a slightly safer one (although not really at this particular call site at this specific point in time). If you'd prefer as any - I can change this any time

Andarist · 2023-01-16T08:01:53Z

What are the breaking change(s)?

Just that you no longer can pass as many generics to the bindAll as you could in the past. That's basically the required change on the call sites:

- bindAll<HTMLInputElement, 'focus', 'blur'>(/* ... */)
+ bindAll<HTMLInputElement, ['focus', 'blur']>(/* ... */)

I don't think this is a big deal though since the inference improvements are quite nice here and both the old and the proposed types were targeting mostly inference-based scenarios. But if somebody was supplying those type arguments manually then it's a breaking change for them.

alexreardon · 2023-01-18T23:31:23Z

src/types.ts

-export type Binding<Target extends EventTarget = EventTarget, EventName extends string = string> = {
-  type: EventName;
-  listener: Listener<Target, EventName>;
+export interface Binding<TTarget extends EventTarget = EventTarget, TType extends string = string> {


why interfaces an not types? (Personally, I prefer the ergonomics of type)

It's rather an accidental change here - I was juggling the code a lot through my couple of attempts at this PR 😅 I reverted the change.

Overall there are not a lot of differences, but sometimes interfaces still "display" better in error tooltips, quick info etc. I don't know what the exact rules are but interfaces are always referenced by name there whereas type aliases are sometimes referenced by name and sometimes they are inlined.

Extending interfaces comes with some extra checks in TS, whereas intersecting aliases is always allowed (but might lead to accidental mistakes): TS playground

I also don't know the exact rules behind this but when we check is IA<T1> is assignable to IA<T2> we only need to check if T1 is assignable to T2 (or the other way around if the given type is a contravariant one). The same might happen with type aliases (I'm not sure?) but they definitely sometimes use a full structural comparison - in other words, if the type is huge but it's type arguments are small then with interfaces such a comparison is guaranteed to be fast whereas with type aliases it sometimes might not be as type aliases might be sometimes compared property by property.

Obligatory note: it usually doesn't matter at all - don't stress it out 😅

alexreardon · 2023-01-25T08:54:51Z

Sorry, it's been a busy few days! I am keen to push this forward. Are you happy with where this PR is at @Andarist?

Post-merge things to do:

Make release notes
Make breaking change

Andarist · 2023-01-25T09:46:16Z

@alexreardon ye, i think it's good to go

alexreardon · 2023-01-27T03:29:10Z

src/types.ts

+
+export type InferEventType<TTarget> = TTarget extends {
+  addEventListener(type: infer P, ...args: any): void;
+  addEventListener(type: infer P2, ...args: any): void;


What is addEventListener(type: infer P2, ...args: any): void; (and P2) for?

Added comments to the source code that explain it

alexreardon · 2023-01-28T07:06:59Z

I am aiming to merge and release this early next week 🎉

alexreardon · 2023-01-28T07:09:43Z

Thanks so much for the change and the explanations @Andarist

alexreardon · 2023-01-30T05:43:54Z

src/types.ts

-export type Listener<Target extends EventTarget, EventName extends string> =
-  | ListenerObject<GetEventType<Target, EventName>>
-  | { (this: Target, e: GetEventType<Target, EventName>): void };
+export type Listener<TTarget extends EventTarget, TEvent extends Event> =


My finger is hovering over the merge button and I am thinking through release notes.

What is the rationale for changing from EventName extends string to TEvent extends Event for Listener? Before Listener did more heavy lifting behind the scenes.

- Listener<{} as HTMLElement, 'click'> + Listener<{} as HTMLElement, InferEvent<{} as HTMLElement, 'click'>>

I'm not 100% sure right now. I was working on this for weeks and all of the minor details are already lost for me. This could start as an experiment that stayed here or it could be needed.

Notice that I am actually passing arguments to it that look slightly different from each other, so there is a chance that unifying those "call sites" was not possible. So maybe I just unwinded a part of the "abstraction" to allow for more flexible type arguments. I'm not sure though.

The change makes sense from the types PoV/design - this doesn't mean that the previous version was not OK though. It all boils down to what kind of API you want to give to your consumers. The returned type is exactly the same - the only difference is in the arguments (and potentially in inference capabilities etc).

The previous version was more on the verge of a type helper, with more type gymnastics in it (and such gymnastics can sometimes have a negative impact on inference because TS might have a harder time understanding this, but again - I'm not sure if that was the case here).

I'm afraid that I probably don't have time to tinker with this more to investigate if this change could be rolled back. Note that what is internally used doesn't have to be exposed directly. So we could rename the existing Listener to something else (InternalListener?) and build the "old" Listener from the available pieces.

alexreardon · 2023-01-30T05:46:14Z

src/bind-all.ts

-    Binding<Target, Type6>,
-  ],
-  sharedOptions?: boolean | AddEventListenerOptions,
-): UnbindFn;
 export function bindAll<


To confirm my understanding, the signature of bindAll has changed:

- bindAll<{} as HTMLElement, 'click', 'mousedown'> + bindAll<{} as HTMLElement, ['click', 'mousedown']>

But if you are using inference then nothing has changed. Is that right?

Yes, that's the hope at least.

Improve TS autocompletion for the type option in bind

2c72de8

devanshj mentioned this pull request Nov 20, 2021

Rewrite with better types #45

Closed

alexreardon approved these changes Jan 27, 2022

View reviewed changes

alexreardon reviewed Jan 27, 2022

View reviewed changes

Andarist added 2 commits December 30, 2022 14:33

Refactor the types

a2d5d20

Merge remote-tracking branch 'origin/master' into improve-autocompletion

818cc3d

Andarist force-pushed the improve-autocompletion branch from ad7ea9a to 818cc3d Compare December 30, 2022 13:34

Andarist added 2 commits December 30, 2022 14:37

Bring back default type params to the Binding type

eeb1f91

Add notes about the completions fix being part of the upcoming TS 5.0

ac4d78f

Andarist requested a review from alexreardon December 30, 2022 13:39

Improve the type signature for bindAll based on reverse mapped types

f331ec1

alexreardon requested changes Jan 16, 2023

View reviewed changes

Andarist force-pushed the improve-autocompletion branch from 650349e to f331ec1 Compare January 17, 2023 09:27

alexreardon reviewed Jan 18, 2023

View reviewed changes

revert irrelevant type -> interface change

d67e4ef

reorder type declarations to produce a smaller diff

c2ee043

Andarist force-pushed the improve-autocompletion branch from 3978b7c to c2ee043 Compare January 25, 2023 09:43

alexreardon reviewed Jan 27, 2023

View reviewed changes

alexreardon approved these changes Jan 27, 2023

View reviewed changes

Added comments to InferEventType

918cc5b

alexreardon reviewed Jan 30, 2023

View reviewed changes

alexreardon merged commit 660e855 into alexreardon:master Feb 2, 2023

alexreardon mentioned this pull request Feb 2, 2023

Running type tests on CI #93

Open

Andarist mentioned this pull request Mar 29, 2023

Prerelease cleanup #98

Merged


		export function bind<Target extends EventTarget, Type extends string>(
		export function bind<Target extends EventTarget, Type extends PossibleEventType<keyof Target>>(

Improve TS autocompletion for the type option in bind #44

Improve TS autocompletion for the type option in bind #44

Uh oh!

Conversation

Andarist commented Nov 19, 2021

Uh oh!

alexreardon commented Nov 19, 2021

Uh oh!

Andarist commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devanshj commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

devanshj commented Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexreardon commented Dec 3, 2021

Uh oh!

alexreardon commented Jan 27, 2022

Uh oh!

devanshj commented Jan 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devanshj Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andarist commented Dec 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexreardon commented Jan 16, 2023

Uh oh!

alexreardon commented Jan 16, 2023

Uh oh!

alexreardon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andarist Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andarist commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Improve TS autocompletion for the type option in `bind` #44

Improve TS autocompletion for the type option in `bind` #44

Andarist commented Nov 19, 2021 •

edited

Loading

devanshj commented Nov 19, 2021 •

edited

Loading

devanshj commented Nov 20, 2021 •

edited

Loading

devanshj Jan 28, 2022 •

edited

Loading

Andarist commented Dec 30, 2022 •

edited

Loading

Andarist Jan 16, 2023 •

edited

Loading

Andarist commented Jan 16, 2023 •

edited

Loading

alexreardon commented Jan 25, 2023 •

edited

Loading

alexreardon Jan 30, 2023 •

edited

Loading