feat(callback): Add Observable.fromCallback #729

SomeKittens · 2015-11-15T04:51:49Z

No description provided.

SomeKittens · 2015-11-15T04:55:26Z

Ran into an odd situation. With the additional tests included in this PR, I get this error:

Failures:
1) Observable.prototype.bufferWhen should NOT handle hot inner empty
  Message:
    RangeError: Maximum call stack size exceeded
  Stack:
    RangeError: Maximum call stack size exceeded

removing any one of the tests (or any test in from-promise-spec.js, but not debouncetime-spec.js, the other two I tried) results in bufferWhen passing.

I'm guessing it's a race condition somewhere, with the extra tests, the tests are running long enough to trigger the overflow. Changing the test to the following results in it passing though I'm not sure if it invalidates the test:

  // bufferWhen is not supposed to handle a factory that returns always empty
  // closing Observables, because doing such would constantly recreate a new
  // buffer in a synchronous infinite loop until the stack overflows. This also
  // happens with buffer in RxJS 4.
  it('should NOT handle hot inner empty', function (done) {
    var source = Observable.of(1, 2, 3, 4, 5, 6, 7, 8, 9);
    var closing = Observable.empty();
    var TOO_MANY_INVOCATIONS = 30;

    var invoked = 0;
    source
      .bufferWhen(function () {
        if (invoked <= TOO_MANY_INVOCATIONS) {
          return closing;
        }
        return Observable.interval(100);
      })
      .subscribe(function (val) {
        invoked++;
        if (invoked > TOO_MANY_INVOCATIONS) {
          return done();
        }
        expect(Array.isArray(val)).toBe(true);
        expect(val.length).toBe(0);
      }, null, null);
  });

SomeKittens · 2015-11-15T05:07:24Z

Ah, missed linting, fixing that now.

kwonoj · 2015-11-15T05:15:10Z

src/observables/CallbackObservable.ts

+import {tryCatch} from '../util/tryCatch';
+import {errorObject} from '../util/errorObject';
+
+export class CallbackObservable<T> extends Observable<T> {


What about align class name to observable, such as Observable.fromEvent has implementation named FromEventObservable?

I tried to mimic PromiseObservable with regards to that sort of thing.

Yes, just noticed that too.

SomeKittens · 2015-11-15T05:15:39Z

Tried to match things to what's going on in PromiseObservable.ts with regards to scalar things and schedulers. Not 100% confident, I'd appreciate it if you'd go over that section with a fine-tooth comb.

kwonoj · 2015-11-15T05:23:09Z

src/observables/CallbackObservable.ts

+        };
+      }
+    } else {
+      let subscription = new Subscription();


this variable seems could be const

kwonoj · 2015-11-15T05:25:31Z

Appreciate for effort, I didn't even noticed fromCallback is yet implemented :) I'll try to add some comment.

SomeKittens · 2015-11-18T19:43:15Z

Hello! Any update on this?

kwonoj · 2015-11-18T19:56:16Z

I am generally ok with current changes, but would like to ask @Blesh 's opinion also may have better suggestions than me. Also, testing failure you've mentioned makes this as bit blocked, since those issue need to be addressed and resolved to correctly merge this PR.

SomeKittens · 2015-11-18T20:00:22Z

Yep, willing to do whatever's needed to get those tests passing but I'm stumped. I can put in the fix I mentioned if you or @Blesh are ok with it (not sure if it breaks the spirit of the test).

kwonoj · 2015-11-18T21:15:14Z

Just rough guess, maybe test cases observable is not unsubscribed?

could possibly revise test case like below, what do you think @staltz ?

source.bufferWhen(function () { return closing; })
      .takeWhile(function(val, index) {
        return index < TOO_MANY_INVOCATIONS;
      })
      .subscribe(function (val) {
        expect(Array.isArray(val)).toBe(true);
        expect(val.length).toBe(0);
      }, done.fail, done);

benlesh · 2015-11-18T22:19:18Z

Sometimes async tests in jasmine get corrupt somehow. I'm not entirely sure what causes it, but I've seen it before. Anecdotally, I've pulled the PR down locally and it runs fine.

However, I don't think this PR matches with what fromCallback did previously. I'll add some comments to the commits.

benlesh · 2015-11-18T22:22:05Z

However, I don't think this PR matches with what fromCallback did previously.

... ignore that, I was just confused by one of the tests. Perhaps the test just needs refactored a little for readability.

SomeKittens · 2015-11-19T04:46:57Z

@Blesh Fixed, test is more specific now.

staltz · 2015-11-19T09:25:24Z

src/observables/CallbackObservable.ts

+    };
+  }
+
+  constructor(private callbackFunc: Function, private ctx, private selector, private args: any[], public scheduler: Scheduler = immediate) {


Can you please give a newline for each argument? Like we do elsewhere, see e.g. https://github.com/ReactiveX/RxJS/blob/master/src/observables/IteratorObservable.ts#L61-L65

staltz · 2015-11-19T09:48:33Z

Thanks @SomeKittens for this PR.

SomeKittens · 2015-11-19T19:45:34Z

@staltz as to the complexity of _subscribe, I based this off https://github.com/ReactiveX/RxJS/blob/master/src/observables/PromiseObservable.ts

Each path is slightly unique, not sure what to target for refactor.

SomeKittens · 2015-11-19T19:52:37Z

(also rebased @kwonoj's test fixes into this branch)

staltz · 2015-11-20T00:05:59Z

spec/observables/from-callback-spec.js

+        expect(err.message).toBe('Yikes!');
+        done();
+      },
+      null


This null here should also be done.fail

SomeKittens · 2015-11-20T01:40:34Z

@staltz fixed missed null.

staltz · 2015-11-20T01:45:21Z

Great

SomeKittens · 2015-11-20T22:16:55Z

Anything else?

benlesh · 2015-11-20T23:19:49Z

src/observables/CallbackObservable.ts

+        subscriber.complete();
+      } else {
+        handler = (...innerArgs) => {
+          let results;


you can move let results down to where it's being used on line 52

benlesh · 2015-11-20T23:33:42Z

Disregard my comments. I never use this creation method.

Frankly I think this is really a weird experience, and I dislike how it works. Every other fromWhatever method returns an Observable, not a function that returns a stateful observable.

benlesh · 2015-11-20T23:35:12Z

This LGTM. However, I'm not sure about including it in Core.

SomeKittens · 2015-11-20T23:47:24Z

@Blesh Agreed on it being weird, but that's how it works in Rx4 (and for that matter, Bluebird and the whole promise spectrum). I'm open to updating the signature to return an observable, possibly passing in the callback args.

SomeKittens · 2015-11-25T06:17:09Z

Sorry to continue pinging everyone, but is there anything else here that needs fixing? (or is the holdup deciding if this should be in core or not?)

benlesh · 2015-11-25T17:43:01Z

or is the holdup deciding if this should be in core or not?)

The hold up is mostly just me being too busy. :) Reviewing this now.

