-
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
fix(bindCallback): emit undefined when callback is without arguments #2328
fix(bindCallback): emit undefined when callback is without arguments #2328
Conversation
In resulting Observable emit undefined when callback is called without parameters, instead of emitting empty array.
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.
Change looks ok to me.
@@ -15,6 +15,7 @@ export class BoundCallbackObservable<T> extends Observable<T> { | |||
subject: AsyncSubject<T>; | |||
|
|||
/* tslint:disable:max-line-length */ | |||
static create(callbackFunc: (callback: () => any) => any, selector?: void, scheduler?: IScheduler): () => Observable<void>; |
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.
It's not related to this PR, but I just noticed bindNodeCallback
doesn't have corresponding overloading signatures. Maybe we need this? /cc @david-driscoll
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.
Now that I think about it, I have seen some operators have type-only tests. Maybe I should add such tests here?
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.
No, that is intended for specific cases if current test case doesn't covers it. it is worth to create those and it'll be appreciated, but I'd like to suggest to separate with this PR since each has different scope.
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
Related to issue #2254 in
bindNodeCallback
. It turns outbindCallback
has the same problem:When wrapped function calls its callback without any arguments, resulting Observable will emit empty array, intead of undefined.
In RxJS 4 in
fromCalllback
resulting Observable emits undefined as seen here:https://github.com/Reactive-Extensions/RxJS/blob/master/src/core/perf/operators/fromcallback.js#L20-L21
(when array is empty, calling
results[0]
results inundefined
, which is emitted).Related to
#2254