Merge pull request #3262 from abpframework/feature/3252

RestService is refactored and optimized

npm/ng-packs/packages/core/src/lib/services/rest.service.ts

@@ -2,10 +2,11 @@ import { HttpClient, HttpRequest } from '@angular/common/http';
 import { Injectable } from '@angular/core';
 import { Store } from '@ngxs/store';
 import { Observable, throwError } from 'rxjs';
-import { catchError, take, tap } from 'rxjs/operators';
+import { catchError } from 'rxjs/operators';
 import { RestOccurError } from '../actions/rest.actions';
 import { Rest } from '../models/rest';
 import { ConfigState } from '../states/config.state';
+import { isUndefinedOrEmptyString } from '../utils/common-utils';
 
 @Injectable({
   providedIn: 'root',
@@ -13,47 +14,40 @@ import { ConfigState } from '../states/config.state';
 export class RestService {
   constructor(private http: HttpClient, private store: Store) {}
 
+  private getApiFromStore(apiName: string): string {
+    return this.store.selectSnapshot(ConfigState.getApiUrl(apiName));
+  }
+
   handleError(err: any): Observable<any> {
     this.store.dispatch(new RestOccurError(err));
-    console.error(err);
     return throwError(err);
   }
 
+  // TODO: Deprecate service or improve interface in v3.0
   request<T, R>(
     request: HttpRequest<T> | Rest.Request<T>,
     config?: Rest.Config,
     api?: string,
   ): Observable<R> {
     config = config || ({} as Rest.Config);
-    const { observe = Rest.Observe.Body, skipHandleError } = config;
+    api = api || this.getApiFromStore(config.apiName);
-    const url =
-      (api || this.store.selectSnapshot(ConfigState.getApiUrl(config.apiName))) + request.url;
     const { method, params, ...options } = request;
+    const { observe = Rest.Observe.Body, skipHandleError } = config;
 
     return this.http
-      .request<T>(method, url, {
+      .request<R>(method, api + request.url, {
         observe,
         ...(params && {
-          params: Object.keys(params).reduce(
+          params: Object.keys(params).reduce((acc, key) => {
-            (acc, key) => ({
+            const value = params[key];
-              ...acc,
+
-              ...(typeof params[key] !== 'undefined' &&
+            if (!isUndefinedOrEmptyString(value)) acc[key] = value;
-                params[key] !== '' && { [key]: params[key] }),
+
-            }),
+            return acc;
-            {},
+          }, {}),
-          ),
         }),
         ...options,
       } as any)
-      .pipe(
+      .pipe(catchError(err => (skipHandleError ? throwError(err) : this.handleError(err))));
-        observe === Rest.Observe.Body ? take(1) : tap(),
-        catchError(err => {
-          if (skipHandleError) {
-            return throwError(err);
-          }
-
-          return this.handleError(err);
-        }),
-      );
   }
 }

npm/ng-packs/packages/core/src/lib/tests/common-utils.spec.ts

@@ -1,4 +1,4 @@
-import { noop } from '../utils';
+import { isUndefinedOrEmptyString, noop } from '../utils';
 
 describe('CommonUtils', () => {
   describe('#noop', () => {
@@ -7,4 +7,20 @@ describe('CommonUtils', () => {
       expect(noop()()).toBeUndefined();
     });
   });
+
+  describe('#isUndefinedOrEmptyString', () => {
+    test.each`
+      value        | expected
+      ${null}      | ${false}
+      ${0}         | ${false}
+      ${true}      | ${false}
+      ${'x'}       | ${false}
+      ${{}}        | ${false}
+      ${[]}        | ${false}
+      ${undefined} | ${true}
+      ${''}        | ${true}
+    `('should return $expected when given parameter is $value', ({ value, expected }) => {
+      expect(isUndefinedOrEmptyString(value)).toBe(expected);
+    });
+  });
 });

npm/ng-packs/packages/core/src/lib/tests/rest.service.spec.ts

@@ -1,9 +1,8 @@
 import { ConfigState } from '@abp/ng.core';
 import { createHttpFactory, HttpMethod, SpectatorHttp, SpyObject } from '@ngneat/spectator/jest';
 import { NgxsModule, Store } from '@ngxs/store';
-import { interval, of, Subscription, throwError, timer } from 'rxjs';
+import { of, throwError } from 'rxjs';
 import { catchError } from 'rxjs/operators';
-import { RestOccurError } from '../actions';
 import { Rest } from '../models';
 import { RestService } from '../services/rest.service';
 
@@ -36,6 +35,10 @@ describe('HttpClient testing', () => {
     });
   });
 
+  afterEach(() => {
+    spectator.controller.verify();
+  });
+
   test('should send a GET request with params', () => {
     spectator.service
       .request({ method: HttpMethod.GET, url: '/test', params: { id: 1 } })
@@ -66,34 +69,16 @@ describe('HttpClient testing', () => {
     spectator.expectOne('bar' + '/test', HttpMethod.GET);
   });
 
-  test('should close the subscriber when observe equal to body', done => {
+  test('should complete upon successful request', done => {
-    jest.spyOn(spectator.httpClient, 'request').mockReturnValue(interval(50));
+    const complete = jest.fn(done);
 
-    const subscriber: Subscription = spectator.service
+    spectator.service.request({ method: HttpMethod.GET, url: '/test' }).subscribe({ complete });
-      .request({ method: HttpMethod.GET, url: '/test' }, { observe: Rest.Observe.Body })
-      .subscribe();
 
-    timer(51).subscribe(() => {
+    const req = spectator.expectOne(api + '/test', HttpMethod.GET);
-      expect(subscriber.closed).toBe(true);
+    spectator.flushAll([req], [{}]);
-      done();
-    });
-  });
-
-  test('should open the subscriber when observe not equal to body', done => {
-    jest.spyOn(spectator.httpClient, 'request').mockReturnValue(interval(50));
-
-    const subscriber: Subscription = spectator.service
-      .request({ method: HttpMethod.GET, url: '/test' }, { observe: Rest.Observe.Events })
-      .subscribe();
-
-    timer(51).subscribe(() => {
-      expect(subscriber.closed).toBe(false);
-      done();
-    });
   });
 
   test('should handle the error', () => {
-    jest.spyOn(spectator.httpClient, 'request').mockReturnValue(throwError('Testing error'));
     const spy = jest.spyOn(store, 'dispatch');
 
     spectator.service
@@ -101,15 +86,17 @@ describe('HttpClient testing', () => {
       .pipe(
         catchError(err => {
           expect(err).toBeTruthy();
-          expect(spy.mock.calls[0][0] instanceof RestOccurError).toBe(true);
+          expect(spy).toHaveBeenCalled();
           return of(null);
         }),
       )
       .subscribe();
+
+    const req = spectator.expectOne(api + '/test', HttpMethod.GET);
+    spectator.flushAll([req], [throwError('Testing error')]);
   });
 
   test('should not handle the error when skipHandleError is true', () => {
-    jest.spyOn(spectator.httpClient, 'request').mockReturnValue(throwError('Testing error'));
     const spy = jest.spyOn(store, 'dispatch');
 
     spectator.service
@@ -120,10 +107,13 @@ describe('HttpClient testing', () => {
       .pipe(
         catchError(err => {
           expect(err).toBeTruthy();
-          expect(spy.mock.calls).toHaveLength(0);
+          expect(spy).toHaveBeenCalledTimes(0);
           return of(null);
         }),
       )
       .subscribe();
+
+    const req = spectator.expectOne(api + '/test', HttpMethod.GET);
+    spectator.flushAll([req], [throwError('Testing error')]);
   });
 });

npm/ng-packs/packages/core/src/lib/utils/common-utils.ts

@@ -3,3 +3,7 @@ export function noop() {
   const fn = function() {};
   return fn;
 }
+
+export function isUndefinedOrEmptyString(value: unknown): boolean {
+  return value === undefined || value === '';
+}