fix(Subject): do not expose static create method to inherited

[kwonoj]

Description:
This PR aligns interface of classes inherited from Subject<T>, do not expose static creation method by introducing abstract class SubjectBase<T>.

Related issue (if exists):

closes #1890

[kwonoj]

I'm feeling this might not be correct understanding of removing static interfaces or either incorrect implementation of having abstract classes - please feel freely close if PR is not feasible.

[coveralls]

Coverage increased (+0.0009%) to 97.114% when pulling 13d3358 on kwonoj:fix-subjectcreate into da8c1c2 on ReactiveX:master.

[kwonoj]

..Or instead static creation method to comply inherited classes (similar to #1876)

[jayphelps]

As discussed in #1890, there may be merit to adding support for create to other subject types, but in the interest of initial simplicity and correctness I think this is probably the right approach for now. This code looks good, but I'm worried that this is in fact a breaking change.

(new AsyncSubject) instanceof Subject will no longer pass, and that scares me and seems counter-intuitive. I'm probably being alarmist though..but I'm going to let others chime in here please.

[kwonoj]

(new AsyncSubject) instanceof Subject will no longer pass, and that scares me and seems counter-intuitive.

: I think that's legit point that I couldn't think of at the moment writing PR. do we have general usecases for checking types of inherited subjects?

[jayphelps]

Not really for instanceof..but I wouldn't be surprised to see it. More commonly I imagine this would bite people with type systems:

class ActionsObservable extends Observable<Action> {
  constructor(source$: Subject<Action>) {
    this.source = source;
  }
}

const source$ = new ReplaySubject<Action>();
const action$ = new ActionsObservable(source$);
// type error: ReplaySubject does not match type Subject

(we do this pattern, but not currently using TypeScript so wouldn't impact us at the moment)

This isn't the end of the world. It's just changing Subject -> SubjectBase obviously. But it's a breaking change.

[kwonoj]

gotcha, you're correct. This PR makes breaking change to consumers should accept SubjectBase as base class instead of explicit Subject. Seems change's more broad than I originally expected..

[kwonoj]

I tried small snippet here and at least as long as subject does not have it's own instance method type inference will work between subjectbase inherited vs subject itself. But as you've mentioned, I agree this isn't only cases hitting consumers and there might be more cases as well.

My current feeling is rather document Subject.create only works for Subject itself for now until fully resolve static creation method inheritance design and close this PR without merging. what do you think?

[kwonoj]

closing PR without check in as intended design is to support create method for all subject implementation.

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