Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(Subject): introduce interface to Subject #2279

Closed
wants to merge 1 commit into from
Closed

refactor(Subject): introduce interface to Subject #2279

wants to merge 1 commit into from

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Jan 14, 2017 •
edited
Loading

Description:
This PR enhances type definition of subject separated from #2233, similar to #2249.

This change introduces new interface ISubject<T> have more clear type definition regarding its properties, and let static creation method returns interface instead of actual implementation type.

Related issue (if exists):

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.696% when pulling 18b729c on kwonoj:refactor-isubject into dd925a8 on ReactiveX:master.

@kwonoj kwonoj force-pushed the refactor-isubject branch from 18b729c to 960a641 Compare January 16, 2017 19:53
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.698% when pulling 960a641 on kwonoj:refactor-isubject into 31cf2bf on ReactiveX:master.

@jayphelps
Copy link
Member

jayphelps commented Jan 27, 2017 •
edited
Loading

So this is only not a breaking change because TS is structurally typed?

Can you clarify specifically what is more clear to users with this change? So they don't have all the other extraneous signatures?

@kwonoj
Copy link
Member Author

kwonoj commented Jan 27, 2017

It is not breaking since this PR only extracts out interface shape from existing Subject implementation. It isn't being actively used elsewhere, so while it gives clarity who reads interface of Subject but actual type inferences related with subject won't benefit hugely. Previous PR was actually changing runtime behavior to prevent unintended interface access though.

@jayphelps
Copy link
Member

@kwonoj hmmm but isn't Subject.create() a public API?

// some user code that assumes Subject, not ISubject
function gimmieStuff(stuff: Subject) {}

let stuff = Subject.create(...);
gimmieStuff(stuff);

@kwonoj
Copy link
Member Author

kwonoj commented Jan 27, 2017

@jayphelps , it is. My comment is based on most of subject creation is currently recommended via instantiation but not by static creation method. Anyway there are opened issue for definition of static subject creation method. #2004 too.

benlesh
benlesh requested changes Jan 29, 2017
View reviewed changes
src/Subject.ts Outdated
@@ -16,40 +16,43 @@ export class SubjectSubscriber<T> extends Subscriber<T> {
}
}

export interface ISubject<T> extends ISubscription, Observable<T> {
readonly observers: ReadonlyArray<Observer<T>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is sort of an implementation detail of Subject.

src/Subject.ts Outdated
export interface ISubject<T> extends ISubscription, Observable<T> {
readonly observers: ReadonlyArray<Observer<T>>;
readonly closed: boolean;
readonly isStopped: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isStopped is also an implementation detail.

src/Subject.ts Outdated
@@ -16,40 +16,43 @@ export class SubjectSubscriber<T> extends Subscriber<T> {
}
}

export interface ISubject<T> extends ISubscription, Observable<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does ISubject not extend an Observer interface as well?

src/Subject.ts Outdated
/**
* @class Subject<T>
*/
export class Subject<T> extends Observable<T> implements ISubscription {
export class Subject<T> extends Observable<T> implements ISubject<T> {

[$$rxSubscriber]() {
return new SubjectSubscriber(this);
}

observers: Observer<T>[] = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably be protected.

src/Subject.ts Outdated
closed = false;

isStopped = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably be protected

@benlesh
Copy link
Member

benlesh commented Jan 29, 2017 •
edited
Loading

So where are we returning ISubject? or expecting ISubject? How does this typing benefit us? multicast is the only thing I can think of, off the top of my head.

@Igorbek
Copy link
Contributor

Igorbek commented Feb 1, 2017

If an interface being extracted, why doesn't it follow the classic definition of the Subject where it is just simply that:

interface ISubject<T> extends /*IObserver<T>*/ Observer<T>, /*IObservable<T>*/ Subscribable<T> { }

all other attributes would make the abstraction leaking (as do Observer and Subscribable currently).

@benlesh
Copy link
Member

benlesh commented Feb 17, 2017

cc @kwonoj ... I'm generally in favor of this change... can you make the changes I've requested? Or rebutt them I guess? haha. :)

@benlesh
Copy link
Member

benlesh commented Feb 17, 2017

Also... I think this is a MINOR version change, since it's not really breaking, but it does sort of add a feature. Opinions? @staltz? @trxcllnt?

@kwonoj
Copy link
Member Author

kwonoj commented Feb 17, 2017

@Blesh oh yes, I just forgot this one :/

and I think this can be definitely minor, as it doesn't breaking existing behavior in most cases.

@kwonoj
Copy link
Member Author

kwonoj commented Feb 21, 2017

Updated PR as suggested. /cc @Blesh

@kwonoj kwonoj mentioned this pull request Feb 21, 2017
Closed
@staltz
Copy link
Member

staltz commented Feb 21, 2017

Seems like a MINOR. Adds feature with backwards compat.

@kwonoj kwonoj added the blocked label Mar 12, 2017
@kwonoj
Copy link
Member Author

kwonoj commented Mar 12, 2017

So, making interface clear makes cannot build this, as some of internal implementation (SubjectSubscription, COnnectableObservable) was relying on incorrectly publicly-exposed ones. :/

@kwonoj kwonoj force-pushed the refactor-isubject branch from a1d5862 to 3e32a35 Compare March 24, 2017 17:33
@kwonoj kwonoj force-pushed the refactor-isubject branch from 3e32a35 to 1133b66 Compare March 24, 2017 17:42
@coveralls
Copy link

coveralls commented Mar 24, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 97.698% when pulling 960a641 on kwonoj:refactor-isubject into 31cf2bf on ReactiveX:master.

@benlesh
Copy link
Member

benlesh commented Jan 23, 2018

@kwonoj.. this seems to have gone stale. Can we close it?

@kwonoj
Copy link
Member Author

kwonoj commented Jan 23, 2018

I still believe this need to be landed in master in some way, but this PR itself has too diverged. Closing it and let me create other PR based on TOT.

@kwonoj kwonoj closed this Jan 23, 2018
@kwonoj kwonoj deleted the refactor-isubject branch January 23, 2018 03:22
@lock
Copy link

lock bot commented Jun 6, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@benlesh benlesh benlesh requested changes

Assignees
No one assigned
Labels
blocked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kwonoj @coveralls @jayphelps @benlesh @Igorbek @staltz