lift called with incorrect context

[cartant]

RxJS version: 5.5.10 and 6.0.0-uncanny-rc.7

Code to reproduce:

As in #3574, a lifted Subject does not behave as you'd expect:

const subject = new Subject();
const lifted = subject.pipe(
    combineLatest(of(2))
);
lifted.subscribe(value => console.log(value));
lifted.next(1);

And a lifted custom observable can explode:

class SomeObservable extends Observable<any> {
    constructor(public path: string) {
        super();
        if (typeof path !== "string") {
            throw new Error("Path is not a string.");
        }
    }
    lift<R>(operator: Operator<any, R>): Observable<R> {
        const observable = new SomeObservable(this.path);
        observable.operator = operator;
        observable.source = this;
        return observable;
    }
}

const some = new SomeObservable("/one");
some.pipe(
    combineLatest(of(2))
);
some.subscribe(value => console.log(value));

Expected behavior:

The first repro should print [1, 2] to the console.

The second repro should not explode.

Actual behavior:

The first repro prints nothing to the console.

The second repro explodes with an error.

Additional information:

The problem is down to lift being called with an incorrect context - that is, with something other than the source observable as this:

$ git grep 'lift.call(' -- '*.ts'
...
src/internal/operators/combineLatest.ts:  return (source: Observable<T>) => source.lift.call(from([source, ...observables]), new CombineLatestOperator(project));
src/internal/operators/concat.ts:  return (source: Observable<T>) => source.lift.call(concatStatic<T, R>(source, ...observables));
src/internal/operators/merge.ts:  return (source: Observable<T>) => source.lift.call(mergeStatic(source, ...observables));
src/internal/operators/race.ts:    return source.lift.call(raceStatic<T>(source, ...observables));
src/internal/operators/zip.ts:    return source.lift.call(zipStatic<R>(source, ...observables));

To understand what's happening with the first repro, if you look at the implementation of Subject.lift, you'll see that an AnonymousSubject is returned. Note that this is passed to the AnonymousSubject constructor as the destination.

The implementation of AnonymousSubject.next calls destination.next if it's defined. Otherwise, it does nothing.

The problem is effected because the implementation of combineLatest doesn't pass the source - the Subject - as the context for the lift call; it passes another observable and that observable has no next method, so nothing happens when next is called.

The second is simpler. The observable used as the this context has no path property, so the constructor explodes with an error, as path is not a string.

The first repro is pretty much an edge case, but the second is - IMO - more of a potential problem, as it means that some operators cannot be used with custom observables unless said custom observables either don't implement lift or implement it in such a way that it's indifferent to the this context.

Interestingly, all of the operators that exhibit the problem were - IIRC - candidates for being removed in favour of their namesake observable factory functions.

[benlesh]

FYI: I'm working on this issue with another contributor.

[cartant]

Thanks. I wasn't working on it; I just thought I'd flag it with a detailed issue. Probably should have mentioned that.

[mlegenhausen]

Whats the progress on this issue? The bug is still there. Is there any workaround?

[cartant]

The bug is still there.

Yes.

Is it affecting you? It's not the a bug that most devs should run into.

The solution to the second point - a lifted custom observable can explode - is trivial, IMO. All that's necessary is for source to be passed as the context. If that's affecting you, you could make the change and open a PR.

[benlesh]

Looked at this today. I truly can't wait to get rid of lift, but this seems to be related to any place we're calling, what I've internally dubbed the "stankyLift". I haven't looked into why we're still using this technique, (the function linked was just a straight port of a behavior to avoid breaking anything) but I do think there are solutions to this problem.

[benlesh]

This was fixed as of 7.0.

https://stackblitz.com/edit/rxjs-lunrjz?file=index.ts