refactor(Observable): Subjects no longer wrapped in Subscriber

[ntilwalli]

Observable.subscribe updated to check for instanceof Subject, test added, import order changed in Rx and KitchenSink to avoid circular dependency issue. closes #825, closes #748

[ntilwalli]

This PR addresses the issue where Subjects are unnecessarily wrapped in a Subscriber during Observable.subscribe. Conceptually there is no reason they should be since they are designed to be both Observable and Subscriber. This fix does introduce a circular dependency between Subject and Observable that must be managed during initial import in Rx and KitchenSink. Is introducing module load ordering dependencies okay? (Also after checkin I noticed I forgot to remove a commented out line of code in Observable.ts)

[benlesh]

LGTM.

[benlesh]

@ntilwalli can you rephrase this commit as refactor(Observable): Subjects no longer wrapped in Subscriber with a description of the work done, also add closes #825, closes #748 to the commit message at the end.

The reason for the change from fix to refactor, is technically the functionality wasn't "broken" before, it was just sub-optimal.

Other than that, LGTM! Thanks for the PR!

[kwonoj]

would you able to remove comment if this is leftover?

[kwonoj]

Except one minor comment about commented code, change looks good to me too.

[ntilwalli]

Great. Done.

[kwonoj]

@ntilwalli , would you able to flat 2 commit 42258b2 & 849d5b2 into single one? both has identical commit message, and change seems quite similar too.

[staltz]

From issue #825, @Blesh said:

Subjects are being unnecessarily wrapped in Subscribers

I originally said the same. But after looking at code, I noticed that Subjects are not Subscribers. A Subject is an Observable and an Observer. And a Subscriber is an Observer. Every Observer is wrapped by a Subscriber when we subscribe with an observer. That's why a Subject is wrapped by a Subscriber.

Look at this inheritance diagram:

Yet this PR introduces:

if (observerOrNext instanceof Subscriber || observerOrNext instanceof Subject) {
  subscriber = (<Subscriber<T>> observerOrNext);
}

Does TypeScript allow us to (unsafe?) cast observerOrNext to Subscriber if it's a Subject?

[kwonoj]

Does TypeScript allow us to (unsafe?) cast...

: Since typescript only does type inference on build time, above code snippet could get passed.

observerOrNext is union type with primary type of Observer<T>, so regardless of runtime type checking (observerOrNext instanceof ...) typescript allows given parameter to be casted as Subscriber. Course, when code is being executed if assigned value is not Subscriber, it'll cause runtime issue.

if you're trying to cast Subject explicitly to Subscriber it'll complains.

Anyway, you're inheritance diagram seems legit, Subject seems does not inherit Subscriber.

[kwonoj]

Removed code snippet that I was thinking incorrectly.

[kwonoj]

code is being executed if assigned value is not Subscriber, it'll cause runtime issue.

: and seems this is not occurring since execution will reach add(...) which is available implementation on Subject & Subscriber both.

[staltz]

Is reimplementing Subject as extending both Observable and Subscriber too much work? Since Subject implements both Observer and Subscription anyway.

[kwonoj]

I'm curious if subject need to extend subscriber. Nature of subject requires to be Observable and Observer as minimum - is there any specific usecases subscriber would benefit subject?

thinking about inheritance & implementation hierarchy, extending subscriber will makes subject

Subject<T> extends Observable<T>, Subscriber<T> implements Observer<T>, Subscription<T> (pseudo, let's forget about typescript doesn't support multiple inheritance. course in practical code it'll be more complex cause it need workaround for multiple inheritance..)

makes subject quite complex object.

[staltz]

makes subject quite complex object.

I know, so maybe it's ok to keep it wrapped in a Subscriber.

[ntilwalli]

@staltz said:

Is reimplementing Subject as extending both Observable and Subscriber too much work? Since Subject >implements both Observer and Subscription anyway.

What does reimplementing mean? Since Typescript is purely duck-typed, from a compile-time/type-checking/semantic perspective, there's no difference between the statements: The code in Subject as it currently stands is shown in statement 1 from below but effectively implements 2 which is an even stronger statement.

1. class Subject extends Observable<T> implements Observer<T>, Subscription<T>
2. class Subject extends Observable<T> implements Subscriber<T>

As long as the implementation of Subject adheres to the stronger contract of statement 2, the cast is safe and there would be no need to reimplement anything in Subject, except to possibly update Subject to statement 2 for added clarity. The discussion though seems to imply that the contract of Subject should be weakened to be:

class Subject extends Observable<T> implements Observer<T>

If that's the case, this PR is not valid and there should be a PR to modify Subject accordingly. When I first proposed this issue, I assumed Subject was coded correctly and Observable was inconsistent. It sounds like you're saying it's the other way around.

[ntilwalli]

I edited the above comment to fix a reasoning error and to be more precise as to my intended meaning.

[benlesh]

It will be fine. Are we tricking TypeScript? Yes. But we're doing it because TypeScript types aren't a real thing, they're helpful for build-time and development-time sanity checks. When you see <casting>obj anywhere, it's basically saying "I don't want your sanity checks, because I know what I'm doing". In this case, it works out. Subject and Subscriber have all of the same API points. This is by design.

This PR should be a small performance improvement when using Subjects to subscribe, and I don't see anything about it that could be bad. ... Other than people who really like typing freaking out a little because we're being dirty, rotten liars about Subject being a Subscriber on that one line.

[ntilwalli]

@Blesh @staltz @kwonoj I've flattened the commits.

[kwonoj]

@ntilwalli Thanks! I'll check in this after waiting couple of more hrs to see if there's any second opinion.

[staltz]

This PR should be a small performance improvement when using Subjects to subscribe, and I don't see anything about it that could be bad. ... Other than people who really like typing freaking out a little because we're being dirty, rotten liars about Subject being a Subscriber on that one line.

Fair enough...

[kwonoj]

Merged with 5cb0f2b, thanks @ntilwalli !

[ntilwalli]

@kwonoj I just realized, when flattening the commits, I forgot to update the commit message to say "refactor" as @Blesh asked. It currently says "fix". Should I amend my message and push it again?

[kwonoj]

Whew, sorry for that. I should've keep track detail before merge.. I think current branch is protected from force update, would like to hear @Blesh which would be best.