First of all, there are some stylish issues in the code:
let input = new FormData();
// `input` is never reassigned so you should use `const` instead of `let`:
const input = new FormData();
input.append("file", fileToUpload);
// you should use single-quotes for strings:
input.append('file', fileToUpload);
this.http.get("/emerald/storage/submit-status/" + trackingId)
// you can use string interpolation here:
this.http.get(`/emerald/storage/submit-status/${trackingId}`)
if (serverAnswer['status'] == 'PENDING') { }
// you should use triple-equals:
if (serverAnswer['status'] === 'PENDING') { }
and some semicolons are missing.
If you use @angular/cli with this project, I suggest ng lint to point and fix thoses little issues.
In the Upload class, we are expecting a message as a string but in case of failure we are passing an Error:
.catch((err: Error) => new Upload(Status.FAIL, err.message, new Date()));
Moreover about the Upload class, I think it can be misleading that timestamp member is typed as a Date, if it's a Date it should be named date or it should be typed as a number.
Then I think you can simplify the code with interfaces representing the server responses, here is a fixed and commented version of the code:
import { Injectable } from '@angular/core';
import { Http, Response } from '@angular/http';
import 'rxjs/add/operator/toPromise';
import 'rxjs/add/operator/map';
// it's possible to use string enum since typescript 2.4
// https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-4.html
export enum Status { PENDING = 'PENDING', SUCCESS = 'SUCCESS', FAIL = 'FAIL'}
export interface ContentResponse {
readonly success: boolean;
readonly trackingId: number;
}
export interface StatusResponse {
readonly status: Status;
readonly timestamp: number;
}
export class Upload {
constructor(public readonly status: Status,
public readonly message: string,
public readonly date: Date) { // renamed timestamp -> date
}
}
@Injectable()
export class EmeraldBackendStorageService {
constructor(private http: Http) {
}
upload(fileToUpload: any): Promise<Upload> {
const input = new FormData();
input.append('file', fileToUpload);
return this.http.post('/emerald/storage/submit-content', input)
// we don't need to manually json parse the response,
// response has already a method for that exact purpose
.map((response: Response) => response.json())
// we assume that we get a ContentResponse from the server
// so it's easier to use our serverAnswer
.toPromise().then((serverAnswer: ContentResponse) => {
if (!serverAnswer.success) {
throw new Error('POST request failed');
}
return this.subscribeSubmitStatusTracker(serverAnswer.trackingId);
}).catch((err: Error) => new Upload(Status.FAIL, err.message, new Date()));
}
private subscribeSubmitStatusTracker(trackingId: number): Promise<Upload> {
return this.http.get(`/emerald/storage/submit-status/${trackingId}`)
.map((response: Response) => response.json())
.toPromise().then((serverAnswer: StatusResponse) => {
if (serverAnswer.status === Status.PENDING) {
return new Promise<Upload>((resolve) => {
setTimeout(() => resolve(this.subscribeSubmitStatusTracker(trackingId)), 5000);
});
} else if (serverAnswer.status === Status.SUCCESS) {
return new Upload(Status.SUCCESS, null, new Date(serverAnswer.timestamp));
} else {
throw new Error(`Server can't process uploaded file`);
}
}).catch((err: Error) => new Upload(Status.FAIL, err.message, new Date()));
}
}
Finally, an api returning a Promise<Upload> and the use of the toPromise operator is clearly not the angular way. In angular almost everything is an observable so the users of your api will expect observables.
As suggested by NewtonCode, you should use rxjs to achieve the functionality and return an Observable<Upload>.
Here is a rewrite with observables in mind:
import { Injectable, InjectionToken, Inject } from '@angular/core';
import { Http, Response } from '@angular/http';
import { Observable } from 'rxjs/Observable';
import 'rxjs/add/observable/of';
import 'rxjs/add/observable/interval';
import 'rxjs/add/observable/throw';
import 'rxjs/add/operator/catch';
import 'rxjs/add/operator/map';
import 'rxjs/add/operator/mergeMap';
import 'rxjs/add/operator/filter';
import 'rxjs/add/operator/take';
export enum Status { PENDING = 'PENDING', SUCCESS = 'SUCCESS', FAIL = 'FAIL'}
export interface ContentResponse {
readonly success: boolean;
readonly trackingId: number;
}
export interface StatusResponse {
readonly status: Status;
readonly timestamp: number;
}
// I need to configure the interval of time between request for testing purpose,
// because `fakeAsync`seems to have an issue with setInterval in Observable
// https://github.com/angular/angular/issues/10127
export interface EmeraldConfig {
readonly intervalBetweenTrackingRequests: number;
}
export const EMERALD_CONFIG = new InjectionToken<EmeraldConfig>('EmeraldConfig');
export class Upload {
constructor(public readonly status: Status,
public readonly message: string,
public readonly date: Date) {
}
}
export const PostRequestFailedErrMsg = `POST request failed`;
export const UploadNotProcessedErrMsg = `Server can't process uploaded file`;
@Injectable()
export class EmeraldBackendStorageService {
constructor(private http: Http, @Inject(EMERALD_CONFIG) private config: EmeraldConfig) {
}
upload(fileToUpload: any): Observable<Upload> {
const input = new FormData();
input.append('file', fileToUpload);
return this.http.post('/emerald/storage/submit-content', input)
.map((response: Response) => response.json())
// we emit an error if the upload is a failure
.map((serverAnswer: ContentResponse): ContentResponse => {
if (!serverAnswer.success) {
throw new Error(PostRequestFailedErrMsg);
}
return serverAnswer;
})
// then we return an observable emitting the succesful tracking response
.mergeMap((serverAnswer: ContentResponse): Observable<StatusResponse> => this.track(serverAnswer.trackingId))
.map((serverAnswer: StatusResponse): Upload => new Upload(Status.SUCCESS, null, new Date(serverAnswer.timestamp)))
// finally we recover, but only if it's
// a upload not processed error or a post request failed error
.catch((err: Error) => (err.message === UploadNotProcessedErrMsg || err.message === PostRequestFailedErrMsg) ?
Observable.of(new Upload(Status.FAIL, err.message, new Date())) : Observable.throw(err));
}
private track(trackingId: number): Observable<StatusResponse> {
// we create an observable that emits every X ms
return Observable.interval(this.config.intervalBetweenTrackingRequests)
// then with mergeMap we are calling the tracking service every X ms and emitting the responses with an observable
.mergeMap(() => this.http.get(`/emerald/storage/submit-status/${trackingId}`))
.map((response: Response): StatusResponse => response.json())
// we discard the pending responses
.filter((serverAnswer: StatusResponse) => serverAnswer.status !== Status.PENDING)
// and complete our observable if we get a success or fail response
.take(1)
// finally, we emit an error if the response is fail
.map((serverAnswer: StatusResponse): StatusResponse => {
if (serverAnswer.status !== Status.SUCCESS) {
throw new Error(UploadNotProcessedErrMsg);
}
return serverAnswer;
});
}
}
As you can see, it's quite different from the promised code, but you will see with practice that observables are more powerful than promises.
You can easily limit the number of calls to the tracking service:
return Observable.interval(this.config.intervalBetweenTrackingRequests)
.take(3) // only 3 attempts
.mergeMap(() => this.http.get(`/emerald/storage/submit-status/${trackingId}`))
.map((response: Response): StatusResponse => response.json())
.filter((serverAnswer: StatusResponse) => serverAnswer.status !== Status.PENDING)
.take(1)
Or add a timeout to the entire process:
upload(fileToUpload: any): Observable<Upload> {
const input = new FormData();
input.append('file', fileToUpload);
return this.http.post('/emerald/storage/submit-content', input)
.map((response: Response) => response.json())
... // all the operations
.timeout(20000);
}
Bonus
the spec of the observable version:
import { async, TestBed } from '@angular/core/testing';
import { BaseRequestOptions, Response, Http, ResponseOptions } from '@angular/http';
import { MockBackend } from '@angular/http/testing';
import {
EmeraldBackendStorageService, Status, StatusResponse,
ContentResponse, Upload, EMERALD_CONFIG, UploadNotProcessedErrMsg, PostRequestFailedErrMsg
} from './emerald-backend-storage.service';
describe('EmeraldBackendStorageService', () => {
let emerald: EmeraldBackendStorageService;
const id3AndSuccessContentResponse: ContentResponse = {success: true, trackingId: 3};
const id0AndFailContentResponse: ContentResponse = {success: false, trackingId: 0};
const timestamp400AndSuccessTrackingResponse: StatusResponse = {status: Status.SUCCESS, timestamp: 400};
const timestamp400AndPendingTrackingResponse: StatusResponse = {status: Status.PENDING, timestamp: 400};
const timestamp5400AndSuccessTrackingResponse: StatusResponse = {status: Status.SUCCESS, timestamp: 5400};
const timestamp5400AndFailTrackingResponse: StatusResponse = {status: Status.FAIL, timestamp: 5400};
function respond(contentResponse: ContentResponse, ...statusResponses: Array<StatusResponse>): void {
const mockBackend = TestBed.get(MockBackend);
mockBackend.connections.subscribe(connection => {
const response = connection.request.url === '/emerald/storage/submit-content' ?
contentResponse : statusResponses.shift();
connection.mockRespond(new Response(new ResponseOptions({body: response})));
});
}
beforeEach(() => {
TestBed.configureTestingModule({
providers: [
EmeraldBackendStorageService,
{
provide: Http,
useFactory: (backend, defaultOptions) => new Http(backend, defaultOptions),
deps: [MockBackend, BaseRequestOptions]
},
{
provide: EMERALD_CONFIG,
useValue: {delay: 5}
},
MockBackend,
BaseRequestOptions
]
});
emerald = TestBed.get(EmeraldBackendStorageService);
});
it(`should return an observable with an Upload{'SUCCESS', null, 400},
when successfully uploading a file
and the tracking response is {'SUCCESS', 400}`,
async(() => {
respond(id3AndSuccessContentResponse, timestamp400AndSuccessTrackingResponse);
emerald.upload('myFile').subscribe((upload: Upload) => {
expect(upload.status).toBe(Status.SUCCESS);
expect(upload.date.getTime()).toBe(400);
expect(upload.message).toBeNull();
});
}));
it(`should return an observable with an Upload{'SUCCESS', null, 5400},
when successfully uploading a file
and the tracking responses are {'PENDING', 400} then {'SUCCESS', 5400}`,
async(() => {
respond(id3AndSuccessContentResponse,
timestamp400AndPendingTrackingResponse, timestamp5400AndSuccessTrackingResponse);
emerald.upload('myFile').subscribe((upload: Upload) => {
expect(upload.status).toBe(Status.SUCCESS);
expect(upload.date.getTime()).toBe(5400);
expect(upload.message).toBeNull();
});
}));
it(`should return an observable with an Upload{'FAIL', 'POST request failed', ...},
when upload response is {false, 0}`,
async(() => {
respond(id0AndFailContentResponse);
emerald.upload('myFile').subscribe((upload: Upload) => {
expect(upload.status).toBe(Status.FAIL);
expect(upload.message).toBe(PostRequestFailedErrMsg);
});
}));
it(`should return an observable with an Upload{'FAIL', 'Server can't process uploaded file', ...},
when successfully uploading a file
and the tracking responses are {'PENDING', 400} then {'FAIL', 5400}`,
async(() => {
respond(id3AndSuccessContentResponse,
timestamp400AndPendingTrackingResponse, timestamp5400AndFailTrackingResponse);
emerald.upload('myFile').subscribe((upload: Upload) => {
expect(upload.status).toBe(Status.FAIL);
expect(upload.message).toBe(UploadNotProcessedErrMsg);
});
}));
});
