-
-
Notifications
You must be signed in to change notification settings - Fork 35
feat: add dynamic cache storage strategy support #106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hi, do you think you can upgrade to Angular 20? |
Sure thing, right on it. Will then be a breaking release tho. |
private _getCacheServices(storageStrategy?: CacheStorageStrategy): ResolvedCacheServices { | ||
if (storageStrategy === 'localStorage') { | ||
const lsStorage = this.injector.get(LocalStorageHttpCacheStorage, null, { optional: true }); | ||
const lsTtl = this.injector.get(LocalStorageTTLManager, null, { optional: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you're importing the services, it's still make it not tree-shakable :)
but never mind, it's not a lot of a code, don't bother
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored it to use injection tokens instead and a generic browser storage class
ttlManager.clear(); | ||
versionsManager.clear(); | ||
} else { | ||
// Clear all storages (memory, localStorage and sessionStorage if available) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we need to clear all? there is one global strategy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we now have multiple storages at the same time, i thought this could be a way to provide granular control of what a user wants to clear.
Clear all caches (memory, local storage, etc.)
clearCache() {
this.manager.clear();
}
Clear just 1 specific cache storage, e.g.: localStorage
clearCache() {
this.manager.clear('localStorage');
}
} | ||
|
||
@Injectable() | ||
export class SessionStorageTTLManager extends TTLManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all session and local storage can be use the same base class just pass super(sessionStorage/localStorage)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
The commit should include the breaking changes for the release |
The whole PR or which one do you refer to? Since the Angular upgrade was already a breaking one i reckon. |
Any breaking change that was introduced in this PR |
Yeah I marked the commit for the angular migration to v20 as breaking. But otherwise it should not break the current local storage implementation. |
The build needs a fix |
Updated CI workflow to use latest action versions and from node 18 --> 22. |
PR Checklist
PR Type
What is the current behavior?
The library required a provider for local storage strategy, and consumers had to use it exclusively. Storage strategy was not configurable per request.
Issue Number: 105
What is the new behavior?
Consumers can now specify the storage strategy (e.g.,
localStorage
,sessionStorage
,memory
) per request using thewithCache
context. The old provider is deprecated but still available for backward compatibility.Does this PR introduce a breaking change?
Angular 20 migration included.
Other Info
provideHttpCacheLocalStorageStrategy
is no longer needed and was deprecated.Documentation and tests have been updated to reflect the new usage.