Extending Observable

[trxcllnt]

I think we're mature enough to start looking at this closer. As requested before (#1124, #1207, #1353, #1757), extending Observable should be seamless. Most of the time extending Observable and overriding lift does the job, but there are a few cases this breaks down.

If this sounds reasonable, I'll start work on a PR in the next few days.

Operators that call static methods

These are combineLatest, concat, merge, race, and zip. Implementing these with lift might be tedious, but doable.

Multicast/ConnectableObservable, and operators that emit inner Observables

1. We should implement multicast with selector as an operator instead of a custom Observable subclass.
2. ConnectableObservable, window, groupBy, and eventually join/groupJoin, should all use the prototype from the current this binding, and we can mixin their respective prototypes with Object.create. Fortunately, the ConnectableObservable implementation is already written with Operators and Subscribers, so aside from the public multicast method, this shouldn't be a lot of rework:

class Observable {
  multicast(subjectFactory) {
    const connectable = Object.create(this, connectableObservableDescriptor);
    connectable.subjectFactory = subjectFactory;
    return connectable;
  }
}
const connectableObservableDescriptor = {
  connect: { value: function () { /* ... */} },
  refCount: { value: function () { /* ... */ } },
  _subscribe: { value: function _subscribe(subscriber) {
      return this.getSubject().subscribe(subscriber);
    }
  },
  getSubject: { value: function getSubject() {
      const subject = this._subject;
      if (!subject || subject.isStopped) {
        this._subject = this.subjectFactory();
      }
      return this._subject;
    }
  }
}

[jayphelps]

regarding the static methods..are you referring to this kind of stuff:

return new ArrayObservable(observables, scheduler).lift(new CombineLatestOperator<T, R>(project));

If so, couldn't this be solved by deferring to the lift from the current prototype instead?

return this.lift.call(new ArrayObservable(observables, scheduler), new CombineLatestOperator<T, R>(project));

[dead-claudia]

@jayphelps That looks useful if you define Observable.prototype.lift to default to this:

lift(o, f) {
  return o.lift(f)
}

But there's a catch: how do you link it with the parent observable, so it can be updated when the parent updates? You still need some sort of API that lets you push data into it, because otherwise, things like .map won't work otherwise.

[jayphelps]

@isiahmeadows perhaps there's some misunderstanding. I'm not recommending changing the lift method signature.

I was saying that static methods could use the lift method from the current prototype instead of using the one from the ArrayObservable because the current prototype lift might be one that was custom.

It might not have been obvious that I was using fn.call(context, ...args)...so this.lift.call(context, operator) or:

this.lift.call(new ArrayObservable(observables, scheduler), new CombineLatestOperator<T, R>(project));

This is Function.protototype.call not the RxJS internal call() method on operator classes. 😉

Or perhaps I'm misunderstanding your comment 😆

[benlesh]

Operators that call static methods

These are combineLatest, concat, merge, race, and zip. Implementing these with lift might be tedious, but doable.

I think it's important to move these to use lift... specifically because it will enable us to do more with tooling around RxJS.

[jayphelps]

Regarding this.lift.call() that I proposed, that obviously wouldn't solve the static operator issue, i.e. Observable.merge, etc where this is no instance prototype to look up this.lift()

What about making lift a static method, so static operators can access it and instance operators can access it as well via this.constructor.lift()?

class Observable {
  static lift() {
    ...
  }

  map(...) {
    this.constructor.lift(...);
  }
}

[benlesh]

@jayphelps ... that's an interesting idea... if we took the current incarnation of lift and changed it to be static, it might look like...

static lift<R>(operator: Operator<T, R>, source: Observable<T>): Observable<R> {
    const observable = new this();
    observable.source = source;
    observable.operator = operator;
    return observable;
 }

lift<R>(operator: Operator<T, R>): Observable<R> {
  return this.constructor.lift(operator, this);
}

This would be a psuedo-code version of combineLatest:

static combineLatest(observables, project) {
  return this.lift(new CombineLatestOperator(project), new ArrayObservable(observables));
}

I'm not sure what that buys us over what we're currently doing.

My spider-sense is tingling about it. I feel like moving to a static lift might be coding ourselves into a corner somehow. But I could be wrong. It just seems odd.

[jayphelps]

@Blesh the example you showed seems legit. I'm confused though about it not seemingly buying us anything? It buys us all that lift provides us..so custom wrapping Observable with our own operators, adding debugging/logging, etc.

