Skip to content
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

fix: state initialization for feature stores #200

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jondewoo
Copy link
Contributor

Feature stores do not receive the @ngrx/store/init action, thus the initialization of the state skipped the merge

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

State is initialized and merged when receiving the @ngrx/store/init action, but in the case of feature stores the first action received is @ngrx/store/update-reducers.

What is the new behavior?

State is now initialized and merged when receiving the @ngrx/store/init action or the @ngrx/store/update-reducers action.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Feature stores do not receive the @ngrx/store/init action, thus the initialization of the state skipped the merge
@BBlackwo BBlackwo added the feature stores Functionality related to feature stores / modules label Aug 31, 2021
@BBlackwo
Copy link
Collaborator

Thanks for the PR @jderrough!

Looks like this is the same change as #155.

I'm a bit hesitant to merge this change in case it has unintended consequences. Can you think of any bugs this might introduce or do you think it's safe?

I think the workaround to this issue is to add the initialState when you call StoreModule.forFeature(). Would that solve your problem? See:

@BBlackwo BBlackwo added the known workaround There is a known workaround for this issue label Aug 31, 2021
@BBlackwo
Copy link
Collaborator

Also, if I do merge this in, I would like to see a unit test added for this scenario if possible.

@SonicDerwille
Copy link

@BBlackwo, this workaround doesn't fix the problem.

Let me post here some examples.

  • Example n.1, not achievable in production due to multiple modules using the same approach:
export function sessionStorage(red: ActionReducer<any>): ActionReducer<any> {
  return localStorageSync({
    keys: [{ userState: ['visits'] }],
    rehydrate: true,
  })(red);
}

@NgModule({
  declarations: [],
  imports: [
    CommonModule,
    StoreModule.forRoot(
      { userState: userStateReducer },
      {
        metaReducers: [sessionStorage],
      }
    ),
    EffectsModule.forRoot([UserStateEffects]),
  ],
})

Outcome: 👌, the object is saved and restored as expected
image

  • Example n.2, forFeature instead of forRoot (in order to have multiple modules to achieve the same result), with PRE-EXISTING DATA from n.1:
export function sessionStorage(red: ActionReducer<any>): ActionReducer<any> {
  return localStorageSync({
    keys: [{ userState: ['visits'] }],
    rehydrate: true,
  })(red);
}

@NgModule({
  declarations: [],
  imports: [
    CommonModule,
    StoreModule.forFeature('userState', userStateReducer, {
      metaReducers: [sessionStorage],
    }),
    EffectsModule.forFeature([UserStateEffects]),
  ],
})

Outcome: 🙅‍♂️, the object is being loaded inside another object with the same name and, therefore, the rehydration is failing. No data is being saved (see n.3), therefore update is not happening.
image

  • Example n.3, forFeature instead of forRoot (in order to have multiple modules to achieve the same result), WITHOUT PRE-EXISTING DATA from n.1 (localstorage cleared):
same code from n.2

Outcome: 🙅‍♂️, no data is being saved, so without the proper object in the storage the rehydration and the updating is impossible
image

export function sessionStorage(red: ActionReducer<any>): ActionReducer<any> {
  return localStorageSync({
    keys: [{ userState: ['visits'] }],
    rehydrate: true,
  })(red);
}

@NgModule({
  declarations: [],
  imports: [
    CommonModule,
    StoreModule.forFeature('userState', userStateReducer, {
      initialState: {},
      metaReducers: [sessionStorage],
    }),
    EffectsModule.forFeature([UserStateEffects]),
  ],
})

Outcome: 🙅‍♂️, same as n.3: no data is being saved, so without the proper object in the storage the rehydration and the updating is impossible
image

  • Example n.5, forFeature instead of forRoot (in order to have multiple modules to achieve the same result), WITHOUT PRE-EXISTING DATA from n.1 (localstorage cleared), using simple keys instead of objects (this is a VERY bad solution, as multiple feature can use the same key and overwrite each other data)
export function sessionStorage(red: ActionReducer<any>): ActionReducer<any> {
  return localStorageSync({
    keys: ['visits'],
    rehydrate: true,
  })(red);
}

@NgModule({
  declarations: [],
  imports: [
    CommonModule,
    StoreModule.forFeature('userState', userStateReducer, {
      metaReducers: [sessionStorage],
    }),
    EffectsModule.forFeature([UserStateEffects]),
  ],
})

Outcome: 🤦‍♂️, it works, yeah... but every feature can overwrite the values of other features. Definitely not a recommended approach.
image

  • Example n.6, forFeature instead of forRoot (in order to have multiple modules to achieve the same result), WITHOUT PRE-EXISTING DATA from n.1 or n.5 (localstorage cleared), applying this "workaround" > Documentation: How to use with feature modules #82
export function sessionStorage(red: ActionReducer<any>): ActionReducer<any> {
  return localStorageSync({
    keys: ['visits'],
    rehydrate: true,
    storageKeySerializer: (key) => ```userState_${key}```,
  })(red);
}

@NgModule({
  declarations: [],
  imports: [
    CommonModule,
    StoreModule.forFeature('userState', userStateReducer, {
      metaReducers: [sessionStorage],
    }),
    EffectsModule.forFeature([UserStateEffects]),
  ],
})

Outcome: 🙄, it works... in a very, very dirty way. In order to avoid the overwriting issue, we're applying here a prefix to the key. At least this is something we can work with.
image

Conclusions:
Example n.1 works as intended, as long as you don't use the forFeature approach (https://www.pinterest.it/pin/585538389055577396/);
Example n.5 works, but it's an unsafe approach due to the risk of overwriting data;
Example n.6 works, and it's the closest to the optimal solution. Still, a dirty workaround.
Please note that in n.5 and n.6 the data is being saved as a string, not as an integer, ignoring de-facto the provided model.

@kuldeepGDI
Copy link

When is this expected to merge ? I am facing this issue on feature module

@Abhirocks889
Copy link

You could review this PR: fix: storage syncing for feature stores
and check if it suffices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature stores Functionality related to feature stores / modules known workaround There is a known workaround for this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants