@NathanWalker watch for #4893 as it will definitely impact this. the (annoyingly ugly) change will be that EventEmitter.subscribe() takes either .subscribe(nextFn, errorFn, completeFn) or .subscribe(observerObj) where observerObj is { next(), error(), complete() }

@robwormald thanks for heads up 👍

while you're at it, i just found a nasty little hiccup, maybe have a look as you're in the guts of this right now:

//create a subject
var s = new Subject();

//subscribe to it via the ObservableWrapper, passing only a next fn
ObservableWrapper.subscribe(s, (val) => {});

//call next
s.next(1);
//call complete (throws!)
s.complete();

This should solve your issue above as well @robwormald. Have a look and let me know.

/cc @vsavkin

As far as I can tell, async.ts does not have any unit tests. I could add some for this unless I'm missing something?

specs are here https://github.com/angular/angular/blob/master/modules/angular2/test/core/facade/async_spec.ts

I have one locally that looks like that throws - this replicates what I mentioned above.

it('should subscribe to Subjects', () => {
  let subject = new Subject();

  ObservableWrapper.subscribe(subject, (val) => {});

  subject.next(1);
  subject.complete();

})

Ah ok. Lemme try that on my branch here.

I have a fix coming (verified in isolation). Running locally all build/tests and will push the squashed commit shortly.

I used the same approach the RxJS team did here:
https://github.com/ReactiveX/RxJS/blob/master/src/Subscriber.ts#L38

Also the unit test now includes yours with an expansion to verify different signatures would still work.

I'm just now seeing your comment... lemme see...

otherwise tho, LGTM

Good point. I updated the unit tests so it didn't look confusing. Not sure if there's anything we should do to prevent .complete from firing if .error had been called? Probably best to rely on RxJS's underlying implementation for that? Thanks for the help and feedback 👍

Hmm, dart not loving that (the TS specs get xpiled to dart and executed as well) - might try calling using the wrapper there:

ObservableWrapper.callNext(e, 1);

etc

Thanks I'll try that shortly.

On Fri, Oct 30, 2015 at 12:17 PM Rob Wormald notifications@github.com
wrote:

Hmm, dart not loving that (the TS specs get xpiled to dart and executed as
well) - might try calling using the wrapper there:

ObservableWrapper.callNext(e, 1);

etc

—
Reply to this email directly or view it on GitHub
#4825 (comment).

@NathanWalker can you add "closes #4443" in your commit message?

Also, could you shorten the title to "fix(EventEmitter): resolve onError and onComplete asynchronously"

This should also be noted as a breaking change, so a BREAKING CHANGE: section should be added to the body, explaining the previous behavior and new behavior.

Meh, nm about breaking change.

Thanks @jeffbcross @robwormald that should clear things up 👍

I added unit tests for async onError and onComplete and have everything passing now.
Should be clear now @robwormald

@NathanWalker awesome. once #5302 lands, rebase and we should be good to go.

5302 came, it saw, and landed; then this just dropped the rebass! take it away @robwormald

LGTM. failing test is dartdoc known issue.

Merged via 019cb41

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}