johannesjo / angular2-promise-buttons

Chilled loading buttons for angular2
https://johannesjo.github.io/angular2-promise-buttons/#demo
MIT License
86 stars 28 forks source link

Observable fires twice #15

Closed dottodot closed 7 years ago

dottodot commented 7 years ago

I'm probably doing something wrong but when I hook my button up to a http request the request is fired twice.

So normally I'd make my http request like

this.service.create(data).subscribe();

and for the button I've tried

this.observableBtn = this.service.create(data);
this.observableBtn.subscribe();
johannesjo commented 7 years ago

Thanks for reporting! I'll look into it tomorrow!

johannesjo commented 7 years ago

Could you by any chance provide a plunkr? As far as I can see, everything works as expected.

rajdeep26 commented 7 years ago

I am also facing this issue. I have a button with click and promiseBtn and whenevrer I click on the button 2 calls are made to the function which is bound to click

rajdeep26 commented 7 years ago

@johannesjo Check this example => https://stackblitz.com/edit/angular-ewhtev?embed=1&file=app/app.component.ts

ghost commented 7 years ago

@rajdeep26 I forked your example and I'm facing 2 calls even without the promiseBtn module. Screenshot => https://i.imgur.com/CSm0UBN.png Sandbox => https://stackblitz.com/edit/angular-ruecva

The second request is made by ServiceWorker. Also, I subscribed to the observable and got only 1 result:

  someAction() {
    this.observable = this.callAPI();
    this.observable.subscribe((data) => {
      console.log('got data', data); // fired only once
    })
  }

Sandbox => https://stackblitz.com/edit/angular-zgfb9o

so there's no problem actually

rajdeep26 commented 7 years ago

I think I was not able to reproduce it properly. The network tab shows 1 call made by serviceworker and 1 by the app. But in my app, no call is made by service worker, but still i can see that 2 calls are being made. See the screenshot below.

image

Please check if you'll can reproduce the issue. Later i'll also give one more try and see if i can reproduce it in the example I had shared.

johannesjo commented 7 years ago

Thanks everybody for looking into it. Can't say that I dived too deeply into observables yet, but I hope I can help a little. angular2-promise-buttons uses the toPromise of rxjs on observables, which in turn just triggers another subscribe to the observable. Not sure why this triggers a second call, but it's starting point.

Some more information about this: https://github.com/Reactive-Extensions/RxJS/issues/294

I'll try digging into an alternative approach/work around, but I'm glad for any help!

rajdeep26 commented 7 years ago

Any update on this?

ghost commented 7 years ago

@rajdeep26 the demo works perfectly fine and your sandbox works fine. The plugin does not initialize a call. It converts Observable to Promise and uses .then and .catch methods to add a spinner. You should look for a problem in your code. Try to make a demo that demonstrates the bug.

rajdeep26 commented 7 years ago

@maxkorz I have just modified the demo, and now it indeed makes 2 calls. Just check this => https://stackblitz.com/edit/angular-ewhtev?embed=1&file=app/app.component.ts

The only new thing I have added is the subscribe on the observable in app.component.ts. If you remove the promiseBtn then you will be able to see only 1 call.

ghost commented 7 years ago

@rajdeep26 ok I see the bug. I don't know what causes it yet, but I'll try to fix it. By the way, In your demo I see that the subscribe function fires only once, so this bug doesn't affect the code behavior.

enzolry commented 7 years ago

@johannesjo @maxkorz Hi, same problem here. When i add a console.log in the subscribe function it logs 1 time in the console but i receive two calls on the server side.

Observable from service:

createAccount(userId, realId, platform){
    let headers = new Headers();
    headers.append('Content-Type', 'application/json');
    return this.http.post(this.apiUrl+'/game_accounts?userId='+userId+'&realId='+realId+'&platform='+platform, {headers: headers})
      .map(res => res.json());
  }

Call in component:

this.observable = this.accountService.createAccount(this.userId, this.steam, "steam");
    this.observable.subscribe(data => {
      console.log('Data is here');
      if(data.success){
        this.accountService.pushAccount(data.game_account);
        this.router.navigate(['/user', this.userId]);
      } else {
        this.flashMessage.show(data.msg, {cssClass: 'card-panel red lighten-2 white-text', timout: 4000});
        this.router.navigate(['/account/new']);
      }
    });
  }

In my Rails 5 server console:

Started POST "/api/v1/game_accounts?userId=7&realId=76561198118594288&platform=steam" for 127.0.0.1 at 2017-10-24 12:13:05 +0200
Started POST "/api/v1/game_accounts?userId=7&realId=76561198118594288&platform=steam" for 127.0.0.1 at 2017-10-24 12:13:05 +0200
Processing by GameAccountsController#create as HTML
Processing by GameAccountsController#create as HTML
  Parameters: {"headers"=>{"Content-Type"=>["application/json"]}, "userId"=>"7", "realId"=>"76561198118594288", "platform"=>"steam", "game_account"=>{}}
  Parameters: {"headers"=>{"Content-Type"=>["application/json"]}, "userId"=>"7", "realId"=>"76561198118594288", "platform"=>"steam", "game_account"=>{}}

The package is great so I hope you gonna be able to fix this.

ymatus commented 7 years ago

I've also stumbled upon this issue.

So far I came up with a following temporary solution: switching .subscribe to .map, which allows us to use .toPromise.

In your api.service.ts:

add(model: any) {
    return this.http.post('api/v1/controller', model);
}

In your component.ts:

import 'rxjs/add/operator/map';
save(model: any) {
    this.promise = this.apiService.add(model)
        .map(data => console.log(data))
        .toPromise();
}

In your component.html:

<button type="submit" [promiseBtn]="promise">Save</button>

Sandbox => https://stackblitz.com/edit/angular-tmnqzf

Hopefully, you gonna come up with a better solution.

johannesjo commented 7 years ago

@ymatus thank you for the tmp fix. If this works we probably can use the same fix for promise buttons itself here: https://github.com/johannesjo/angular2-promise-buttons/blob/3e7e647902f41c77d751474827085229c2de34b0/src/promise-btn.directive.ts#L46

Somebody wants to check? :)

enzolry commented 7 years ago

@ymatus Thanks for the fix, it works well :+1:

@johannesjo I tried to add map() operator before .toPromise() directly in promise-btn.directive.ts to see if it helps. I don't know if it's really what you suggested, and I'm not really familiar with rxjs so I probably do stupid things. But it didn't work for me since I need to put the logic to handle the response directly in the map() operator in the component.

ghost commented 7 years ago

The problem is that subscribe function initializes the request. The toPromise operator uses subscribe internally, that's why the second request is being made. I'm still looking for a way to fix this.

ghost commented 7 years ago

Apparently it's not possible to do what we want with Observable because of this:

Cold observables start running upon subscription, i.e., the observable sequence only starts pushing values to the observers when Subscribe is called. Values are also not shared among subscribers. When an observer subscribes to a hot observable sequence, it will get all values in the stream that are emitted after it subscribes. The hot observable sequence is shared among all subscribers, and each subscriber is pushed the next value in the sequence.

All observables are cold by default and it's not possible to access any existing subscriptions or any received data without making a new subscription (which makes the http request). To convert the cold observable sequence source to a hot one we need to use the publish operator before any subscription, which is not possible from within the plugin.

Instead, I suggest to pass the Subscription to the plugin, like this:

const observable = new Observable(observer => {
  setTimeout(() => {
    observer.complete();
  }, 10000);
});

this.successObservable = observable.subscribe(
  () => {},
  () => {},
  () => {}
);
  <button class="btn btn-raised"
          (click)="initSuccessObservable()"
          [promiseBtn]="successObservable">Success after delay
  </button>
johannesjo commented 7 years ago

@maxkorz thank you very much for the PR! Looks good to me at first glance. I'll have a closer look tomorrow.

johannesjo commented 7 years ago

@maxkorz Thanks again. Excellent work! I added some documentation and another unit test too.

enzolry commented 7 years ago

@johannesjo I followed the steps you added to the documentation. It does prevent the observable from firing twice but it doesn't disable the button anymore.

So I keep using @ymatus fix for now.

ghost commented 7 years ago

@EnzoArenaEsport I believe @johannesjo didn't release the new version yet

johannesjo commented 7 years ago

I will release one tomorrow. For now you should be able to directly point to the master branch.

johannesjo commented 7 years ago

@EnzoArenaEsport I published the new version 1.1.0.

enzolry commented 7 years ago

@johannesjo My bad I was too impatient ^^' I updated the package and everything works fine now. Good job !