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

Split multicast, publish, et al into static functions and operators? #3833

Closed
benlesh opened this issue Jun 12, 2018 · 4 comments
Closed

Split multicast, publish, et al into static functions and operators? #3833

benlesh opened this issue Jun 12, 2018 · 4 comments

Comments

@benlesh
Copy link
Member

benlesh commented Jun 12, 2018

Given that composition of ConnectableObservable isn't something TypeScript is capable of, and given that ConnectableObservable is just a little different, it's probably worth discussing having the functions that return ConnectableObservable be static, and their projected counterparts stay as operators.

This is likely something we could not do until the next major version.

Problems

  • ConnectableObservable doesn't compose through pipe very well in typed languages.
  • It's not readily obvious that you're doing something different with these operators, and people don't know you need to connect them at times.
  • Two very different results between publish and publish(fn), for example.

Static Functions

function multicast<T>(source: Observable<T>, subject: Subject<T>): ConnectableObservable<T>;
function multicast<T>(source: Observable<T>, subjectFactory: () => Subject<T>): ConnectableObservable<T>;

function publish<T>(source: Observable<T>): ConnectableObservable<T>;
function publishLast<T>(source: Observable<T>): ConnectableObservable<T>;
function publishBehavior<T>(source: Observable<T>, initialValue: T): ConnectableObservable<T>;
function publishReplay<T>(source: Observable<T>, bufferSize?: number, bufferTime?: number): ConnectableObservable<T>;

Operator Functions

(bike shed names later)

function multicastWith<T, R>(subject: Subject<T>, project: (source: Observable<T>) => Observable<R>): OperatorFunction<T, R>;
function multicastWith<T, R>(subjectFactory: () => Subject<T>, project: (source: Observable<T>) => Observable<R>): OperatorFunction<T, R>;

function publishWith<T, R>(project: (source: Observable<T>) => Observable<R>): OperatorFunction<T, R>;
function publishLastWith<T, R>(project: (source: Observable<T>) => Observable<R>): OperatorFunction<T, R>;
function publishBehaviorWith<T, R>(initialValue: T, project: (source: Observable<T>) => Observable<R>): OperatorFunction<T, R>;
function publishReplayWith<T, R>(bufferSize: number, bufferTime: number, project: (source: Observable<T>) => Observable<R>): OperatorFunction<T, R>;

Risks

  • Larger API surface area
  • We'll need to support refactoring for these cases in the migration process for the next version

Benefits

  • Easier to understand than the current variadic operators, who behave very differently given different arguments.
  • Easier to read (subjective, I suppose)

Other things

I don't think all variations shown above actually exist. We can probably get away with just the multicastWith and publishWith variants of the operators.

@benlesh benlesh added type: discussion AGENDA ITEM Flagged for discussion at core team meetings labels Jun 12, 2018
@benlesh
Copy link
Member Author

benlesh commented Jun 12, 2018

@ReactiveX/rxjs-core

@cartant
Copy link
Collaborator

cartant commented Jun 13, 2018

Would this also involve deprecating the refCount operator? It's only applicable to a ConnnectableObservable source, so refCount should just be called as a method on the ConnnectableObservable, right?

If refCount is deprecated and there are only multicastWith and publishWith operators, the increase in API surface area is pretty minimal. And things would definitely be clearer - regarding the behaviour when a project function is specified.

@jgbpercy
Copy link

Hah, well I just spend a good evening (no really, it was pretty educational) getting as far as drafting pretty much this exact issue, after banging my head against multicast weirdness today.

This probably isn't a part of the API that many people use that often (I'm guess from experience, and the amount of activity on this issue), but when you do it is fairly gnarly and confusing. I can see there are some draft PRs, which I'll have a glance at soon - but it doesn't look like the plan is to get these in for v7? In the absence of that, there are a couple of things that could hopefully be fixed without larger/breaking changes:

  • The example here is not valid TS (although the JS works fine). multicasted is just Observable<unknown>, so the last line needs to be (multicasted as any).connect() or (multicasted as ConnectableObservable<number>).connect() for TS to be happy.
  • For multicast, I believe the second overload export function multicast<T, O extends ObservableInput<any>>(subject: Subject<T>, selector: (shared: Observable<T>) => O): UnaryFunction<Observable<T>, ConnectableObservable<ObservedValueOf<O>>>; is wrong. This is a variant with a selector, so the return should just be OperatorFunction<T, ObservedValueOf<O>>, like the 4th overload. Although I believe the types all end up looking the same inside pipe, it's still probably useful for both of the variants that act like "normal" operators (the ones that would become multicastWith) to have correct types.
  • The documentation for multicast and publish could be made to be much more clear about the behaviour difference between passing a selector/project fn and not.

Assuming the proposed change wont be in v7 I'm happy to make a PR for the above?

@cartant
Copy link
Collaborator

cartant commented Mar 13, 2021

Closed by #5634 AFAICT.

@cartant cartant closed this as completed Mar 13, 2021
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