[benlesh]

@jayphelps sorry, I needed to be more specific... I'm trying to think of the use case where having lift be static is more useful than what we're currently doing.

In both cases, we're able to get the source and the operator and hook them (if we need too) for debugging, etc.

I'm mildly concerned about static lift in the wild. Using it directly seems like an anti-pattern. But hooking it seems like something useful for tooling, etc. ... I'm just leery of the change I think.

[jayphelps]

@Blesh hehe that didn't clear it up for me. How can you easily hook into the existing static methods and return your own observable wrapper? You would have to override every static method individually, wouldn't you?

[benlesh]

How can you easily hook into the existing static methods and return your own observable wrapper? You would have to override every static method individually, wouldn't you?

Yes, I see what you're saying.

Okay, okay, I'm warming up to it.

[trxcllnt]

@jayphelps I've got this ready on my local machine, would you like it broken up into single/multiple commits, and single/multiple PRs, or some permutation of the two?

[jayphelps]

@trxcllnt without knowing what it all entails, hard to say. Throw it all into a single PR with how ever many commits you think it should have and if anyone thinks it's a problem we'll discuss there after seeing it.

If you think there some controversial elements not discussed here yet, those might be good candidates for separate commits so you can more easily tease them out if we wait on those but merge the rest.

[trxcllnt]

@jayphelps I've only touched the prototype methods (combineLatest, concat, merge, multicast, race, and zip + tests) that weren't using lift yet, and each change is relatively minor. I'm worried multiple commits would just be noisy, but I'm happy to do it if you don't think so.

[benlesh]

@trxcllnt is this work complete now? I haven't been keeping track. Either way, I'm closing this issue due to inactivity. Feel free to reopen if necessary.

[larssn]

I'd like to leave my 2c.

This is a feature we highly desire at my company. We're working various Observable wrappers, such as AngularFire2, and its a bit of nightmare to code around this limitation; as we might have a FirebaseListObservable, which is a standard Observable with a few extra functions on it.

We can't, however, use any operators on it, because then it gets transformed into a regular Observable, and the extra functions are lost.

So I hope you haven't given up on this! 😄

(and I hope I'm not misunderstanding what this topic is about 😅 )

[trxcllnt]

@larssn certainly not! Since the beginning I've advocated flowing Observable types through operators, perhaps more than any other issue. Along with perf, it was one of the driving forces behind implementing operators in terms of lift.

I may be the only person in the community who relies on it, but it's a really useful feature for hooking in and building custom async DSLs on top of Observable. My public rxjs-fs, rxjs-mapd, rxjs-gestures, and @graphistry/client-api projects are all built this way, as well as a number of internal APIs.

From my understanding there are just two minor issues we haven't addressed:

1. Calling the static Observable methods on an Observable subclass won't return an Observable of the subtype, since they're all implemented as Observable subclasses themselves. In practice we can get around this by wrapping the statics in functions that do return the subtype, but it's not as ergonomic as I'd like.
2. Even though subclassing works at runtime, TypeScript doesn't know how to flow the subtype through Rx operator calls, leading to type errors if you rely on subclassing in TS.

[dead-claudia]

@trxcllnt

Even though subclassing works at runtime, TypeScript doesn't know how to flow the subtype through Rx operator calls, leading to type errors if you rely on subclassing in TS.

Based on reading that, you would probably need a couple specific things:

• A way in TS to require and use a parametric constructor in subclasses.
• Modifying this to use this.constructor rather than Observable directly.

[trxcllnt]

@isiahmeadows ah, I should have been more clear. TypeScript could infer the subtype through Rx operator calls, but we'd have to remove the explicit return type declarations from all the operators (as well as lift). We could start that discussion, but I'm not sure @benlesh would go for it:

Type error (explicit types):

No type error (inferred types):

To your second point, we can't use the current this.constructor to build the out Observable for two reasons. Subclasses of Observable have their own constructor implementations that we don't want to invoke when an operator is called (e.g. operators called on fromArray() would invoke the ArrayObservable constructor each time). Additionally, we've implemented the instance versions of some static methods in terms of the static methods, so we do some funny this re-binding for convenience.

[raysuelzer]

If the explicit type is not actually the type that is being returned, then it seems like the correct approach would be to let typescript infer the the type. The current situation makes it impossible to strongly type subjects and then use any operators that are inherited from Observable.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.