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

Stop composing custom observable instances in version 8 #5431

Open
benlesh opened this issue May 14, 2020 · 3 comments
Open

Stop composing custom observable instances in version 8 #5431

benlesh opened this issue May 14, 2020 · 3 comments
Assignees

Comments

@benlesh
Copy link
Member

benlesh commented May 14, 2020

We are currently jumping through a lot of hoops in order to preserve the type through the operator chain, when that is no longer as relevant. As a hold-over from the dot-chained days, there was an attempt, starting in v5, to maintain the same type through operators by leveraging lift. This was done such that this would still work:

class CustomObservable<T> extends Observable<T> {
  specialMethod(): void { }
}

const custom = new CustomObservable<T>;

custom.filter(fn).map(fn2).mergeMap(fn3).specialMethod()

Over the years this has proven to have limited value, and in fact, is flawed in some ways. For example, look at the code we've had for years in order to support "lifting" publish operators in such a way that connect composes through.

Fact is, the current implementation doesn't support what it claims, because if you have more than one publish operator in the chain, it breaks connect. Try this:

This doesn't work:

interval(100).pipe(
  publish(),
  publish(),
  refCount()
).subscribe(x => console.log(x));

And this doesn't work:

const source = interval(100).pipe(publish(), publish());
source.subscribe(x => console.log(x));
source.connect();

This the byproduct of a few things, but chief among them are:

  1. publish() et al, should not be "pipeable operators".
  2. Composing custom types (or even Subject) through the operators via lift was a cool experiment, but ultimately was of limited value and required some strange workarounds in all cases.
  3. Implementing lift on a subclass of Observable requires the developer to have knowledge of Observable's implementation details. (You have to set subclass.operator and subclass.source etc).

For version 8, I'd like to eliminate the composition of types like ConnectableObservable, Subject and "custom observables" through operators as a goal.

@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label May 14, 2020
@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label Jun 3, 2020
@benlesh
Copy link
Member Author

benlesh commented Jun 3, 2020

Core Team Meeting Notes: Do it.

@fan-tom
Copy link

fan-tom commented Jun 5, 2020

Yeah, recently I was managed to map BehaviorSubject to BehaviorSubject and it required some boilerplate, because if you just use pipe, you get Observable back, not BehaviorSubject

benlesh added a commit to benlesh/rxjs that referenced this issue Jul 4, 2020
- Removes `_isScalar` as it was unused
- makes `lift` `protected`. This is an internal implementation detail.
- makes `source` `protected`, this is an internal implementation detail.
- Refactors operators to use new utility functions that do the lift or provide a reasonable error if the observable someone is trying to use with the operator does not have a lift method. Adds documentation.

BREAKING CHANGE: `lift` no longer exposed. It was _NEVER_ documented that end users of the library should be creating operators using `lift`. Lift has a [variety of issues](ReactiveX#5431) and was always an internal implementation detail of rxjs that might have been used by a few power users in the early days when it had the most value. The value of `lift`, originally, was that subclassed `Observable`s would compose through all operators that implemented lift. The reality is that feature is not widely known, used, or supported, and it was never documented as it was very experimental when it was first added. Until the end of v7, `lift` will remain on Observable. Standard JavaScript users will notice no difference. However, TypeScript users might see complaints about `lift` not being a member of observable. To workaround this issue there are two things you can do: 1. Rewrite your operators as [outlined in the documentation](https://rxjs.dev/guide/operators), such that they return `new Observable`. or 2. cast your observable as `any` and access `lift` that way. Method 1 is recommended if you do not want things to break when we move to version 8.
benlesh added a commit that referenced this issue Jul 5, 2020
- Removes `_isScalar` as it was unused
- makes `lift` `protected`. This is an internal implementation detail.
- makes `source` `protected`, this is an internal implementation detail.
- Refactors operators to use new utility functions that do the lift or provide a reasonable error if the observable someone is trying to use with the operator does not have a lift method. Adds documentation.

BREAKING CHANGE: `lift` no longer exposed. It was _NEVER_ documented that end users of the library should be creating operators using `lift`. Lift has a [variety of issues](#5431) and was always an internal implementation detail of rxjs that might have been used by a few power users in the early days when it had the most value. The value of `lift`, originally, was that subclassed `Observable`s would compose through all operators that implemented lift. The reality is that feature is not widely known, used, or supported, and it was never documented as it was very experimental when it was first added. Until the end of v7, `lift` will remain on Observable. Standard JavaScript users will notice no difference. However, TypeScript users might see complaints about `lift` not being a member of observable. To workaround this issue there are two things you can do: 1. Rewrite your operators as [outlined in the documentation](https://rxjs.dev/guide/operators), such that they return `new Observable`. or 2. cast your observable as `any` and access `lift` that way. Method 1 is recommended if you do not want things to break when we move to version 8.
@benlesh benlesh self-assigned this Aug 13, 2020
@voliva
Copy link
Contributor

voliva commented Mar 16, 2021

I've been probably late on the lift train, but I still don't fully understand what was lift for.

From OP it looks like it was added to keep methods from the prototype chain when applying operators, but as of now, this code (using just JS):

class CustomObservable extends Observable {
    specialMethod() { }
}
const custom = new CustomObservable();
console.log(
  map(x => x)(custom).specialMethod
);

logs undefined, which means that even though map is using lift internally, it doesn't seem like it's keeping the original functions from CustomObservable 🤔

Edit: Asked in #6148

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants