3

i have one service that get data , and i called it 5 times with different parametes to get different data. I called a function to execute in success case : it work fine. but in case of failure one from the 5 calls i need to do something else what's not happen : it always enter in success function. I'm using ionic 4 angular 2 this is the service i have :

 public getdataLookUps(type, lcid): Promise<string[]> {
return new Promise((resolve, reject) => {
  if (this.data[type + lcid]) {
    resolve(this.data[type + lcid]);
    return;
  }
  this.authService.getToken().then(
    (accessToken) => {
      let headers = new Headers({'Authorization': 'bearer ' + accessToken});
      let url = 'error url to test failure case';
      this.http.get(url, {headers: headers})
        .map(res => res.json())
        .toPromise()
        .then(
          (res) => {
            this.data[type + lcid] = res;
            resolve(res);
          },
          (error) => {
            reject(error);
          }
        );
    }
  );
});

}

then I wrapper the function that calls the service like this: ( repeated 5 times with different params):

public getAreas() {
    return this.lookupsService.getdataLookUps('Region', this.lcid).then(
      (res) => {
        this.areas = res;
      },
      () => {
        //todo
return Promise.reject('rejection error');
      }
    );
  }

then I call the 5 functions :

  ngOnInit() {
    this.getCaseCategories();
    this.getAreas();
    this.getWeather();
    this.getMifonProjects();
    this.getUserInfo();
}

and I do promise.all() here :

ngAfterViewInit(){
 Promise.all(
      [
        this.getCaseCategories(),
        this.getAreas(),
        this.getWeather(),
        this.getMifonProjects(),
        this.getUserInfo(),
      ]
    ).then(
      () => {
        this.loadMap();
      },
      () => {
        this.showErrorMessage = true;
      }
    );
}
Joe Sleiman
  • 2,416
  • 5
  • 25
  • 40
  • if i add return Promise.reject inside getArea() wrapper function , I got this error : Uncaught (in promise): 1 – Joe Sleiman Jun 09 '17 at 14:39
  • maybe this article is usefull: https://stackoverflow.com/questions/30362733/handling-errors-in-promise-all – enno.void Jun 09 '17 at 14:40
  • Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it) in `getdataLookUps`! – Bergi Jun 09 '17 at 14:41
  • @Bergi I didn't get you – Joe Sleiman Jun 09 '17 at 14:43
  • @JoeSleiman Write it without a `new Promise` call, only by chaining. Hint: your current approach led you to not handle errors from `this.authService.getToken()` – Bergi Jun 09 '17 at 14:45
  • @Bergi can you write the service how it will become? – Joe Sleiman Jun 09 '17 at 14:47
  • it enters the failure message in promise.all but an error occured : Uncaught (in promise): at c (polyfills.js:3) at c (polyfills.js:3) at polyfills.js:3 at t.invokeTask (polyfills.js:3) at Object.onInvokeTask (core.es5.js:4119) at t.invokeTask (polyfills.js:3) at r.runTask (polyfills.js:3) at o (polyfills.js:3) at XMLHttpRequest.invoke (polyfills.js:3) – Joe Sleiman Jun 09 '17 at 15:10
  • I don't think it's useful to implement your own HTTP cache. The browser has an HTTP cache that works a lot better than simply chucking everything into a `this.data` object. Just re-issue HTTP requests and control cache validity with response headers. Expired items will be re-requested and the fresh items will be available immediately. – Tomalak Jun 09 '17 at 15:49
  • @Tomalak without using cache the browser didn't cache anything. Because when i access the form it call the request and if go back and return to the same form it calls again so because of that i did the cache – Joe Sleiman Jun 09 '17 at 16:17
  • 1
    Well if the browser did not cache anything then set proper `Cache-Control` response headers (and - if you have the developer tools open - uncheck the "always reload from server" checkbox in the network tab which overrides the regular cache handling). – Tomalak Jun 09 '17 at 16:20
  • @Tomalak i think Isn't a solution for a mobile app – Joe Sleiman Jun 09 '17 at 16:24
  • Huh? Why would using the browser-integrated cache not be a solution for a mobile app? What has mobile/not mobile to do with anything anyway? – Tomalak Jun 09 '17 at 16:25
  • Because the user on a mobile app didn't have developer tools to uncheck the " always reload from server " . Honestely i don't know how this happen . I want to do some search – Joe Sleiman Jun 09 '17 at 16:28
  • The "always reload from server" is only in effect as long as the developer tools are open, this does not really apply to mobile apps. ;) I was only mentioning it because it might look like caching would not work when you are developing/debugging your app. – Tomalak Jun 09 '17 at 16:59
  • Ahh yea i know , i putting it off because i want to test if the cache method that i do works or no. – Joe Sleiman Jun 09 '17 at 17:12

2 Answers2

3

This code has two callbacks for then, a success handler, and an error handler. If the code is as you have shown the error handler returns a success result so your Promise.all() will always succeed:

public getAreas() {
    return this.lookupsService.getdataLookUps('Region', this.lcid).then(
      (res) => {
        this.areas = res;
      },
      () => {
        //todo
      }
    );
  }

Don't add an error handler unless you are really able to handle the error here. Instead just let the error propagate out to the next handler:

public getAreas() {
    return this.lookupsService.getdataLookUps('Region', this.lcid)
    .then(res => this.areas = res);
  }

Now your Promise.all will give you an error when the data lookup fails.

Also stop nesting your promise handlers:

public getdataLookUps(type, lcid): Promise<string[]> {
    if (this.data[type + lcid]) return Promise.resolve(this.data[type + lcid]);
    return this.authService.getToken().then(
      (accessToken) => {
        let headers = new Headers({'Authorization': 'bearer ' + accessToken});
        let url = 'error url to test failure case';
        return this.http.get(url, {headers: headers})
          .map(res => res.json())
          .toPromise();
     })
     .then((res) => this.data[type + lcid] = res);
}

Once you have a Promise just return the Promise there is no need to create a new Promise. And if your promise success handler creates another promise return that to avoid nesting. Your error handler did nothing but propagate the error, so when you don't have the nested promise you don't need that either, just let the error propagate naturally.

Duncan
  • 92,073
  • 11
  • 122
  • 156
0

I solved it by removing the calls of the functions in ngOnInit(); and keep everything same as my example above (not change anything in the getDataLookUps service)

Joe Sleiman
  • 2,416
  • 5
  • 25
  • 40