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

fix(bindCallback): emit undefined when callback is without arguments #2328

Merged
merged 1 commit into from
Feb 14, 2017
Merged

fix(bindCallback): emit undefined when callback is without arguments #2328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions spec/observables/bindCallback-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,23 @@ const Observable = Rx.Observable;
/** @test {bindCallback} */
describe('Observable.bindCallback', () => {
describe('when not scheduled', () => {
it('should emit undefined from a callback without arguments', () => {
function callback(cb) {
cb();
}
const boundCallback = Observable.bindCallback(callback);
const results = [];

boundCallback()
.subscribe((x: any) => {
results.push(typeof x);
}, null, () => {
results.push('done');
});

expect(results).to.deep.equal(['undefined', 'done']);
});

it('should emit one value from a callback', () => {
function callback(datum, cb) {
cb(datum);
Expand Down Expand Up @@ -104,6 +121,25 @@ describe('Observable.bindCallback', () => {
});

describe('when scheduled', () => {
it('should emit undefined from a callback without arguments', () => {
function callback(cb) {
cb();
}
const boundCallback = Observable.bindCallback(callback, null, rxTestScheduler);
const results = [];

boundCallback()
.subscribe((x: any) => {
results.push(typeof x);
}, null, () => {
results.push('done');
});

rxTestScheduler.flush();

expect(results).to.deep.equal(['undefined', 'done']);
});

it('should emit one value from a callback', () => {
function callback(datum, cb) {
cb(datum);
Expand Down
5 changes: 3 additions & 2 deletions src/observable/BoundCallbackObservable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Copy link
Member

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

Copy link
Contributor Author

@mpodlasin mpodlasin Feb 6, 2017 •
edited
Loading

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?

Copy link
Member

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.

static create<R>(callbackFunc: (callback: (result: R) => any) => any, selector?: void, scheduler?: IScheduler): () => Observable<R>;
static create<T, R>(callbackFunc: (v1: T, callback: (result: R) => any) => any, selector?: void, scheduler?: IScheduler): (v1: T) => Observable<R>;
static create<T, T2, R>(callbackFunc: (v1: T, v2: T2, callback: (result: R) => any) => any, selector?: void, scheduler?: IScheduler): (v1: T, v2: T2) => Observable<R>;
Expand Down Expand Up @@ -119,7 +120,7 @@ export class BoundCallbackObservable<T> extends Observable<T> {
subject.complete();
}
} else {
subject.next(innerArgs.length === 1 ? innerArgs[0] : innerArgs);
subject.next(innerArgs.length <= 1 ? innerArgs[0] : innerArgs);
subject.complete();
}
};
Expand Down Expand Up @@ -157,7 +158,7 @@ export class BoundCallbackObservable<T> extends Observable<T> {
self.add(scheduler.schedule(dispatchNext, 0, { value: result, subject }));
}
} else {
const value = innerArgs.length === 1 ? innerArgs[0] : innerArgs;
const value = innerArgs.length <= 1 ? innerArgs[0] : innerArgs;
self.add(scheduler.schedule(dispatchNext, 0, { value, subject }));
}
};
Expand Down