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.

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(buffer): cleanup notifier subscription when unsubscribed #537

Closed
wants to merge 1 commit into from

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Oct 15, 2015

Code snippet

var source = Rx.Observable.timer(0, 25).buffer(Rx.Observable.interval(50)).take(3);
var subscription = source.subscribe(
  function (x) {
    console.log('Next: %s', x);
  },
  function (err) {
    console.log('Error: %s', err);
  },
  function () {
    console.log('Completed');
  });

emits buffered elements like

Next: 0,1
Next: 2,3
Next: 4,5
Completed

--> But process does not exit here

This PR let buffer unsubscribes closingNotifier to avoid above issue.

@staltz
Copy link
Member

staltz commented Oct 15, 2015

Good catch. Should we also add a test for that?

@kwonoj
Copy link
Member Author

kwonoj commented Oct 15, 2015

Would be better to have, bit unsure how to design test case for this. @staltz any suggestions?

@benlesh
Copy link
Member

benlesh commented Oct 16, 2015

Looks like we need to add tests for the work? Coveralls is complaining on this one, @kwonoj

@kwonoj
Copy link
Member Author

kwonoj commented Oct 16, 2015

Yes I agree, as asked above bit unsure how to design test case for this. May I borrow some tips from you for insight?

@staltz
Copy link
Member

staltz commented Oct 16, 2015

@kwonoj here's a draft:

  it('should', function () {
    var a = hot('--1--2--^--3--4--5---6----7--8--9---0---|');
    var unsub =         '              !                  ';
    var subs =          '^             !                  ';
    var b = hot('--------^--a-------b---cd|               ');
    var bsubs =         '^             !                  '; // <-----------------
    var expected =      '---a-------b---                  ';
    var expectedValues = {
      a: ['3'],
      b: ['4', '5']
    };
    expectObservable(a.buffer(b), unsub).toBe(expected, expectedValues);
    expectSubscriptions(a.subscriptions).toBe(subs);
    expectSubscriptions(b.subscriptions).toBe(bsubs); // <------------------
  });

@kwonoj
Copy link
Member Author

kwonoj commented Oct 16, 2015

@staltz Appreciate for help! I'll try to add those test cases.

@kwonoj
Copy link
Member Author

kwonoj commented Oct 16, 2015

Updated PR by copy-paste of @staltz 's help 😁

@staltz
Copy link
Member

staltz commented Oct 17, 2015

Looks good. I wonder why Coveralls is reporting less coverage (by -0.01%, negligible in my opinion).

@kwonoj
Copy link
Member Author

kwonoj commented Oct 17, 2015

why Coveralls is reporting less coverage

@staltz This was reason I started looking into #557, for this specific it seems because current coverage try to check *spec.js also.

@benlesh
Copy link
Member

benlesh commented Oct 26, 2015

merged with 1b30aa9 ... thanks @kwonoj!

@benlesh benlesh closed this Oct 26, 2015
@kwonoj kwonoj deleted the fix-buffer branch October 26, 2015 20:57
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants