Skip to content

Improve TS autocompletion for the type option in bind #44

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Feb 2, 2023
Merged

Improve TS autocompletion for the type option in bind #44

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2c72de8
Improve TS autocompletion for the type option in `bind`
Andarist Nov 19, 2021
a2d5d20
Refactor the types
Andarist Dec 30, 2022
818cc3d
Merge remote-tracking branch 'origin/master' into improve-autocompletion
Andarist Dec 30, 2022
eeb1f91
Bring back default type params to the `Binding` type
Andarist Dec 30, 2022
ac4d78f
Add notes about the completions fix being part of the upcoming TS 5.0
Andarist Dec 30, 2022
f331ec1
Improve the type signature for `bindAll` based on reverse mapped types
Andarist Dec 30, 2022
d67e4ef
revert irrelevant type -> interface change
Andarist Jan 19, 2023
c2ee043
reorder type declarations to produce a smaller diff
Andarist Jan 25, 2023
918cc5b
Added comments to `InferEventType`
Andarist Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
263 changes: 20 additions & 243 deletions src/bind-all.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Binding, UnbindFn } from './types';
import { Binding, InferEvent, InferEventType, Listener, UnbindFn } from './types';
import { bind } from './bind';

function toOptions(value?: boolean | AddEventListenerOptions): AddEventListenerOptions | undefined {
Expand Down Expand Up @@ -35,254 +35,31 @@ function getBinding(original: Binding, sharedOptions?: boolean | AddEventListene
return binding;
}

export function bindAll<Target extends EventTarget, Type extends string>(
target: Target,
bindings: [Binding<Target, Type>],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn;
export function bindAll<Target extends EventTarget, Type1 extends string, Type2 extends string>(
target: Target,
bindings: [Binding<Target, Type1>, Binding<Target, Type2>],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn;
export function bindAll<
Target extends EventTarget,
Type1 extends string,
Type2 extends string,
Type3 extends string,
>(
target: Target,
bindings: [Binding<Target, Type1>, Binding<Target, Type2>, Binding<Target, Type3>],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn;
export function bindAll<
Target extends EventTarget,
Type1 extends string,
Type2 extends string,
Type3 extends string,
Type4 extends string,
>(
target: Target,
bindings: [
Binding<Target, Type1>,
Binding<Target, Type2>,
Binding<Target, Type3>,
Binding<Target, Type4>,
],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn;
export function bindAll<
Target extends EventTarget,
Type1 extends string,
Type2 extends string,
Type3 extends string,
Type4 extends string,
Type5 extends string,
>(
target: Target,
bindings: [
Binding<Target, Type1>,
Binding<Target, Type2>,
Binding<Target, Type3>,
Binding<Target, Type4>,
Binding<Target, Type5>,
],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn;
export function bindAll<
Target extends EventTarget,
Type1 extends string,
Type2 extends string,
Type3 extends string,
Type4 extends string,
Type5 extends string,
Type6 extends string,
>(
target: Target,
bindings: [
Binding<Target, Type1>,
Binding<Target, Type2>,
Binding<Target, Type3>,
Binding<Target, Type4>,
Binding<Target, Type5>,
Binding<Target, Type6>,
],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn;
export function bindAll<
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the hope at least.

Target extends EventTarget,
Type1 extends string,
Type2 extends string,
Type3 extends string,
Type4 extends string,
Type5 extends string,
Type6 extends string,
Type7 extends string,
TTarget extends EventTarget,
TTypes extends ReadonlyArray<InferEventType<TTarget> & string>,
>(
target: Target,
target: TTarget,
bindings: [
Binding<Target, Type1>,
Binding<Target, Type2>,
Binding<Target, Type3>,
Binding<Target, Type4>,
Binding<Target, Type5>,
Binding<Target, Type6>,
Binding<Target, Type7>,
...{
[K in keyof TTypes]: {
type: TTypes[K] | (string & {});
listener: Listener<
TTarget,
InferEvent<
TTarget,
// `& string` "cast" is not needed since TS 4.7 (but the repo is using TS 4.6 atm)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can update to TS@latest to remove this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

TTypes[K] & string
>
>;
options?: boolean | AddEventListenerOptions;
};
}
],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn;
export function bindAll<
Target extends EventTarget,
Type1 extends string,
Type2 extends string,
Type3 extends string,
Type4 extends string,
Type5 extends string,
Type6 extends string,
Type7 extends string,
Type8 extends string,
>(
target: Target,
bindings: [
Binding<Target, Type1>,
Binding<Target, Type2>,
Binding<Target, Type3>,
Binding<Target, Type4>,
Binding<Target, Type5>,
Binding<Target, Type6>,
Binding<Target, Type7>,
Binding<Target, Type8>,
],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn;
export function bindAll<
Target extends EventTarget,
Type1 extends string,
Type2 extends string,
Type3 extends string,
Type4 extends string,
Type5 extends string,
Type6 extends string,
Type7 extends string,
Type8 extends string,
Type9 extends string,
>(
target: Target,
bindings: [
Binding<Target, Type1>,
Binding<Target, Type2>,
Binding<Target, Type3>,
Binding<Target, Type4>,
Binding<Target, Type5>,
Binding<Target, Type6>,
Binding<Target, Type7>,
Binding<Target, Type8>,
Binding<Target, Type9>,
],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn;
export function bindAll<
Target extends EventTarget,
Type1 extends string,
Type2 extends string,
Type3 extends string,
Type4 extends string,
Type5 extends string,
Type6 extends string,
Type7 extends string,
Type8 extends string,
Type9 extends string,
Type10 extends string,
>(
target: Target,
bindings: [
Binding<Target, Type1>,
Binding<Target, Type2>,
Binding<Target, Type3>,
Binding<Target, Type4>,
Binding<Target, Type5>,
Binding<Target, Type6>,
Binding<Target, Type7>,
Binding<Target, Type8>,
Binding<Target, Type9>,
Binding<Target, Type10>,
],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn;
export function bindAll<
Target extends EventTarget,
Type1 extends string,
Type2 extends string,
Type3 extends string,
Type4 extends string,
Type5 extends string,
Type6 extends string,
Type7 extends string,
Type8 extends string,
Type9 extends string,
Type10 extends string,
Type11 extends string,
>(
target: Target,
bindings: [
Binding<Target, Type1>,
Binding<Target, Type2>,
Binding<Target, Type3>,
Binding<Target, Type4>,
Binding<Target, Type5>,
Binding<Target, Type6>,
Binding<Target, Type7>,
Binding<Target, Type8>,
Binding<Target, Type9>,
Binding<Target, Type10>,
Binding<Target, Type11>,
],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn;
export function bindAll<
Target extends EventTarget,
Type1 extends string,
Type2 extends string,
Type3 extends string,
Type4 extends string,
Type5 extends string,
Type6 extends string,
Type7 extends string,
Type8 extends string,
Type9 extends string,
Type10 extends string,
Type11 extends string,
Type12 extends string,
>(
target: Target,
bindings: [
Binding<Target, Type1>,
Binding<Target, Type2>,
Binding<Target, Type3>,
Binding<Target, Type4>,
Binding<Target, Type5>,
Binding<Target, Type6>,
Binding<Target, Type7>,
Binding<Target, Type8>,
Binding<Target, Type9>,
Binding<Target, Type10>,
Binding<Target, Type11>,
Binding<Target, Type12>,
],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn;
export function bindAll(
target: EventTarget,
bindings: Binding[],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn;
export function bindAll(
target: EventTarget,
bindings: Binding[],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn {
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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator Author

@Andarist Andarist Jan 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return bind(target, binding);
});

Expand Down
33 changes: 27 additions & 6 deletions src/bind.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,31 @@
import { UnbindFn, Binding } from './types';
import { UnbindFn, InferEventType, InferEvent, Listener } from './types';

export function bind<Target extends EventTarget, Type extends string>(
target: Target,
{ type, listener, options }: Binding<Target, Type>,
): UnbindFn;
export function bind(target: EventTarget, { type, listener, options }: Binding) {
export function bind<TTarget extends EventTarget, TType extends InferEventType<TTarget> & string>(
target: TTarget,
// binding: Binding<
// TTarget,
// // `| (string & {})` should be moved to the Type's constraint
// // however, doing that today breaks autocompletion
// // this is being by https://github.com/microsoft/TypeScript/pull/51770 but we need wait for its release in TS 5.0
// TType | (string & {})
// >

// this "inline" variant works better when it comes to limiting `InferEvent` to using the `TType` from the "outer scope" (bind's and not Binding's)
// we can still export Binding and it could be used by people if they with to. To aid inference we can keep this inline within `bind`'s signature
// most likely we'll be able to refactor this when https://github.com/microsoft/TypeScript/pull/51770 gets released in TS 5.0
{
type,
listener,
options,
}: {
// `| (string & {})` should be moved to the Type's constraint
// however, doing that today breaks autocompletion
// this is being fixed by https://github.com/microsoft/TypeScript/pull/51770 but we need wait for its release in TS 5.0
type: TType | (string & {});
listener: Listener<TTarget, InferEvent<TTarget, TType>>;
options?: boolean | AddEventListenerOptions;
},
): UnbindFn {
target.addEventListener(type, listener, options);

return function unbind() {
Expand Down
48 changes: 24 additions & 24 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,37 @@
export type UnbindFn = () => void;

type ExtractEventTypeFromHandler<MaybeFn extends unknown> = MaybeFn extends (
this: any,
event: infer MaybeEvent,
) => any
? MaybeEvent extends Event
? MaybeEvent
: Event
type AnyFunction = (...args: any[]) => any;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments to the source code that explain it

}
? P
: never;

// Given an EventTarget and an EventName - return the event type (eg `MouseEvent`)
// Rather than switching on every time of EventTarget and looking up the appropriate `EventMap`
// We are being sneaky an pulling the type out of any `on${EventName}` property
// This is surprisingly robust
type GetEventType<
Target extends EventTarget,
EventName extends string,
> = `on${EventName}` extends keyof Target
? ExtractEventTypeFromHandler<Target[`on${EventName}`]>
: Event;
export type InferEvent<TTarget, TType extends string> =
// we check if the inferred Type is the same as its defined constraint
// if it's the same then we've failed to infer concrete value
// it means that a string outside of the autocompletable values has been used
// we'll be able to drop this check when https://github.com/microsoft/TypeScript/pull/51770 gets released in TS 5.0
InferEventType<TTarget> extends TType
? Event
: `on${TType}` extends keyof TTarget
? Parameters<Extract<TTarget[`on${TType}`], AnyFunction>>[0]
: Event;

// For listener objects, the handleEvent function has the object as the `this` binding
type ListenerObject<TEvent extends Event> = {
handleEvent(this: ListenerObject<TEvent>, e: TEvent): void;
handleEvent(this: ListenerObject<TEvent>, event: TEvent): void;
};

// event listeners can be an object or a function
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> =
Copy link
Owner

@alexreardon alexreardon Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'>>

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

| ListenerObject<TEvent>
| { (this: TTarget, ev: TEvent): void };

export type Binding<Target extends EventTarget = EventTarget, EventName extends string = string> = {
type: EventName;
listener: Listener<Target, EventName>;
export type Binding<TTarget extends EventTarget = EventTarget, TType extends string = string> = {
type: TType;
listener: Listener<TTarget, InferEvent<TTarget, TType>>;
options?: boolean | AddEventListenerOptions;
};