angelnikolov / ts-cacheable

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

Invalidate cache when parameter is an object #7

Closed ThibaudAV closed 6 years ago

ThibaudAV commented 6 years ago

Hi,

I would like to run this test in cacheable.decorator.spec.ts

[...]
  @Cacheable()
  getDataWithParamObj(parameter: any) {
    return this.mockServiceCall(parameter);
  }
[...]
  it("return the cached observable with params object", () => {
    let params = {
      limit: 5,
      stars: ["1"],
      page: 1,
      q: ""
    };
    /**
     * call the service endpoint five hundred times with the same parameter
     * but the service should only be called once, since the observable will be cached
     */
    for (let i = 0; i < 500; i++) {
      service.getDataWithParamObj(params);
    }

    expect(mockServiceCallSpy).toHaveBeenCalledTimes(1);
    /**
     * return the response
     */
    jasmine.clock().tick(1000);

    params.stars.push("2");
    /**
     * call again..
     */
    service.getDataWithParamObj(params);
    /**
     * service call count should still be 1, since we are returning from cache now
     */
    expect(mockServiceCallSpy).toHaveBeenCalledTimes(2);
  });
[...]

Wouldn't it miss a clone of the parameters before saving them as old parameters. or something like that ?

Thank you for your help 🙂

UPDATE :

If you add cloneDeep of the parameters before the call to the "cacheable observable" it corrects the problem. But I still think it's up to the lib to do the job 🙂

If I find what I need to modify in the library, I will propose a PR 😉

ThibaudAV commented 6 years ago

I think we should add:

      (propertyDescriptor.value as any) = function(..._parameters) {
        let parameters = JSON.parse(JSON.stringify(_parameters));

here cacheable.decorator.ts#L79

angelnikolov commented 6 years ago

Yeah that makes sense, since you are passing a referential type, the stringify comparison that's done later will always return true. Can you make a PR about that and I will merge it and re-publish as soon as I can :) Thanks!