Generic events

[shirakaba]

With this PR, EventData and methods for adding/removing listeners will now be generic:

// This event is inherited from View, so eventData.object is
// inferred to be a View.
new Button().on("loaded", (eventData) => {
  eventData.object; // TypeScript infers this to be a View
});

// You can, however, explicitly pass the generic to narrow
// that down to Button:
new Button().on<Button>("loaded", (eventData) => {
  eventData.object; // TypeScript infers this to be a Button
});

// Button-specific events don't require passing the generic:
new Button().on("tap", (eventData) => {
  eventData.object; // TypeScript infers this to be a Button
});

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

• The PR title follows our guidelines - TODO: I'm keeping them as individual commits for review, but we can rename upon squash.
• There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
• You have signed the CLA.
• All existing tests are passing: https://github.com/NativeScript/NativeScript/blob/master/tools/notes/DevelopmentWorkflow.md#running-unit-tests-application.
• Tests for the changes are included - It's largely a typings change, so no new tests to add.

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:

// What I wanted:
export abstract class View extends ViewCommon {
  on<T extends View = this>(event: 'loaded', callback: (args: EventData<T>) => void, thisArg?: any);
}

// The only thing I could get to work:
export abstract class View extends ViewCommon {
  on<T extends Observable = View>(event: 'loaded', callback: (args: EventData<T>) => void, thisArg?: any);
}

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.:

export class SegmentedBar extends View implements AddChildFromBuilder, AddArrayFromBuilder {
  on<T extends Observable = SegmentedBar>(event: 'selectedIndexChanged', callback: (args: SelectedIndexChangedEventData<T>) => void, thisArg?: any): void;
}

... I am unsure about whether the EventData interfaces should behave the same:

// Would we prefer a narrow default?
export interface SelectedIndexChangedEventData<T extends Observable = SegmentedBar> extends EventData<T> {
  oldIndex: number;
  newIndex: number;
}

// ... or just stick to Observable?
export interface SelectedIndexChangedEventData<T extends Observable = Observable> extends EventData<T> {
  oldIndex: number;
  newIndex: number;
}

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.

[nx-cloud]

☁️ Nx Cloud Report

We didn't find any information for the current pull request with the commit 9a07b1b.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.

---

Sent with 💌 from NxCloud.

[ammarahm-ed]

This is ok but I think it doesn't really solve the problem. All Views "Should" infer to correct types in events automatically without passing some Generic or anything.

new Button().on<Button>("loaded", (eventData) => {
  eventData.object; // TypeScript infers this to be a Button
});

is almost as much work as doing

new Button().on("loaded", (eventData) => {
  eventData.object as Button;
});

The ideal way would be to not have to pass any generic at all when you are subscribing to an event on Button. For example:

new Button().on("loaded", (eventData) => {
  eventData.object // is Button automatically
});

Also my question, why does this work or to be more specific, how does this work:

// Button-specific events don't require passing the generic:
new Button().on("tap", (eventData) => {
  eventData.object; // TypeScript infers this to be a Button
});

[farfromrefug]

@ammarahm-ed be careful some views fire event for which the object is not them self! I know some plugins already do that. And I think some N components also do that. Will try to find them