angelnikolov / ts-cacheable

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

Problems with Cachable in Angular Universal Server Side Rendering #85

Closed jigfox closed 3 years ago

jigfox commented 4 years ago

when ngx-cachable is used in angular universal the cache is shared for multiple requests, because only one Cache Instance is created in the @Cacheble() decorator on server startup and this is then shared across all requests. In our case we have an interceptor which sets an authentication header, which could be used for the CacheKey, but since it isn't a param of the service methods itself, it can't be used. I'm not sure if this can be easily fixed.

@Injectable()
export class SomeService {
  constructor(private http: HttpService) {}

  @Cachable()
  cachedMethodWithoutParams() {
    return this.http.get('/api/some-resource');
  }
}
@Injectable()
export class AuthInterceptor implements HttpInterceptor {
  constructor(private auth: AuthService) {}

  intercept(request: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> {
    request = request.clone({
      setHeaders: { 'X-Token': this.auth.getToken() },
    });

    return next.handle(request);
  }
}

On client side it works as expected, because we only have one user, but on server side we have multiple users per instance. If it can't be fixed, maybe this situation could be documented?

A workaround could be to remove the interceptor, and add the token vie param on the method like this:

@Injectable()
export class SomeService {
  constructor(private http: HttpService) {}

  @Cachable()
  cachedMethodWithToken(token: string) {
    return this.http.get('/api/some-resource', { headers: { 'X-Token': token } });
  }
}

with the disadvantage that we need to do this on every method using this api

angelnikolov commented 4 years ago

@jigfox, sorry for answering so late. I can see the issue here, but I am afraid that you have also provided the solution :( The decorator does not care about the internals of what's being called in the method it is applied on.

The only idea I can think of is to maybe provide more context (maybe class instance) to cacheResolver. Then you could do something like this: GlobalCacheConfig.cacheResolver = (oldParams, newParams, oldContext, newContext) => oldContext.id !== newContext.id. Than we can also introduce GlobalCacheConfig.cacheResolverContextCallback, which in your case will be setup like this: GlobalCacheConfig.cacheResolverContextCallback = (classInstance: any) => classInstance.authService.getToken() (in this case you need to inject AuthService in every class where you use the decorators) or if you do this in your AppModule initialization (assuming you use Nest here) - GlobalCacheConfig.cacheResolverContextCallback = (classInstance: any) => moduleRef.get(AuthService).getToken() What we then can do is to capture and make copies of the context on every method call and use it as an extra comparator to figure out if cache should be evicted or not. What do you think?

angelnikolov commented 4 years ago

@jigfox Did u have the chance to take a look at my last comment? Thanks!@

jigfox commented 4 years ago

@angelnikolov sorry somehow I missed your response. But after Reading it, it gave me some some Idea: https://github.com/angelnikolov/ngx-cacheable/pull/89 What do you think about this? If you think this is a good Idea, I could work further on this and add some tests

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.