angelnikolov / ts-cacheable

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

@Cacheable on method with Guid as parameter seems not to work (Version: 1.0.7) #20

Closed dscheibe closed 5 years ago

dscheibe commented 5 years ago

The example that is working fine (with id being a Guid.toString()):

@Cacheable()
  getUser(id:string) {
    return this.http
    .get(`api/users/${id}`);
  }

Parameter {id} is resolved to:

GET /api/users/143dffe4-fe9e-40fb-ad49-673f8850bff5

If i know change the string parameter type to Guid like so:

import { Guid } from 'guid-typescript';

@Cacheable()
  getUser(id:Guid) {
    return this.http
    .get(`api/users/${id}`);
  }

In that case for some strange reason, the parameter ${id} that is passed to my api call is resolved to:

GET /api/users/[object%20Object]

Any idea on what i might be doing wrong here?

NOTE: Without @Cacheable, the Guid type is working fine.

Thanks & Cheers!

angelnikolov commented 5 years ago

Can you try calling .toString() on the id, before calling the method? I think the issue is because the cacheable decorator does not know that it needs to call the toString method on the Guid object to extract the actual guid.

dscheibe commented 5 years ago

Yes, i just did that for the id : string parameter type example before passing it to the method and it works fine. Passing it as id : Guid to the method displays the object with @Cacheable in the debugger like that:

{value: "143dffe4-fe9e-40fb-ad49-673f8850bff5"}

So it is still correct but since the type has changed to object the toString() method won't work anymore and using Typescript i can't do neither of the two (the latter obviously won't compile due to value being private in Guid):

@Cacheable()
  getUser(id:Guid) {
    return this.http
    .get(`api/users/${id.toString()}`);
  }
@Cacheable()
  getUser(id:Guid) {
    return this.http
    .get(`api/users/${id.value}`);
  }

Do you see a way having the type of Guid remain a real Guid type instead of object?

angelnikolov commented 5 years ago

I reproduced your use case and it's indeed a bit tricky. However, isn't calling toString before passing it to the method not a solution for you?

dscheibe commented 5 years ago

Sure, it's not the end of the world, but i don't want to change too much of my API, so this is a viable solution for now (which let's me keep my interfaces the same way):

getUser(id:Guid) {
  return this.getUserInternal(id.toString());
}

@Cacheable()
private getUserInternal(id:string) {
  return this.http
  .get(`api/users/${id}`);
}

Maybe you can make it work in a future version, if it is not too tricky... :)

angelnikolov commented 5 years ago

The issue is that pass the parameters through a JSON.parse(JSON.stringify(parameters)) so we make a deep copy of those because of https://github.com/angelnikolov/ngx-cacheable/issues/7 which is also a valid issue. Let me think about it.

dscheibe commented 5 years ago

I see, thanks for taking the time, much appreciated :)