search

angelnikolov / ts-cacheable

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

Added support for caching in local storage #32

Closed kaiser-raja closed 5 years ago

kaiser-raja commented 5 years ago

@angelnikolov I added support for caching in local storage. Please review and merge it.

angelnikolov commented 5 years ago

@kaiser-raja Thank you so much for this PR. I have some notes however. Your implementation couples the CacheableDecorator tightly with a DOM api and since this decorator should be platform-agnostic (meaning - client/server), we need a better way to provide this functionality. What I suggest to do is to create a generic IPersistenceAdapter or something in those lines, which would look like:

export interface IPersistenceAdapter {
     public set(any): any | Promise<any> | Observable<any>;
     public get(): any | Promise<any> | Observable<any>;
}

Then, you can extend the cacheable decorator and add a static property to the Cacheable function called Cacheable.persistenceAdapter. Then you can create a class DomPersistenceAtapter implements IPersistenceAdapter and put it somewhere in the source code of ngx-cacheable but do not depend on it directly from the decorators. Whoever wants to use persistence, will just have to provide the default dom adapter somewhere up top in their code and it should just work. That way, people could even provide different adapters for persistence like Redis, file-system or whatever.

In the decorators we would need to check what's the runtime return types of the set/get functions of the adapter and handle the setting/getting accordingly. We will also need to update the promise.cacheable.decorator.ts with those as well.

Again, thank you so much for your contribution and I hope you see the value in what I am proposing :) If you don't have the time to implement that, I will surely find some time, but won't be today.

kaiser-raja commented 5 years ago

@angelnikolov Thanks a lot for reviewing my code. Your suggestion is greatly appreciated and I will look into implementing this and then resubmitting the code.

kaiser-raja commented 5 years ago

@angelnikolov I have pushed some changes that I made according to what you suggested. Could you please have a look and tell me if I am on the right path? With this new implementation what I am intending is that whoever uses ngx-cacheable will declare a persistence adapter in their code and then pass it to the decorator via the annotation Cacheable.

For instance, we could declare in our code const cache = window.localStorage;

then pass this thus, @Cacheable({persistenceAdapter: cache})

Is this kind of what you wanted?

angelnikolov commented 5 years ago

I think you are on the right track and also given me some food for thought on some things that I thought would be a lot easier to implement. Give me a couple of days and I will show you what I've been thinking and maybe you can do a quick review on it. It's not that different than yours but I had some stuff in mind that would be best explained by code. Thank you so much for this initial implementation!:)

kaiser-raja commented 5 years ago

@angelnikolov I really appreciate you taking out the time to review and comment on my code. Yeah, I'll be very interested in what you have cooked up. I am actually intending to use this library of yours in a project of mine where caching in localStorage is required so it would be nice to have it implemented fully. Best regards.

angelnikolov commented 5 years ago

@kaiser-raja check this out :) https://github.com/angelnikolov/ngx-cacheable/pull/35