refactor(Notification): Define a private 'isObserver' type guard.

[tetsuharuohzeki]

No description provided.

[benlesh]

This is a bit tricky... technically any object is a valid Observer.

Observers aren't required to have next, error or complete. They could have any combination of them, including none at all.

This is valid:

observable.subscribe({ error(err) { console.log('I have an error'); });

What you've implemented is a test for isCompleteObserver.

We could also test for isNotObserver(o) which would be something like:

function isNotObserver(o: any): boolean {
  return !o || (typeof o.next  !== 'function' && typeof o.error  !== 'function' && typeof o.completed  !== 'function');
}

[benlesh]

Also, we're not using this functionality in more than one spot yet, I don't think. So we probably don't need to abstract it out just yet. If this were an end-user app, or any more than a one-liner, then absolutely I'd say we should abstract it into its own call.

What do you think?

[david-driscoll]

Feels like the cast to <Observer<T>> is a little miss-leading.

I could see type guards for isNextObserver, isCompleteObserver and isErrorObserver but I don't know how much value that would really add.

[tetsuharuohzeki]

@Blesh @david-driscoll

I agree these points:

• At this moment, we don't have to add to Observer.ts as general functions
• But the casting <Observer<T>> is a little miss-leading.

I think we should change a Notification.observe() to the following and I'll create a patch:

type NextObserver<T> =  {
  next(value: T): void;
};
type ErrorObserver = {
   error(error: any): void;
};
type CompleteObserver = {
   complete(): void;
};

class Notification<T> {
observe(observer: NextObserver<T>  | ErrorObserver | CompleteObserver): any {...}
}

How about this?

[kwonoj]

would you able to elaborate bit more details of reason Observer<T> is misleading?

[tetsuharuohzeki]

would you able to elaborate bit more details of reason Observer is misleading?

As my thought, Notification.observe() & the 1st argument of Notification.accept() don't need to take a full Observer<T>. Its observer don't have to implement isUnsubscribed, and all members of Observer<T>. at same time.

[tetsuharuohzeki]

I pushed a new patch and changed this pull request's title.

@Blesh @kwonoj @david-driscoll how don you think about this new change?

[kwonoj]

Notification::observe() requires full observer implementation, isn't it?

My personal take on this is, revised interface does not gives clarity. Previous signature explicitly requires Observer as parameter so it's expected to fail if given object is missing implementation from it. Now revised interface, signature allows partial implementation of observer, but may possibly throw unexpectedly by given object is missing it per value of Notification holds.

For same reason, accept still needs to require Observer, or partial implementations of it. If it's full Observer accept will invoke observe internally, now those semantics are not working as same as above.

[tetsuharuohzeki]

Notification::observe() requires full observer implementation, isn't it?

Umm, I'll reorganize.

1. Rx v5 includes isUnsubscribed to Observer<T> but Rx v4 doesn't include it as the part of Observer<T>.
2. By the code, Notification.observe() does not use Observer<T>.isUnsubscribed. In theory, we don't have to take a full Observer<T> as arguments of Notification.observe()/Notification.accept()
3. But in the v4, Notification.accept() take a v4's Observer<T> as the argument.

By these points, we should take a Observer<T> as the argument of them (so this current patch is wrong!). So basically, we should define a private type guard function which checks whether the argument has notify(), error(), and complete().

To follow the v5's type definitions strictly, we also need to check typeof v.isUnsubscribed === 'boolean'. But this checking would potentially break a code which is on v4.

As my thought, we don't have to check typeof v.isUnsubscribed === 'boolean' because this guard function would be a private function.

[kwonoj]

I'm bit fence on this, this provides more strict type guarding means bringing additional checkers. Wish do we have some numbers to compare with. In any case, @saneyuki , I think commit history need to be aligned to remove some of previous commit (i.e, Revert ....)

[benlesh]

I have an alternative PR that I think fits a little nicer into what we should do with observe for Notification. #1260 ... have a look and see what you think.

[kwonoj]

@saneyuki , I think #1260 is aiming original changes of this PR which allows partial observer implementation - what about continue discussion on those PR?

[benlesh]

#1282 should solve the issues this was trying to solve, as well as fix the signatures of things like subscribe to accept partial observers.

@saneyuki, please have a look and see if you feel this PR can be closed in favor of that?

[tetsuharuohzeki]

#1282 would be more good place for the discussion. I'll close this.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.