angelnikolov / ts-cacheable

Observable/Promise Cache Decorator
https://npmjs.com/package/ts-cacheable
ISC License
340 stars 42 forks source link

Anguar 7 - Request URL is changed when using @Cacheable #38

Closed exc414 closed 5 years ago

exc414 commented 5 years ago

Hello,

I found this library from an article here

I added the @Cacheable annotation to a request method that makes a GET request to an API.

  @Cacheable({ cacheBusterObserver: cacheBuster$ })
  makeRequest<T>(requestType: string, apiCall: string, params: HttpParams): Observable<T> {
    return this.http.request<T>(requestType, apiCall, {responseType: 'json', params})
      .pipe(catchError(HttpRequestService.errorHandler));
  }

The base URL looks like this when no @Cacheable is used :

https://:8443/api/users.

The HttpParams have a page and size parameters. The whole URL looks like :

https://:8443/api/users?page=1&size=10

However when adding @Cacheable the request URL looks like this (web console) :

https://:8443/api/users?updates=%5Bobject%20Object%5D&updates=%5Bobject%20Object%5D&cloneFrom=%5Bobject%20Object%5D&encoder=%5Bobject%20Object%5D&map=null

I then get no results back as the server returns a 400 error. Since that end point does not exist.

This is what my whole service looks like.

const cacheBuster$ = new Subject<void>();

@Injectable({
  providedIn: 'root'
})
export class HttpRequestService {

  constructor(private http: HttpClient) { }

  static errorHandler(error: HttpErrorResponse) {
    if (error.error instanceof ErrorEvent) {
      console.error('An error occurred:', error.error.message);
    } else {
      console.error(`Backend returned code ${error.status}, ` + `body was: ${error.error}`);
    }
    return throwError('Something bad happened; please try again later.');
  }

  @Cacheable({ cacheBusterObserver: cacheBuster$ })
  makeRequest<T>(requestType: string, apiCall: string, params: HttpParams): Observable<T> {
    return this.http.request<T>(requestType, apiCall, {responseType: 'json', params})
      .pipe(catchError(HttpRequestService.errorHandler));
  }
}

I am using angular 7 and the new HTTP Client. Thank you.

angelnikolov commented 5 years ago

This is due to the fact that I am doing a deep copy on the parameters you call the decorated method with because otherwise, if you pass an object to your method, someone could later mutate that object, which will automatically propagate to the cached object inside the decorator, because it will be the same reference. Let me think how to handle this and I will get back to you :)

angelnikolov commented 5 years ago

So, the problematic line is here. The issue is that your makeRequest method uses angular's HttpParams construct. Three ways we can solve this:

  1. Remove the parameters copying, remove the tests about that and update the documentation stating that the passed parameters should be immutable.
  2. You can fix your code to not depend on HttpParams, but rather use a plain javascript object since Angular also supports that:
    request(method: string, url: string, options: {
        params?: HttpParams | {
            [param: string]: string | string[];
        };
    }
  3. Add a new cacheConfig parameter - mutableParameters: boolean which will dictate if the parameters should be mutable and if such - we won't copy them but leave them as they are. @exc414 Let me know what you think :)
exc414 commented 5 years ago

Hello,

So I went the second route and did not depend on HttpParams. Works fine then. Thank you very much for your help.