fix(skipUntil): fix skipUntil when innerSubscription is null

[claylaut]

Description: Fix a minor issue with skipUntil

Related issue:
#3685

[cartant]

Your fix looks appropriate, to me, but, ideally, the PR should also include a test that's based on code that reproduces the error.

The problem is effected because the implementation doesn't account for the synchronous completion of the notifier. You don't need to use a BehaviorSubject to effect the error; a simple, synchronous observable will do:

import { of } from "rxjs";
import { skipUntil } from "rxjs/operators";

of(1).pipe(
  skipUntil(of(2))
).subscribe(x => console.log(x));

[claylaut]

@cartant I've just simplified my usage example and opened an issue.
I've tested the fix by modifying the node modules and it worked fine.

I m unsure how to do the test if you can guide me I will appreciate it.

[cartant]

Sure.

Interestingly, I cannot get a marble test to effect the error, so I'd just add a test based on the repro. You could add one like this:

  it('should emit elements after a synchronous notifer emits', () => {
    const values: string[] = [];

    of('a', 'b').pipe(skipUntil(of('x'))).subscribe(
      value => values.push(value),
      err => { throw err; },
      () => expect(values).to.deep.equal(['a', 'b'])
    );
  });

I'd add it after the test named 'should emit elements after notifer emits' - near the top of the https://github.com/ReactiveX/rxjs/blob/master/spec/operators/skipUntil-spec.ts file.

Once you've added the test to the file, run npm run test to run the test suite. With your fix, it should pass. Then commit the test to your branch and forcibly push it to your repo with git push -f. That will see the PR updated.

[claylaut]

@cartant thanks a lot for your help.
I've updated the tests and they passed successfully.
PR Updated.

[martinsik]

This is just nitpicking but in all code around here there's always space between if and the starting parenthesis if (this.innerSubscription).

[claylaut]

@martinsik fixed

[rxjs-bot]

	Warnings
⚠️	commit message does not follows conventional change log (1)

(1) : RxJS uses conventional change log to generate changelog automatically. It seems some of commit messages are not following those, please check contributing guideline and update commit messages.

Generated by 🚫 dangerJS

[coveralls]

Coverage decreased (-0.2%) to 96.657% when pulling a36f718 on claylaut:feature/fix-skipUntil into dc66731 on ReactiveX:master.

[claylaut]

@cartant / @martinsik any progress on this? its quite important this for me because without it i cannot migrate to rxjs 6

[cartant]

@claylaut The repository's maintainers will eventually get around to reviewing and, likely, merging your PR. You will just need to be a little patient, as they're busy people with other priorities. In the interim, you could use a scheduler for a temporary workaround:

import { asapScheduler, of } from "rxjs";
import { skipUntil } from "rxjs/operators";

of(1).pipe(
  skipUntil(of(2, asapScheduler)) // or skipUntil(of(2).pipe(subscribeOn(asapScheduler)))
).subscribe(x => console.log(x));

[benlesh]

Thanks, @claylaut!

[claylaut]

@benlesh thanks for merge.

@cartant thanks for the workaround 👍