From 5655ce433189efd6d93a58fe3719fd32164eb88d Mon Sep 17 00:00:00 2001 From: Arman Ozak Date: Fri, 20 Mar 2020 14:00:00 +0300 Subject: [PATCH 1/3] feat(core): add a utility function that checks if a value is undefined or empty string --- .../core/src/lib/tests/common-utils.spec.ts | 18 +++++++++++++++++- .../core/src/lib/utils/common-utils.ts | 4 ++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/npm/ng-packs/packages/core/src/lib/tests/common-utils.spec.ts b/npm/ng-packs/packages/core/src/lib/tests/common-utils.spec.ts index 98c6614987..60280e396b 100644 --- a/npm/ng-packs/packages/core/src/lib/tests/common-utils.spec.ts +++ b/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); + }); + }); }); diff --git a/npm/ng-packs/packages/core/src/lib/utils/common-utils.ts b/npm/ng-packs/packages/core/src/lib/utils/common-utils.ts index 756d3a34dd..b8609be130 100644 --- a/npm/ng-packs/packages/core/src/lib/utils/common-utils.ts +++ b/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 === ''; +} From ddf286d47760be173d1e14097c5c8a2f7c49e579 Mon Sep 17 00:00:00 2001 From: Arman Ozak Date: Fri, 20 Mar 2020 14:22:58 +0300 Subject: [PATCH 2/3] refactor(core): change internals of request method of RestService --- .../core/src/lib/services/rest.service.ts | 42 ++++++++--------- .../core/src/lib/tests/rest.service.spec.ts | 46 ++++++++----------- 2 files changed, 36 insertions(+), 52 deletions(-) diff --git a/npm/ng-packs/packages/core/src/lib/services/rest.service.ts b/npm/ng-packs/packages/core/src/lib/services/rest.service.ts index 15cc13cf50..eaebecf994 100644 --- a/npm/ng-packs/packages/core/src/lib/services/rest.service.ts +++ b/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'; @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 { this.store.dispatch(new RestOccurError(err)); - console.error(err); return throwError(err); } + // TODO: Deprecate service or improve interface in v3.0 request( request: HttpRequest | Rest.Request, config?: Rest.Config, api?: string, ): Observable { config = config || ({} as Rest.Config); - const { observe = Rest.Observe.Body, skipHandleError } = config; - const url = - (api || this.store.selectSnapshot(ConfigState.getApiUrl(config.apiName))) + request.url; + api = api || this.getApiFromStore(config.apiName); const { method, params, ...options } = request; + const { observe = Rest.Observe.Body, skipHandleError } = config; return this.http - .request(method, url, { + .request(method, api + request.url, { observe, ...(params && { - params: Object.keys(params).reduce( - (acc, key) => ({ - ...acc, - ...(typeof params[key] !== 'undefined' && - params[key] !== '' && { [key]: params[key] }), - }), - {}, - ), + params: Object.keys(params).reduce((acc, key) => { + const value = params[key]; + + if (!isUndefinedOrEmptyString(value)) acc[key] = value; + + return acc; + }, {}), }), ...options, } as any) - .pipe( - observe === Rest.Observe.Body ? take(1) : tap(), - catchError(err => { - if (skipHandleError) { - return throwError(err); - } - - return this.handleError(err); - }), - ); + .pipe(catchError(err => (skipHandleError ? throwError(err) : this.handleError(err)))); } } diff --git a/npm/ng-packs/packages/core/src/lib/tests/rest.service.spec.ts b/npm/ng-packs/packages/core/src/lib/tests/rest.service.spec.ts index ecc8e9dce8..8251bcabb4 100644 --- a/npm/ng-packs/packages/core/src/lib/tests/rest.service.spec.ts +++ b/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 => { - jest.spyOn(spectator.httpClient, 'request').mockReturnValue(interval(50)); + test('should complete upon successful request', done => { + const complete = jest.fn(done); - const subscriber: Subscription = spectator.service - .request({ method: HttpMethod.GET, url: '/test' }, { observe: Rest.Observe.Body }) - .subscribe(); + spectator.service.request({ method: HttpMethod.GET, url: '/test' }).subscribe({ complete }); - timer(51).subscribe(() => { - expect(subscriber.closed).toBe(true); - 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(); - }); + const req = spectator.expectOne(api + '/test', HttpMethod.GET); + spectator.flushAll([req], [{}]); }); 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')]); }); }); From 4844ca8b53252e64e158804e43ffd33b1b5eaac5 Mon Sep 17 00:00:00 2001 From: Mehmet Erim <34455572+mehmet-erim@users.noreply.github.com> Date: Fri, 20 Mar 2020 16:44:00 +0300 Subject: [PATCH 3/3] Update rest.service.ts --- npm/ng-packs/packages/core/src/lib/services/rest.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/npm/ng-packs/packages/core/src/lib/services/rest.service.ts b/npm/ng-packs/packages/core/src/lib/services/rest.service.ts index eaebecf994..936bc2a768 100644 --- a/npm/ng-packs/packages/core/src/lib/services/rest.service.ts +++ b/npm/ng-packs/packages/core/src/lib/services/rest.service.ts @@ -6,7 +6,7 @@ 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'; +import { isUndefinedOrEmptyString } from '../utils/common-utils'; @Injectable({ providedIn: 'root',