-
-
Notifications
You must be signed in to change notification settings - Fork 29
feat(forms-manager): add support for session storage #32
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
Adds ability to use session storage for persistence, configurable through NgFormsManagerConfig
|
Thanks for the PR. I'm wondering why not use one token: export const FORMS_MANAGER_STORAGE = new InjectionToken<Storage | undefined>('FORMS_MANAGER_STORAGE', {
providedIn: 'root',
factory: () => (isPlatformBrowser(inject(PLATFORM_ID)) ? localStorage : undefined),
});Now if someone wants to use {
provide: FORMS_MANAGER_STORAGE,
useValue: sessionStorage
}We can also export it from the library: export const FORMS_MANAGER_SESSION_STORAGE = {
provide: FORMS_MANAGER_STORAGE,
....
} |
Adds ability to use sessionStorage or other custom storage solution for persistence.
|
Thanks for your feedback! I agree with your suggested approach. It's simpler and more flexible. I have updated the PR accordingly. Please let me know if you have any further comments. Thanks, |
| constructor(@Optional() @Inject(NG_FORMS_MANAGER_CONFIG) private config: NgFormsManagerConfig) { | ||
| constructor( | ||
| @Optional() @Inject(NG_FORMS_MANAGER_CONFIG) private config: NgFormsManagerConfig, | ||
| @Optional() @Inject(FORMS_MANAGER_STORAGE) private readonly browserStorage?: Storage |
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.
The default is localStorage so why do we need @optional?
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.
|
|
||
| describe('FORMS_MANAGER_SESSION_STORAGE_PROVIDER', () => { | ||
| it('should provide SESSION_STORAGE_TOKEN', () => { | ||
| TestBed.configureTestingModule({ providers: [FORMS_MANAGER_SESSION_STORAGE_PROVIDER] }); |
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.
Could you please test that it uses sessionStorage in the manager?
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.
I think, this is already tested through the test in injection-tokens.spec.ts.
describe('FORMS_MANAGER_SESSION_STORAGE_PROVIDER', () => {
it('should provide SESSION_STORAGE_TOKEN', () => {
TestBed.configureTestingModule({ providers: [FORMS_MANAGER_SESSION_STORAGE_PROVIDER] });
expect(TestBed.inject(FORMS_MANAGER_STORAGE)).toBe(TestBed.inject(SESSION_STORAGE_TOKEN));
});
});
However, I have added a test to forms-manager.spec.ts as requested.
Adds ability to use sessionStorage or other custom storage solution for persistence.
Adds ability to use session storage for persistence, configurable through NgFormsManagerConfig
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, when
persistStateis set totrueinUpsertConfig, state is always persisted to local storage.Issue Number: N/A
What is the new behavior?
Through configuration using the
NG_FORMS_MANAGER_CONFIGtoken, the user can now choose to use session storage instead. Projects using the library that don't use the new configuration option will continue to use local storage by default.Configuration example:
Does this PR introduce a breaking change?
Other information
This provides a simple solution to the common requirement of having to delete any user data when the browser session closes. This solution is backward-compatible, requires minimal configuration and is fully tested and documented.