Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this PR, EventData and methods for adding/removing listeners will now be generic:
Please do give comment on the discussion points I've posted and (once we've reviewed it and prepared a build of it) a try on your projects, as I fully expect it'll result in some TypeScript grumbles.
PR Checklist
What is the current behavior?
Types for EventData and adding/removing listeners are non-generic, so we always have to write
eventData.object as Button
if we want anything more specific than an Observable.What is the new behavior?
Improved inference in most cases. The events in Application have always been very messy though, so I've left those as defaulting to Observable even though some could be narrowed to View, as I simply can't get my head round them.
Note that this PR would introduce breaking changes for library authors. As although we do provide default args for each generic, if they've extended
on
anywhere, they'll have to add the generic, as demonstrated in this playground:For discussion
What should the generics be?
The generics implemented in this PR are "better than nothing", but still not ideal. See what I ideally wanted to implement vs. what I was able to get working:
The former is more strictly correct, but I just couldn't get TypeScript to accept it 🤷
Should we narrow the default generic for EventData in the first place?
Although I think it is correct for the methods for adding/removing event listeners to have more narrow generic defaults than just
Observable
, e.g.:... I am unsure about whether the EventData interfaces should behave the same:
Mainly because we don't know whether others might repurpose the same event (SelectedIndexChangedEvent can be used for more than just SegmentedBar, and indeed there are duplicates of this interface across the codebase). Of course consumers can just specify the generic explicitly if they want to override the assumed default of SegmentedBar, so it's not the end of the world whichever choice we go with.