Skip to content

Add correct this type for event handlers#2166

Merged
marvinhagemeister merged 1 commit into
masterfrom
this-jsx
Dec 3, 2019
Merged

Add correct this type for event handlers#2166
marvinhagemeister merged 1 commit into
masterfrom
this-jsx

Conversation

@marvinhagemeister

Copy link
Copy Markdown
Member

We didn't type the this binding for event handlers at all. According to MDN it always points to the DOM node the event handler was invoked upon.

Fixes #2164

@JoviDeCroock JoviDeCroock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice find!

@coveralls

coveralls commented Dec 3, 2019

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 100.0% when pulling f0306e8 on this-jsx into 51f0fb8 on master.

@marvinhagemeister marvinhagemeister merged commit b40d02c into master Dec 3, 2019
@marvinhagemeister marvinhagemeister deleted the this-jsx branch December 3, 2019 13:06
MicahZoltu pushed a commit to MicahZoltu/preact that referenced this pull request Jan 21, 2023
Originally, `this` was set to `E['currentTarget']` which, based on the MDN doc linked in preactjs#2166, certainly sounds like the correct thing to do.  A couple years later it was changed to `never` in preactjs#3147 with no commentary on the PR or commit indicating why.  My guess is that this was changed so arrow functions would work, which I believe are permanently bound and cannot be rebound by the caller.

I believe that `never` is wrong because this code doesn't compile:
```ts
type TargetedEvent<Target extends EventTarget = EventTarget, TypedEvent extends Event = Event> = Omit<TypedEvent, 'currentTarget'> & { readonly currentTarget: Target }
interface EventHandler<E extends TargetedEvent> {
	(this: never, event: E): void
}
declare const apple: EventHandler<TargetedEvent<HTMLElement, Event>>
declare const event: TargetedEvent<HTMLElement, Event>
apple(event) // error TS2684: The 'this' context of type 'void' is not assignable to method's 'this' of type 'never'.
```
Changing to `void` resolves this error and should work everywhere I believe.

I removed the comment since it no longer applies with this being `never` or `void`.  It appears that it was just forgotten when the switch from `E['currentTarget']` to `never` was made.
JoviDeCroock added a commit that referenced this pull request Feb 1, 2023
* Changes `EventHandler<...>` to have a `this` of type `void`.

Originally, `this` was set to `E['currentTarget']` which, based on the MDN doc linked in #2166, certainly sounds like the correct thing to do.  A couple years later it was changed to `never` in #3147 with no commentary on the PR or commit indicating why.  My guess is that this was changed so arrow functions would work, which I believe are permanently bound and cannot be rebound by the caller.

I believe that `never` is wrong because this code doesn't compile:
```ts
type TargetedEvent<Target extends EventTarget = EventTarget, TypedEvent extends Event = Event> = Omit<TypedEvent, 'currentTarget'> & { readonly currentTarget: Target }
interface EventHandler<E extends TargetedEvent> {
	(this: never, event: E): void
}
declare const apple: EventHandler<TargetedEvent<HTMLElement, Event>>
declare const event: TargetedEvent<HTMLElement, Event>
apple(event) // error TS2684: The 'this' context of type 'void' is not assignable to method's 'this' of type 'never'.
```
Changing to `void` resolves this error and should work everywhere I believe.

I removed the comment since it no longer applies with this being `never` or `void`.  It appears that it was just forgotten when the switch from `E['currentTarget']` to `never` was made.

* Removes incorrectly passing test.

The type signature's intent is to make it clear to users that they cannot rely on `this` being the element in callbacks, and they should use `event.currentTarget` instead.  This test was testing that the user could unwisely ignore that.

---------

Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>
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

Successfully merging this pull request may close these issues.

Untyped "this" parameter in the type declarations.

3 participants