benlesh · 2015-11-25T17:46:19Z

Merged as of f6cebb6, with some minor lint fixes. Thanks @SomeKittens! This was a nice PR.

SomeKittens · 2015-11-28T23:52:22Z

The hold up is mostly just me being too busy. :) Reviewing this now.

It happens, especially around this time of year. Thanks!

kwonoj reviewed Nov 15, 2015
View reviewed changes

SomeKittens force-pushed the from-callback branch 2 times, most recently from 2a28cbe to c1e30fe Compare November 15, 2015 18:36

kwonoj mentioned this pull request Nov 16, 2015

Add Observable.fromNodeCallback #736

Closed

kwonoj mentioned this pull request Nov 19, 2015

fix(test): make explicit unsubscription for observable #754

Merged

SomeKittens force-pushed the from-callback branch from c1e30fe to f253326 Compare November 19, 2015 04:46

staltz reviewed Nov 19, 2015
View reviewed changes

SomeKittens force-pushed the from-callback branch from f253326 to 2760c50 Compare November 19, 2015 19:43

SomeKittens force-pushed the from-callback branch from 2760c50 to c81c413 Compare November 19, 2015 19:52

staltz reviewed Nov 20, 2015
View reviewed changes

spec/observables/from-callback-spec.js

expect(err.message).toBe('Yikes!');

done();

},

null

Copy link

Member

staltz Nov 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This null here should also be done.fail

feat(callback): Add Observable.fromCallback

b4fa0c6

SomeKittens force-pushed the from-callback branch from c81c413 to b4fa0c6 Compare November 20, 2015 01:40

benlesh reviewed Nov 20, 2015
View reviewed changes

benlesh closed this Nov 25, 2015

benlesh mentioned this pull request Nov 25, 2015

Rename fromCallback to factoryFromCallback #787

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(callback): Add Observable.fromCallback #729

feat(callback): Add Observable.fromCallback #729

SomeKittens commented Nov 15, 2015

SomeKittens commented Nov 15, 2015

SomeKittens commented Nov 15, 2015

kwonoj Nov 15, 2015

SomeKittens Nov 15, 2015

kwonoj Nov 15, 2015

SomeKittens commented Nov 15, 2015

kwonoj Nov 15, 2015

SomeKittens Nov 15, 2015

kwonoj commented Nov 15, 2015

SomeKittens commented Nov 18, 2015

kwonoj commented Nov 18, 2015

SomeKittens commented Nov 18, 2015

kwonoj commented Nov 18, 2015

benlesh commented Nov 18, 2015

benlesh commented Nov 18, 2015

SomeKittens commented Nov 19, 2015

staltz Nov 19, 2015

SomeKittens Nov 19, 2015

staltz commented Nov 19, 2015

SomeKittens commented Nov 19, 2015

SomeKittens commented Nov 19, 2015

staltz Nov 20, 2015

SomeKittens commented Nov 20, 2015

staltz commented Nov 20, 2015

SomeKittens commented Nov 20, 2015

benlesh Nov 20, 2015

benlesh commented Nov 20, 2015

benlesh commented Nov 20, 2015

SomeKittens commented Nov 20, 2015

SomeKittens commented Nov 25, 2015

benlesh commented Nov 25, 2015

benlesh commented Nov 25, 2015

SomeKittens commented Nov 28, 2015

feat(callback): Add Observable.fromCallback #729

feat(callback): Add Observable.fromCallback #729

Conversation

SomeKittens commented Nov 15, 2015

SomeKittens commented Nov 15, 2015

SomeKittens commented Nov 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SomeKittens commented Nov 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwonoj commented Nov 15, 2015

SomeKittens commented Nov 18, 2015

kwonoj commented Nov 18, 2015

SomeKittens commented Nov 18, 2015

kwonoj commented Nov 18, 2015

benlesh commented Nov 18, 2015

benlesh commented Nov 18, 2015

SomeKittens commented Nov 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staltz commented Nov 19, 2015

SomeKittens commented Nov 19, 2015

SomeKittens commented Nov 19, 2015

Choose a reason for hiding this comment

SomeKittens commented Nov 20, 2015

staltz commented Nov 20, 2015

SomeKittens commented Nov 20, 2015

Choose a reason for hiding this comment

benlesh commented Nov 20, 2015

benlesh commented Nov 20, 2015

SomeKittens commented Nov 20, 2015

SomeKittens commented Nov 25, 2015

benlesh commented Nov 25, 2015

benlesh commented Nov 25, 2015

SomeKittens commented Nov 28, 2015