fix(Subscriber): do not call complete with undefined value param #2559
Conversation
src/Subscriber.ts
Outdated
| try { | ||
| fn.call(this._context, value); | ||
| fn.call(this._context, ...value); |
There was a problem hiding this comment.
@kwonoj since this is a hot code path, can we avoid allocating the rest args Array with a third boolean argument as a flag?
There was a problem hiding this comment.
mean like __tryOrUnsub(fn: Function, value: Array<any>, passValue: boolean)?
There was a problem hiding this comment.
I can, it's eventually same. I just picked rest operator for simplicity of readability.
There was a problem hiding this comment.
@kwonoj yeah but it transpiles down to an Array allocation + apply call, which is 20-25x slower than the equivalent conditional call in this jsperf test. so it's important in this code path.
There was a problem hiding this comment.
ah yes, what I meant of same is the perspective of consequences only without perf differences. again, I just picked it up for readability at the moment.
kwonoj
force-pushed
the
fix-subscribe-complete
branch
from
4bae1c0
to
10dcdd4
Compare
| @@ -234,11 +234,13 @@ class SafeSubscriber<T> extends Subscriber<T> { | |||
| if (!this.isStopped) { | |||
| const { _parentSubscriber } = this; | |||
| if (this._complete) { | |||
| const wrappedComplete = () => this._complete.call(this._context); | |||
There was a problem hiding this comment.
@trxcllnt what about this approach? create one fn closure for complete only, and let __try... avoid any branching (if, or rest operator...). There is overhead to create one additional closure, but only for completion and will not impact next.
There was a problem hiding this comment.
@kwonoj that looks great! I like how it removes the branching in the next case
spec/Subscriber-spec.ts
Outdated
| const sub1 = new Subscriber(observer); | ||
| sub1.complete(); | ||
|
|
||
| expect(observer.complete.firstCall.args).to.empty; |
There was a problem hiding this comment.
Also, I don't know enough about sinon to know if this is how we check this. It seems like a more straight forward test just using a plain object and expect(arguments.length).to.equal(0) would do.
There was a problem hiding this comment.
yeah, probably simple test like this could be achieved in those way. I'll update test.
kwonoj
force-pushed
the
fix-subscribe-complete
branch
from
10dcdd4
to
56ce619
Compare
|
test cases are updated as suggested. |
|
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. |
Description:
This PR makes to call observer's complete function without delivering one param as undefined value all times. While observer is requested to follow its contract interfaces, change seems trivial to avoid this behavior in code as well.
I think this is non-breaking changes maybe?
Related issue (if exists):