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

Add close state to finally operator? #2823

Closed
benlesh opened this issue Sep 1, 2017 · 6 comments
Closed

Add close state to finally operator? #2823

benlesh opened this issue Sep 1, 2017 · 6 comments
Labels
feature PRs and issues for features

Comments

@benlesh
Copy link
Member

benlesh commented Sep 1, 2017

@matthewwithanm presented a scenerio here that I think we can address with a non-breaking change to the finally operator.

That is that we could provide, as an argument to finally's callback, a flag of some sort to declare whether or not it was finalized because of error, complete or unsubscribe.

Strawman:

source$.finally((type: string) => {
  switch(type) {
    case 'complete':
      doThing1();
      break;
    case 'error':
      doThing2();
      break;
    case 'unsubscribe':
      doThing3();
      break;
  }
})

The only additional thing I can think of is additionally providing the error in the event of an error, but I'm not sure how helpful that would be.

Thoughts?

@cartant
Copy link
Collaborator

cartant commented Sep 1, 2017

One observation is that it's like passing a materialized Notification - except that there's no unsubscribe Notification.

@martinsik
Copy link
Contributor

martinsik commented Nov 21, 2017

I'm thinking how could this work with #3122 (Using AbortSignal and AbortController). If the finally operator just adds another dispose function should I be able to handle abort with finally()? I guess there would have to be another type because it's not the same as unsubscribe if I understand it correctly.

@martinsik
Copy link
Contributor

martinsik commented Feb 7, 2019

I've stumbled upon exactly the same issue as described here where I wanted to know if the chain is being disposed because all observers unsubscribed or whether the chain completed.

I made a custom operator for this because it seems to be a pretty simple thing and like @benlesh said this would be a non-breaking change if finalize supported this.

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

import { defer, of, never, throwError, Observable, MonoTypeOperatorFunction } from 'rxjs'; 
import { tap, finalize } from 'rxjs/operators';

enum DisposeReason {
  Unsubscribe = 'unsubscribe',
  Complete = 'complete',
  Error = 'error',
}

type CallbackFunc = (reason: DisposeReason) => void;

const finalizeWithReason = <T>(callback: CallbackFunc): MonoTypeOperatorFunction<T> => 
  (source: Observable<T>) => 
    defer(() => {
      let completed = false;
      let errored = false;

      return source.pipe(
        tap({
          error: () => errored = true,
          complete: () => completed = true,
        }),
        finalize(() => {
          if (errored) {
            callback(DisposeReason.Error);
          } else if (completed) {
            callback(DisposeReason.Complete);
          } else {
            callback(DisposeReason.Unsubscribe);
          }
        }),
      );
    });

const finalizeCallback = (reason: DisposeReason) => console.log(reason);
const observer = {
  next: () => {},
  error: () => {},
  complete: () => {},
};

throwError(new Error()).pipe(
  finalizeWithReason(finalizeCallback),
).subscribe(observer);

of().pipe(
  finalizeWithReason(finalizeCallback),
).subscribe(observer);

never().pipe(
  finalizeWithReason(finalizeCallback),
).subscribe(observer).unsubscribe();

This prints the following output to console:

error
complete
unsubscribe

@matthewwithanm
Copy link

@martinsik We wound up with basically the same thing. I think you need to wrap your operator body in a defer() though. Otherwise you're using the same completed and errored variables for every subscription and once one errors they all will.

@martinsik
Copy link
Contributor

@matthewwithanm You're right, it should be wrapped inside defer.

@benlesh
Copy link
Member Author

benlesh commented Oct 2, 2019

This has gone pretty stale, and I don't think there's much appetite to add this at the moment. Closing for now.

@benlesh benlesh closed this as completed Oct 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature PRs and issues for features
Projects
None yet
Development

No branches or pull requests

4 participants