Observable and EventData cleanup

[shirakaba]

This PR cherry-picks some of the changes done in the course of #10100. It does not implement DOM Events; it just cleans up the existing Observable and EventData source code to reduce the changes needed for DOM Events support (whether implemented into Core or as a separate opt-in package).

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?

Inconsistent EventData typings, with lots of object?: Partial<Observable>, and thisArg not being passed along and/or correctly typed here and there.

We also have conflicts between observable/index.d.ts and observable/index.ts.

What is the new behavior?

Consistent, simplified, correct typings. One singular observable/index.ts.

Fixes/Implements/Closes #[Issue Number].

No breaking changes anticipated.

[nx-cloud]

☁️ 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

---

✅ Successfully ran 2 targets

• nx run-many --target=test --configuration=ci --projects=core
• nx run webpack5:build

---

Sent with 💌 from NxCloud.

[shirakaba]

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.

[farfromrefug]

Looks awesome to me as it is! 2 things i would love to see added:

• template type for EventData T extending Observable to define "object"
• adding [k:string]:any to EventData because most EventData are augmented with data props or others.

[shirakaba]

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.

[farfromrefug]

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.

[shirakaba]

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.

[farfromrefug]

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

[shirakaba]

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

[farfromrefug]

@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

[shirakaba]

@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!

[farfromrefug]

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

[shirakaba]

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.

[farfromrefug]

You dont need to pass object:this here. It is added by default from notifiy (if no object set)

[shirakaba]

This PR actually removes the logic in Observable.prototype.notify that fills in an empty object field, so it's necessary (unless we decide to put that logic back in):

- public notify<T extends NotifyData>(data: T): void {
-   const eventData = data as EventData;
-   eventData.object = eventData.object || this;
+ public notify<T extends EventData>(data: T): void { 

Demanding that object be filled in allows us greatly simply the EventData typings. The user gets the same EventData out as they put in. No partials or optionals or anything, and no NotifyData vs. EventData. I think there's a great advantage to simplicity when the codebase has turned off strict type-checking.

In terms of performance as well, making the users pass in the EventData in its final shape rather than us potentially altering EventData, it may go (in some cases, a tiny bit) faster, as V8 won't have to re-assess its Shape upon the reassignment. Although maybe that's only when JIT is enabled...

But I can understand that it's less typing for the user, which is an advantage. I wonder whether this would be an okay middle-ground:

+ // Makes the specified properties in a type optional.
+ type Optional<T, K extends keyof T> = Pick<Partial<T>, K> & Omit<T, K>;
+ 
- public notify<T extends NotifyData>(data: T): void {
+ public notify<T extends EventData>(data: Optional<T, 'object'>): void { 
-   const eventData = data as EventData;
    eventData.object = eventData.object || this;

[farfromrefug]

@shirakaba I missed that change but that would be a breaking change though. I know i have many plugins depending on that.

[shirakaba]

How about the middle ground I suggested, though, which preserves the optional member?

[shirakaba]

I applied a solution in the commit fix: allow "object" to be optional in notify(). Hope you like it!

[farfromrefug]

wouldn't it be cleaner to do

import * as IApplication from '.';
... 
export const on = global.NativeScriptGlobals.events.on.bind(global.NativeScriptGlobals.events) as typeof IApplication.on;

I have not "tested" this but i guess it should work

[shirakaba]

Now done in the commit chore: cleaner types using IApplication.

[farfromrefug]

Same as before. Wouldn't it be cleaner to import data/observable as Observable from top and use it here?
maybe it wasn't working?

[shirakaba]

I'll have to check. I never have much luck defining imports in .d.ts files (have to be very careful not to accidentally turn it into a module, causing it to lose its ambient context). But happy to give that a try.

... Side note, changes to this file are a moot point in the first place as the global types have been broken since @types/node was updated to version 16, as they changed the way you have to augment the global/globalThis context. I have a branch fix/global-types that fixes them that I haven't opened a PR for yet.

[shirakaba]

I tried out a few different approaches in a new minimal project using @types/node@14 (which still uses NodeJS.Global).

❌ Attempt one: importing at top of file

This doesn't work (as expected), as the top-level import changes global-types.d.ts into a module, so it stops filling ambient types.

❌ Attempt two: importing within the namespace

This doesn't work either. We'd expect events to bear the eventOne key, but it doesn't auto-complete.

⚠️ Attempt three: creating a type alias within the namespace

This does work, but it leaves some pollution - it now means that the users have to put up with NodeJS.Observable being in tsc's compilation context. i.e. they can write const obs = new Observable as NodeJS.Observable.

✅ Original attempt

The original suggestion gets working Intellisense without leaving any pollution in the user types.

So I think we'd best stick with the code in the PR!

[farfromrefug]

not needed. Added by default and less JS code

[shirakaba]

Now reverted.

[vallemar]

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

[shirakaba]

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
});