-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
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 😅 |
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,
this
was 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 tonever
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:Changing to
void
resolves this error and I believe will work everywhere that never worked.I removed the comment since it no longer applies with this being
never
orvoid
. It appears that it was just forgotten when the switch fromE['currentTarget']
tonever
was made.