Skip to content

Commit

Permalink
Changes EventHandler<...> to have a this of type void. (#3867)
Browse files Browse the repository at this point in the history
* 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 <[email protected]>
  • Loading branch information
MicahZoltu and JoviDeCroock authored Feb 1, 2023
1 parent 15913ad commit e703a62
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 11 deletions.
6 changes: 1 addition & 5 deletions src/jsx.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -418,11 +418,7 @@ export namespace JSXInternal {
>;

export interface EventHandler<E extends TargetedEvent> {
/**
* The `this` keyword always points to the DOM element the event handler
* was invoked on. See: https://developer.mozilla.org/en-US/docs/Web/Guide/Events/Event_handlers#Event_handlers_parameters_this_binding_and_the_return_value
*/
(this: never, event: E): void;
(this: void, event: E): void;
}

export type AnimationEventHandler<Target extends EventTarget> = EventHandler<
Expand Down
6 changes: 0 additions & 6 deletions test/ts/Component-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,6 @@ import 'mocha';
import { expect } from 'chai';
import { createElement, Component, RenderableProps, Fragment } from '../../';

// Test `this` binding on event handlers
function onHandler(this: HTMLInputElement, event: any) {
return this.value;
}
const foo = <input onChange={onHandler} />;

export class ContextComponent extends Component<{ foo: string }> {
getChildContext() {
return { something: 2 };
Expand Down

0 comments on commit e703a62

Please sign in to comment.