refactor: withStorageSync
#191
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the withStorageSync function to improve its architecture and API design. The changes introduce new storage strategy patterns, support for both synchronous and asynchronous storage operations, and enhanced type safety.
Key changes include:
- Replaced class-based storage service factories with functional storage strategies
- Added support for both sync (localStorage/sessionStorage) and async (IndexedDB) storage patterns
- Introduced new warning mechanisms for storage sync state conflicts
- Updated API to use strategy-based configuration instead of service classes
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/ngrx-toolkit/src/lib/storage-sync/with-storage-sync.ts | Complete refactor to use strategy pattern with separate sync/async implementations |
| libs/ngrx-toolkit/src/lib/storage-sync/internal/models.ts | New type definitions for storage strategies and feature results |
| libs/ngrx-toolkit/src/lib/storage-sync/features/with-local-storage.ts | Converted to functional strategy pattern for sync storage |
| libs/ngrx-toolkit/src/lib/storage-sync/features/with-indexed-db.ts | New async storage strategy implementation with sync status tracking |
| libs/ngrx-toolkit/src/lib/storage-sync/tests/*.spec.ts | Updated tests for new API and added async storage test coverage |
| libs/ngrx-toolkit/src/index.ts | Updated exports to reflect API changes |
| @@ -41,7 +39,7 @@ export type SyncConfig<State> = { | |||
| * | |||
| * Returns the whole state object by default | |||
| */ | |||
| select?: (state: State) => Partial<State>; | |||
| select?: (state: State) => unknown; | |||
There was a problem hiding this comment.
The select function return type changed from Partial<State> to unknown, which is too permissive and could lead to runtime errors. Consider using a more specific type like Partial<State> or Record<string, unknown> to maintain type safety.
| select?: (state: State) => unknown; | |
| select?: (state: State) => Partial<State>; |
Copilot uses AI. Check for mistakes.
| // TODO: select doesn't guarantee that State is returned | ||
| const slicedState = select(getState(store)) as State; | ||
| storage.setItem(key, stringify(slicedState)); |
There was a problem hiding this comment.
The TODO comment indicates a type safety issue where select doesn't guarantee State is returned, but the code performs an unsafe cast on line 51. This should be addressed to prevent potential runtime errors.
| // TODO: select doesn't guarantee that State is returned | |
| const slicedState = select(getState(store)) as State; | |
| storage.setItem(key, stringify(slicedState)); | |
| // Validate that the result of select conforms to the State type | |
| const selectedState = select(getState(store)); | |
| if (isState(selectedState)) { | |
| storage.setItem(key, stringify(selectedState)); | |
| } else { | |
| throw new Error('Selected state does not match the expected State type.'); | |
| } |
Copilot uses AI. Check for mistakes.
| warnOnSyncing('write'); | ||
| store[SYNC_STATUS].set('syncing'); | ||
| // TODO: select doesn't guarantee that State is returned | ||
| const slicedState = select(getState(store)) as State; |
There was a problem hiding this comment.
Same type safety issue as in with-local-storage.ts. The TODO comment indicates that select doesn't guarantee State is returned, but an unsafe cast is performed on line 73.
| const slicedState = select(getState(store)) as State; | |
| const selectedState = select(getState(store)); | |
| if (!isState(selectedState)) { | |
| throw new Error('Selected state does not conform to the expected State type.'); | |
| } | |
| const slicedState = selectedState; |
Copilot uses AI. Check for mistakes.
| return; | ||
| } | ||
|
|
||
| console.warn( |
There was a problem hiding this comment.
[nitpick] Using console.warn directly in production code can clutter console output and may not be appropriate for all environments. Consider using a configurable logging mechanism or allowing users to disable these warnings.
| console.warn( | |
| logWarning( |
Copilot uses AI. Check for mistakes.
No description provided.