Changes EventHandler<...> to have a this of type void.#3867
Conversation
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 can you remember the rationale for the change? Edit: There is some context in #3137.
You can't call a function if it's |
I believe that |
|
Hey, this was a big miss on my behalf. The failing test is what I think lead us down the path of changing this. Going to fish our issues in a bit. When trying it out in the browser logging out EDIT: this was one of the original issues #3137 I wonder whether we can make it the element 😅 |
|
I get When you bind an arrow function however, the binding is ignored ( The underlying problem here is that JavaScript functions can be rebound. Just because The net takeaway here I think is that relying on |
|
Reviewing the failing test, it appears that the test is incorrect here: // Test `this` binding on event handlers
function onHandler(this: HTMLInputElement, event: any) {
return this.value;
}
const foo = <input onChange={onHandler} />;Specifically, if we want to say that there are no guarantees that On the other hand, if we want to guarantee that I'm happy to update the PR to "fix the test" (delete it), or switch back to |
|
Let's delete the test, I think we should be good to tell folks not to rely on |
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.
Done, and tests passing. |
Originally,
thiswas set toE['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 toneverin #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
neveris wrong because this code doesn't compile:Changing to
voidresolves this error and I believe will work everywhere that never worked.I removed the comment since it no longer applies with this being
neverorvoid. It appears that it was just forgotten when the switch fromE['currentTarget']toneverwas made.