-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
BIG: Subject rewrite #1701
BIG: Subject rewrite #1701
Conversation
NOTE: Also fixes the behavior of Once the Subjects were refactored to match Rx 4 behavior, groupBy was broken, in looking into that, it seemed the behavior was slightly different as well. So I fixed groupBy and some tests. |
super(); | ||
this.scheduler = scheduler; | ||
this.bufferSize = bufferSize < 1 ? 1 : bufferSize; | ||
this._windowTime = windowTime < 1 ? 1 : windowTime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like these should have a consistent naming scheme. Either use the prefixed '_' or don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree... but that's been sitting there, and I was already knee-deep in yak hair.
Any performance impact of the rewrite? |
BREAKING CHANGE: Subjects no longer duck-type as Subscriptions BREAKING CHANGE: Subjects will no longer throw when re-subscribed to if they are not unsubscribed BREAKING CHANGE: Subjects no longer automatically unsubscribe when completed or errored BREAKING CAHNGE: Minor scheduling changes to groupBy to ensure proper emission ordering
@jadbox overall the tests seem to be positive, in that this is a little faster. However, the tests I wrote were having some issues where V8 was optimizing |
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. |
This is a rewrite of Subjects for RxJS 5 with the following effects:
This is a big set of changes. Breaking changes are listed in the commit.
Attn: @trxcllnt @mattpodwysocki @staltz @kwonoj @jayphelps