☁️ Nx Cloud Report

CI is running/has finished running commands for commit 23a3498. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Sent with 💌 from NxCloud.

I've built locally and didn't expect to see test failures, so I'll dig into those.

EDIT: the test failures seem like they may just be false-negatives regarding sub-pixel rendering in layout tests, so I expect they'll pass on CI even if they don't pass here. Asking advice in the TSC chat.

Thanks for the review, @farfromrefug! As for the wishlist:

• template type for EventData T extending Observable to define "object"

I like it too in theory (have always wanted to just do data.object and get a ready-typed object), though not sure how well it will go - would greatly prefer to tackle that as a followup PR rather than rethink this one.

But in practice, in most contexts (like defining a click handler in React or Svelte), unless it were inline, you'd still need to explicitly specify the generic argument in your callback anyway, so it'd be no less typing saved.

• adding [k:string]:any to EventData because most EventData are augmented with data props or others.

I see the use - though I think the proper solution for this is for the library-maker to properly type their event handlers (e.g. say that the event named "MyEvent" corresponds to the EventData extension, MyEventData). We really need to get away from loose types and enforce strictness. For now, users can assert as MyEventData where libraries have mistakenly marked the EventData type as just EventData.

About the template type. A simple <T extends Observable = Observable> would help a lot not only for you and your callbacks but also for plugins developer creating custom event types. Plus it is the actual correct typing.

As for the other point I understand what you mean. It is just a pain to have tsc always giving errors about EventData properties while everyone know there is always other properties.

About the template type. A simple would help a lot not only for you and your callbacks but also for plugins developer creating custom event types. Plus it is the actual correct typing.

I can see that that change would touch a lot of files though, so to reduce the review burden, I'll try preparing a draft PR now that depends on this one that implements just that extra step.

@shirakaba it won't change anything! The default T makes it work without a single change !

@shirakaba it won't change anything! The default T makes it work without a single change !

It certainly wouldn't break anything, but... on, off, addEventListener, removeEventListener, and every interface extending EventData (of which there are many) would need to pass that generic argument around, otherwise there would be no point of use where we'd actually be benefitting from making EventData use a generic.

So I could do just that one-line change, but I don't see what it improves as we'd still have to be casting eventData.object as StackLayout as usual pretty much everywhere, as all the event handlers would still be typed as non-generic unless we followed this rabbit hole and got them all to pass the generic around. I think there's an extra PR's work in there to get it all hooked up.

@shirakaba yes it would be best. But as I said I could already use it in extending EventData or in my callbacks. It is already a free and useful gain.
And no would not have to cast to StackLayout if I use the T in my callback signature.
But any way I dont want to force you. Do as you see best

@farfromrefug I've put together a follow-up commit to implement generic events - as you can see, doing it end-to-end is a big change on top of this PR:

events-cleanup...generic-events

It would potentially be a breaking change for libraries/plugins - any class that were subclassing Observable would now have to add those generic types to the signatures of anything handling event listeners.

It probably wouldn't introduce any breaks for end-users.

But as it may be a breaking change for libraries, let's keep this commit for generics out of this PR and review it in a follow-up PR. Don't want to have to roll back this cleanup PR just due to finding a problem with the generics!

@shirakaba awesome. But I dont think it is a breaking change. You dont need to use generic as all methods / types have default generic.

The full commit to implement the generic end-to-end would be a compile-time breaking change - playground:

Any existing plugins extending Observable (or ViewBase or whatever) and overriding on or addEventListener, etc., would now find that they need to add the generic to the methods.

Just implementing the one-line change export interface EventData<T extends Observable = Observable>, though, would be non-breaking, I agree.

a doubt, this will make the type in the event for example: loaded, come defined for the object property?

This PR

@vallemar This PR will behave as follows:

The callback for the "loaded" event will return an EventData:

export abstract class View extends ViewCommon {
  // ...
  on(event: 'loaded', callback: (args: EventData) => void, thisArg?: any);
  // ...
}

EventData will look like this (note how object is no longer an optional or a Partial<Observable> anywhere):

export interface EventData {
  eventName: string;
  object: Observable;
}

Thus, usage will look like this:

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

So, if you want to access any Button-specific APIs, you will still have to cast the Observable to a Button. But if you only need to call Observable APIs, then no type-casting is necessary.

More than anything, the advantage is that the object is never Partial. And in strict-mode TypeScript, you no longer have to check whether it's undefined.

The end-goal

This PR paves the way for a follow-up PR (this commit: events-cleanup...generic-events ) where we'd introduce generics. That would improve the API to:

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

// Or (if I can set up the inference correctly by defaulting to the class itself):
new Button().on("loaded", (eventData) => {
  eventData.object; // TypeScript infers this to be a Button
});