chore(refCount): remove unnecessary reference of object #4720
Conversation
Pull Request Test Coverage Report for Build 8388
💛 - Coveralls |
|
I'm not sure how I feel about exposing all of these properties as public. They really aren't meant to be consumed by other users. |
githubxiaowen
force-pushed
the
feat-opti-refcount
branch
5 times, most recently
from
2d70c6c
to
02a9eac
Compare
githubxiaowen
force-pushed
the
feat-opti-refcount
branch
from
02a9eac
to
793056f
Compare
| protected _refCount: number = 0; | ||
| protected _connection: Subscription; | ||
| private _refCount: number = 0; | ||
| private _connection: Subscription; |
There was a problem hiding this comment.
This is technically a breaking change for TypeScript users.
There was a problem hiding this comment.
Yeah, as this would be a breaking change, I don't see any advantage to changing these to private. Especially as this is a 'perf' PR. This is unrelated to perf.
There was a problem hiding this comment.
reverted to protected
| } as MonoTypeOperatorFunction<T>; | ||
| } | ||
|
|
||
| class RefCountOperator<T> implements Operator<T, T> { | ||
| constructor(private connectable: ConnectableObservable<T>) { |
There was a problem hiding this comment.
Removing the constructor is also a breaking change.
There was a problem hiding this comment.
I'm fine with this. AFAICT, this is not a breaking change as RefCountOperator is not exported. And if someone has reached into the internals to get it and this breaks them, then that's on them for being naughty.
| protected _refCount: number = 0; | ||
| protected _connection: Subscription; | ||
| private _refCount: number = 0; | ||
| private _connection: Subscription; |
There was a problem hiding this comment.
Yeah, as this would be a breaking change, I don't see any advantage to changing these to private. Especially as this is a 'perf' PR. This is unrelated to perf.
| } as MonoTypeOperatorFunction<T>; | ||
| } | ||
|
|
||
| class RefCountOperator<T> implements Operator<T, T> { | ||
| constructor(private connectable: ConnectableObservable<T>) { |
There was a problem hiding this comment.
I'm fine with this. AFAICT, this is not a breaking change as RefCountOperator is not exported. And if someone has reached into the internals to get it and this breaks them, then that's on them for being naughty.
revert private to protected
|
Yeah，i gave it a second thought. This commit indeed is just removing the unnecessary reference of |
githubxiaowen
changed the title
This was mostly just to get CI to kick off again.
Again, I was really only doing this to kick off CI. LOL... My silly change in the dark from the GitHub UI didn't work. :)
Really I'm just kicking CI again. (and I made a dumb mistake on the last one)
Description:
Since the
sourceand theconnectableObservableis the same thing in RefCountOperator, there is no need to hold the variableconnectableint the RefCountOperatorRelated issue (if exists):
NONE