Reactive-Extensions / RxJS

The Reactive Extensions for JavaScript
http://reactivex.io
Other
19.49k stars 2.1k forks source link

finally bloc feature for subscribe function ? #1434

Closed chaouiy closed 7 years ago

chaouiy commented 7 years ago

It would be greate if we can have an option to define finally function to execute whatever returned by the observer. eg: stoploadingbutton. we need to stop loading a button for success and for error responses and it's redundunt to write this twice.

paulpdaniels commented 7 years ago

Could you elaborate on the type of behavior you are looking for?

function to execute whatever returned by the observer

I don't understand what you expect to be returned by the Observer

chaouiy commented 7 years ago

Consider the following : this.toggleLoading()is redundant since it's called for both success and error. Imagin 10 instructions instead of just this.toggleLoading()

login() {
        this.toggleLoading()
        this.seesionService.login(this.username, this.password).subscribe(
            (data) => {
                this.getUser(this.username)
                this.toggleLoading()
            }, (error) => {
                this.errorHandlerService.display(error)
                this.toggleLoading()
            }
        );
    }
paulpdaniels commented 7 years ago

I think you want .finally() then.

login() {
        return Observable.defer(() => {
          this.toggleLoading();
          return this.sessionService.login(this.username, this.password);
        })
          .finally(() => this.toggleLoading())
          .subscribe(
            (data) => {
                this.getUser(this.username)
            }, (error) => {
                this.errorHandlerService.display(error)
            }
        );
    }
chaouiy commented 7 years ago

why is it separated from subscibe function. a possibility for adding third arrow function to subscribe() makes code cleaner

paulpdaniels commented 7 years ago

The third block in a subscribe call is the completed. If you did it that way you would still need to have this.toggleLoading() in two places, once in the error block and once in the completed block. finally gets executed regardless of the terminal case.

gioragutt commented 7 years ago

@paulpdaniels thanks for the replies. Can I ask though, why do you use Observable.defer? I see that you provide a function that returns an Observable, but without calling it(aka (() => { })()) you .finally the method. The defer is closed in the end of the method though. Could you explain what goes on in the method after you've refactored it? Thanks!

paulpdaniels commented 7 years ago

Whoops @gioragutt thats a typo, I just fixed it. The defer is just so that the toggleLoading call gets set by the stream being subscribed to and not by the method being called. Simply calling login shouldn't toggle the loading on, only when you actually initiate the action by subscribing to the Observable does the loading actually start. This ties the toggling directly to the lifecycle of the stream which makes it more portable.

gioragutt commented 7 years ago

I get it. So basically, defer's use case is tying some action(s) to an Observable (login in our example)? That's pretty cool. Guess my reviewing skills truly shine when looking for stuff like that 😅 thanks for the answer dude!

gioragutt commented 7 years ago

Also, @amineparis, having finally as an operator makes the code a lot more readable and clean, than having a third function parameter to subscribe, aside from the fact that it's already used, in case you wonder why would you not put it as another parameter to subscribe.