-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: Add the possibility to specify action types to invoke the reducer in the storage sync helper #1320
base: release/9.6
Are you sure you want to change the base?
feat: Add the possibility to specify action types to invoke the reducer in the storage sync helper #1320
Conversation
69cd590
to
3098090
Compare
Essentially, when a store is imported inside a lazy loaded module and the store sync meta reducer is used, the state is undefined as can be seen in the attached screenshot: Since the action type INIT is specified, the reducer is not invoked resulting in an empty state. So, for such scenarios we need to specify the UPDATE action type as well for invoking the reducer. We do not have the issue when the sync is done in the root app module. |
Thank you for the explanation. |
3098090
to
6f91a4c
Compare
@@ -328,7 +331,7 @@ export const syncStorage = (config: SyncStorageConfig) => (reducer: any) => { | |||
|
|||
// If state arrives undefined, we need to let it through the supplied reducer | |||
// in order to get a complete state as defined by user | |||
if (action.type === INIT && !state) { | |||
if ((action.type === INIT || action.type === UPDATE || action.type === RECOMPUTE) && !state) { |
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.
UPDATE
and RECOMPUTE
should trigger the reducer only if no INIT
has been emitted before for this store, isn't it?
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.
Yes and when syncing the state for feature stores, INIT is not dispatched. Only UPDATE and RECOMPUTE (in dev mode)
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.
Turned this comment as unresolved, please do not resolve a question without the question author approval
If we agree on the fact that the reducer
should be called on RECOMPUTE
and INIT
actions only if the INIT
event has not be emitted for this store, this is not the implementation on the code.
So:
- either you should consider checking that the
INIT
action has not been emitted to this store before checking for the 2 others actions, - either it s a case it never happen (then an explanation as response of the question would be appreciated),
- either executing the
reducer
a second time on a store that has already got anINIT
action is not an issue (an explanation would also be needed).
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.
As mentioned above, when syncing the state using the syncStorage
method within a lazily loaded feature module, the INIT
action is not received and therefore, the reducer is not invoked resulting in an undefined state.
So, point no 2 from your comment. INIT
is never called.
Please check the minimal reproduction of the same: https://github.com/Abhirocks889/store-sync-error
@@ -3,6 +3,9 @@ import { deepFill } from '@o3r/core'; | |||
import type { Logger } from '@o3r/core'; | |||
import type { StorageKeyConfiguration, StorageKeys, SyncStorageConfig, SyncStorageSyncOptions } from './interfaces'; | |||
|
|||
/** The recompute action type of ngrx store devtools */ | |||
const RECOMPUTE = '@ngrx/store-devtools/recompute'; |
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.
you could use RECOMPUTE
from @ngrx/store-devtools
instead of redefining the variable
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.
That would add the entire store dev tools chunk to the main bundle if we use this method for syncing stores in the app module.
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.
A compromise can be to import the type only (to ensure alignment):
import type { RECOMPUTE } from '@ngrx/store-devtools';
const RECOMPUTE: typeof RECOMPUTE = '@ngrx/store-devtools/recompute';
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.
Updated
Also, @kpanot , @matthieu-crouzet I have created a repo for minimal reproduction of the issue: |
985abb2
to
fda2794
Compare
@@ -328,7 +332,7 @@ export const syncStorage = (config: SyncStorageConfig) => (reducer: any) => { | |||
|
|||
// If state arrives undefined, we need to let it through the supplied reducer | |||
// in order to get a complete state as defined by user | |||
if (action.type === INIT && !state) { |
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.
Maybe we should check syncForFeature
when action.type
is not INIT
I don't understand why we need RECOMPUTE
because UPDATE
will be called when loading the lazy loaded store, and it should be enough no ?
RECOMPUTE
seems to be for devtools so in production mode it will not be called no ?
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.
Added the check. Also, for the second question, we need RECOMPUTE
because in dev mode, we get an undefined state for both UPDATE
and RECOMPUTE
action types even if the reducer is invoked for UPDATE
, the state arrives as undefined for RECOMPUTE
. Hence, the app breaks and is not usable.
Please check the reproduction: https://github.com/Abhirocks889/store-sync-error
12163d0
to
b7d8ed7
Compare
…er in the storage sync helper
10f2e55
to
97a95a0
Compare
It seems that your issue could be fixed by using |
@Abhirocks889 , is this PR still required or did the fix mentioned by @matthieu-crouzet fix your issue? |
Proposed change
This PR adds a new optional property actionTypes to the SyncStorageConfig that will specify the action types for which the reducer should be invoked in case the state is undefined. This is required for syncing feature stores for which the action type UPDATE needs to be taken into account.
Related issues