-
Notifications
You must be signed in to change notification settings - Fork 30.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
[jquery] Improve declaration of Events API. #30144
[jquery] Improve declaration of Events API. #30144
Conversation
@leonard-thieu Thank you for submitting this PR! 🔔 @diegovilar @thorn0 @calebstdenis @scipper @borisyankov @nvivo @kenjiru @denisname @dbeckwith @choffmeister @Steve-Fenton @Diullei @tasoili @jasons-novaleaf @seanski @Guuz @ksummerlin @basarat @nwolverson @derekcicerone @AndrewGaspar @seikichi @benjaminjackman @s093294 @JoshStrobl @johnnyreilly @DickvdBrink @King2500 @terrymun @AndersonFriaca @gandjustas @andrei-markeev @baywet @teroarvola @dennispg - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
[type: string]: TypeEventHandler<TDelegateTarget, TData, TCurrentTarget, TTarget, TContext, string> | | ||
false | | ||
undefined | | ||
object; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding object
to the union weakens the type a bit, although in practice, it's unlikely that users will attempt to a assign a non-function object to a property. I don't understand what's causing the assignability issue nor do I understand why including object
in the union fixes it.
Thanks for this change and pinging me. Update 2018-11-08: |
After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience! |
@leonard-thieu One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you! |
…interface. These are undocumented and don't appear to be intended to be public.
…>` from handler types. Handler types previously included `JQuery.EventHandlerBase<TContext = any, JQuery.Event>`. This was a fix to what turned out to be a problem with `jQuery.proxy`. That issue was properly fixed by DefinitelyTyped#29930.
* Introduced the concept of a triggered event. A triggered event is a jQuery event that has been triggered (either manually by `.trigger()` or `.triggerHandler()`, or automatically by the user agent). Many properties on an event are not set until it has been triggered. Triggered events are now represented by `JQuery.TriggeredEvent`. `JQuery.Event` represents the object returned from the `jQuery.Event` constructor and serves as the base type for all jQuery events. * Added a type parameter for `currentTarget` (`TCurrentTarget`). Previously, `currentTarget` was set to the type of `delegateTarget`. This was only correct for events received from directly bound event handlers. This allows delegate bound event handlers to specify the correct type of `currentTarget`. * Added a type parameter for `target` (`TTarget`). Previously, `target` was set to the type of `delegateTarget`. This was not always correct. For delegate bound event handlers, `target` can be `delegateTarget` or any of its descendents. The exact type of `target` cannot be known until run time, so consumers should use a type assertion. * Changed methods that bind event handlers to more accurately reflect the event object received through the callback. Both direct and delegate binding are now properly handled. This also includes a framework for providing more specific events for known event types.
…n jQuery Events API.
6263423
to
bf2beee
Compare
@@ -74,6 +74,7 @@ | |||
"typedef-whitespace": false, | |||
"unified-signatures": false, | |||
"void-return": false, | |||
"whitespace": false | |||
"whitespace": false, | |||
"no-angle-bracket-type-assertion": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, please don't turn off linter rules. Especially on so many packages at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lint failures were caused by an unrelated change (microsoft/dtslint#141).
I really wasn't interested in fixing all of those failures by hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. The tests fail when you revert these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they're still failing. 565 violations of the no-angle-bracket-type-assertion
rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I'm really sorry about that, I was unaware. That PR must have been merged without the necessary followup.
Thanks for fixing all the fallout. Could you revert your revert then? Once the tests pass I can merge this PR.
TData = undefined | ||
> = EventHandlerBase<TCurrentTarget, TriggeredEvent<TCurrentTarget, TData>>; | ||
|
||
type TypeEventHandler< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find any instantiations of TypeEventHandler
where TCurrentTarget
, TTarget
and TContext
aren't all exactly the same type. It would be simpler to just have one type argument (TTarget
?) instead of 3 separate ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment was for an earlier commit -- I see you removed TContext
, but my comment still applies to TCurrentTarget
and TTarget
. They could be simplified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a possible mistake elsewhere, an event's currentTarget is definitely not always the same as its target.
The way React dealt with it is that Change events and others that don't bubble have target = currentTarget, but other events have target being EventTarget.
TContext is supposed to be whatever was the 2nd argument of .on in 3-argument form so sounds like another omission.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For directly bound event handlers, TElement
is passed for TCurrentTarget
and TTarget
because they are known to be the same object. For delegated event handlers, any
is passed for TCurrentTarget
and TTarget
because they can be any element in the event propagation path and potentially refer to different objects.
I went with 2 type parameters because they're not semantically the same. I was also concerned there might be scenarios where say a user asserts that currentTarget
is of a certain type which might allow TypeScript to infer that target
is the same type since they share a type parameter. I don't know if TypeScript actually does this but it didn't seem unreasonable. There's also concern with what to name it. jQuery.Event
has 4 properties that have target
in the name, one of which is named target
. I think if a user came across a type parameter named TTarget
, they'd either expect it to cover just the target
property or all 4 *Target
properties, not just a particular 2 of the 4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TContext is supposed to be whatever was the 2nd argument of .on in 3-argument form so sounds like another omission.
TContext
is passed TCurrentTarget
since they should be the same unless the function is bound to a different context.
@leonard-thieu I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues. |
@leonard-thieu The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
2058e08
to
e067afd
Compare
Please fill in this template.
npm test
.)npm run lint package-name
(ortsc
if notslint.json
is present).Select one of these and delete the others:
If changing an existing definition:
Provide a URL to documentation or source code which provides context for the suggested changes:Increase the version number in the header if appropriate.If you are making substantial changes, consider adding atslint.json
containing{ "extends": "dtslint/dt.json" }
.Summary of changes
Improve declaration of Events API. (f13af8b / 00c5463 / 3a54bad / db83bc7)
The
*Target
properties (currentTarget
,delegateTarget
,relatedTarget
, andtarget
) were all set to the same type parameter (TTarget
). API methods passed the type of selected elements (TElement
) toTTarget
. This is only guaranteed to be correct for directly bound event handlers (excludingrelatedTarget
, which varies based on event type instead of binding type). This PR fixes this issue by replacingTTarget
withTDelegateTarget
fordelegateTarget
,TCurrentTarget
forcurrentTarget
, andTTarget
fortarget
. Methods that bind directly bound event handlers pass inTElement
for all target-related type parameters as they are guaranteed to be the same object. Methods that bind delegate bound event handlers pass inTElement
forTDelegateTarget
andany
for the rest of the target-related parameters.unknown
would be better here but requires TypeScript 3.0. As the types ofcurrentTarget
,relatedTarget
, andtarget
cannot be known until run time, consumers will still need type assertions when accessing these properties but they are no longer presented with a type that could be outright wrong.This PR also introduces the concept of a triggered event. A triggered event is a jQuery event that has been triggered (either manually by
.trigger()
/.triggerHandler()
or automatically by the user agent). Many properties on a jQuery event are not set until it has been triggered. In particular, the*Target
properties are not set on a freshly constructedjQuery.Event
object.JQuery.Event
now represents the object returned from thejQuery.Event
constructor and serves as the base type for all jQuery events. This means that the*Target
properties (and their type parameters) were removed fromJQuery.Event
and added to a new triggered event type (JQuery.TriggeredEvent
).This PR also introduces event type-specific types for known events. When binding an event handler for a known event type, the event handler will receive an event type-specific type for that event type. For example, when binding to a "click" event, the event handler will receive a
ClickEvent
event that has all properties copied fromMouseEvent
(button
,ctrlKey
,shiftKey
, etc) declared as non-nullable andoriginalEvent
declared asMouseEvent
. This should reduce the need for type assertions for consumers. Types have been added for event types that have an associated shorthand method and for touch events. Types for the rest of the DOM events may be added at a later point.Migration notes
As a general note, there are many cases where the correct type can be inferred but consumers have annotated their code with an incorrect type. In these cases, it's best to just drop the annotation and let inference do its work.
Most breaks will be due to splitting
JQuery.Event
intoJQuery.TriggeredEvent
. As code that referencesJQuery.Event
will typically be event handlers (and therefore dealing with a triggered event), the fix is to simply change the declaration fromJQuery.Event
toJQuery.TriggeredEvent
. In cases where code actually wants to reference a base event type, the fix is to remove the type arguments from the declaration (the declaration will be simplyJQuery.Event
).Fixes #17377
Fixes #26674 (review)
Fixes #30705
Add deprecation notice for
Event.which
. (16770fa)Deprecated since 3.3. See jquery/api.jquery.com#821.
Remove
jQuery.event.props
andjQuery.event.fixHooks
. (50b56a1 / e64bea6)Removed since 3.0. See https://jquery.com/upgrade-guide/3.0/#breaking-change-jquery-event-props-and-jquery-event-fixhooks-removed.
Remove
jQuery.Event(EventLike)
signatures andEventLike
interface. (32e0482)These are undocumented and don't appear to be intended to be public.
Migration notes
If consumers still require the
EventLike
interface, they may add this declaration to their project.Remove
JQuery.EventHandlerBase<TContext = any, JQuery.Event>
from handler types. (c1ec1c2)Handler types previously included
JQuery.EventHandlerBase<TContext = any, JQuery.Event>
. This was a fix to what turned out to be a problem withjQuery.proxy
. That issue was properly fixed by #29930.Fixes 5 violations of
no-unnecessary-generics
.Fixes 5 violations of
unified-signatures
.Pinging @Kovensky @HolgerJeromin as they have expressed interest in this before.
@zhamid @nvivo @sventschui @razorness @confususs
The change to
@types/backbone
(68c38a8) was to fix a conflict with theEventsHash
declaration in@types/backbone.marionette
. You may wish to review that the change is appropriate.The addition of touch events was to fix test failures with
@types/viewporter
. Not going to bother mentioning authors of@types/viewporter
here as this was simply a matter of extending support.