Bug with Lazy Loaded feature modules

[bmalinconico]

Hello,

The following PR introduced a bug related to lazy loaded feature modules: https://github.com/btroncone/ngrx-store-localstorage/pull/76/files

The net effect of this PR is to re-hydrate the state when every feature module is loaded, and this breaks sections of the state which are selectively serialized (localStorageSync({keys: [{todos: ['name', 'status'] }, ... ]}))).

The result is the state being rehydrated and deleting all other keys not in local storage.

[philjones88]

I’ve just hit this too

[bmalinconico]

@philjones88 Possible solution that works for my usage: https://github.com/btroncone/ngrx-store-localstorage/pull/94

[btroncone]

@bmalinconico Do you want to take the lead on this? I can make you a collaborator to this project. I am not able to put much time into this right now and do not want to be a bottleneck.

[bmalinconico]

@btroncone Sure, I think the biggest risk of my PR is the kinda "stupid" way it determines the state key name const name = Object.keys(key)[0]; What do you want me to do beyond deal with bugs I create myself? :)

[btroncone]

@bmalinconico Really looking for another extra set of eyes that is using this library every day. Need help determining priority and impact of some of the proposed PR's / issues.

[tanyagray]

I pulled it down but had problems getting the tests running. I'm also hesitant to merge anything that's untested cos I'm not a "regular" user as such. I do use feature modules though so maybe @bmalinconico we could collab on this one? Happy to live chat elsewhere.

[btroncone]

@tanyagray Agree, definitely want to be careful here. Thanks for taking a look! 👍

[bmalinconico]

@tanyagray odd, tests ran fine for me. Was it a suite error or a test failure?

[bmalinconico]

@tanyagray I'd be happy to collab on this if we can find time. I'm on the east coast of the USA and am really only on during work hours.

[magnattic]

Is there a workaround or older version to revert to? I just introduced lazy loaded modules to my project and this resets my state when loading the module.

[bmalinconico]

@magnattic My PR (https://github.com/btroncone/ngrx-store-localstorage/pull/94) works for my use-case and I'm using it in production.

In your package.json

    "ngrx-store-localstorage": "https://github.com/bmalinconico/ngrx-store-localstorage.git#fix_partial_rehydration_of_keys_dist",

However I started by reverting to 0.3.0 and that worked for me too but had its own issues relate to feature modules.

[magnattic]

Thanks for providing your PR. Unfortunately I can't get it to work with my setup: I get an

ERROR TypeError: keys.map is not a function

in mergePartialStates, this is the line:

keys.map(function (key) { return newValues_1[key] = rehydratedState[name_2][key]; });

The problem seems to be that it's not compatible with the custom serialize/deserialize functions I am using. This is my setup:

export function localStorageSyncReducer(reducer: ActionReducer<any>): ActionReducer<any> {
  return localStorageSync({
    keys: [
      {
        shop: {
          deserialize: (json: any) => {
            const jsonFixed = fixInvalidCombinations(fixOldState(json));
            return _.defaultsDeep(jsonFixed, { homePage: fromHomepage.initialState, prospect: fromProspect.initialState });
          },
          serialize: (state: ShopState) => {
            const { numCardsVisible, ...serializedHomePage } = state.homePage;
            return { homePage: serializedHomePage, prospect: state.prospect };
          }
        }
      },
      {
        checkout: {
          deserialize: (json: any) => _.defaultsDeep(json, { customer: fromCustomer.initialState })
        }
      }
    ],
    rehydrate: true
  })(reducer);
}

[bmalinconico]

@magnattic I see what is wrong, and can update the PR. It looks like you serializing the entire shop and checkout state?

If you are I'll make a small adjustment, but if you are serializing the whole state you are probably running into a different problem then I was trying to solve.

[bmalinconico]

@magnattic updated my branch, give that a try. PR hasn't updated yet.

[magnattic]

Thanks, this removes the error, but still overwrites my State when the feature module is loaded.

This is my scenario:

• I have 2 keys in my state: shop and checkout
• shop has 3 subkeys: homePage, prospect and offerData
• checkout is part of my feature module and is being lazy loaded.
• I use custom serializers/deserializers for shop and checkout (as shown above)
• I don't want to store shop.offerData in localstorage, as this is loaded on the fly.

What happens?

When I load my feature module for checkout, the whole state for shop.offerData is reset to its default of null.

Basically what I want is for localStorageSync to leave shop.offerData alone and neither sync nor overwrite it.

Do I have to setup my sync differently?

[bmalinconico]

@magnattic So the problem is how do we determine when to partially overwrite the state from localstorage, and when do we perform a merge.

The current rules are:

• If rehydration key is a string or object (your case), we replace the state with local storage
• If rehydration key is an array, we only replace the strings in that array.

If it is a object and custom serializers/deserializers are used, how do we determine if we should perform a merge or a replace.

I can provide an update to get you moving if 0.3.0 isn't going to work for you, but this behavior determination is something I should defer to an owner or collaborator. I'll update my solution with their chosen solution.

We could always add a parameter to the object to indicate the rehydrated state, when provided an object, should be merged.

[magnattic]

@bmalinconico I realized I misrepresented my scenario, I updated my comment above.

Sorry if I am missing something, but why can't we simply do something like this?

if ((action.type === INIT_ACTION || action.type === UPDATE_ACTION) && rehydratedState) {
      state = _.defaultsDeep({}, rehydratedState, state);

Use the rehydratedState where possible and if a property is missing somewhere, add it from the existing state. This is working recursively. (I am using lodash here, but I guess the function could be replicated to remove the dependency.)

I am not sure why we need the stateKeys at all at this point. Aren't they only needed to get the rehydratedState?

[bmalinconico]

I feel like, if you are rehydrating the entire section of a state an undefined value coming from localstorage should be respected. We should only do a deep merge when you are selectively rehydrating keys.

We should also be careful to ensure we are only duping one level deep, unless a true deep merge is what we are after.

Either way these are behavior choices I'll leave up to owners.

[bmalinconico]

@magnattic ah I didn't see the filter property on that object, perhaps that could be used to control the behavior.

[magnattic]

I am still massively confused by the whole thing. Why are we even touching the whole state when a feature module is loaded? The 'update-reducers' action has a property feature where it states which feature is being loaded, so why are we not limiting our rehydration to that slice of state? Why would we be overwriting or merging anything from localstorage a that point that is not part of the feature slice?

[voidek-work]

In your package.json

"ngrx-store-localstorage": "https://github.com/bmalinconico/ngrx-store-localstorage.git#fix_partial_rehydration_of_keys_dist",

However I started by reverting to 0.3.0 and that worked for me too but had its own issues relate to feature modules.

Hi, I tried to put this PR when I noticed that my lazy loadable modules get the default state instead of the saved one. Everything worked as it should, but as soon as I connected one more module everything broke. Sorry my English :)

[voidek-work]

The problem was solved when I specified a synchronous module in an array of keys localStorageSync({ keys: ['authContainer', 'layout', 'admin'], removeOnUndefined: true, rehydrate: true })(reducer); layout is not lazy

[bmalinconico]

@magnattic So the reason inevitable is how NGRX gets a feature module bootstrapped. See this line: https://github.com/ngrx/platform/blob/cb473c00475289509f0924321bb293d0ad1137ca/modules/store/src/reducer_manager.ts#L73

When a new feature module is bootstrapped, it isn't bootstraping with an initial state of what is currently in the state, it bootstraps with an initial state of what is provided via the INITIAL_STATE injectable. As such, when NGRX performs this bootstrapping, this lib comes along a little later and rebuilds it, or merges it.

As for using the feature key to selectivity refresh the data. At first swag that seems like an option, I'm mostly extending and correcting existing functionality and this seemed like the smallest touch to correct the issue.

@voidek-work I'm glad that fixes it for you, but what was the localStorageSync config that wasn't working? That is the only way I can fix it :D

[voidek-work]

@bmalinconico If I get you right... this working:

export function localStorageSyncReducer(
  reducer: ActionReducer<any>,
): ActionReducer<any> {
  return localStorageSync({ keys: ['authContainer', 'layout', 'admin'], removeOnUndefined: true, rehydrate: true })(reducer);
}

not working:

export function localStorageSyncReducer(
  reducer: ActionReducer<any>,
): ActionReducer<any> {
  return localStorageSync({ keys: ['authContainer', 'admin'], removeOnUndefined: true, rehydrate: true })(reducer);
}

layout - state not lazy feature module. authContainer, admin - state lazy loading feature module. for example auth module.

@NgModule({
  imports: [
    ReactiveFormsModule,
    FormsModule,
    MaterialModule,
    CommonModule,
    AuthRoutingModule,
    TextMaskModule,
  ],
  declarations: COMPONENTS,
  exports: COMPONENTS,
})
export class AuthModule {
  static forRoot(): ModuleWithProviders {
    return {
      ngModule: RootAuthModule,
    };
  }
}

@NgModule({
  imports: [
    AuthModule,
    StoreModule.forFeature('authContainer', reducersAuth),
    EffectsModule.forFeature([AuthEffects, VerifyEffects, IdentityEffects, BanksEffects, StepEffects]),
  ],
})
export class RootAuthModule {}

if I rehydrate only lazy modules, then state reset to default.

[bmalinconico]

@voidek-work I don't see any reason in my patch for why the lack of the 'layout' key itself is special.

[voidek-work]

@bmalinconico Unfortunately, I'm just getting acquainted with the technology, so I'm not sure exactly what the problem is. But the essence I described. In any case, thanks for the fixes

[ilennert]

Basically this renders this mechanism for restoring my state, useless... I don't understand what is happening but I know this is the cause. If I remove the persistence my problem goes away, unfortunately, so does my persisted state... Is anyone planing on fixing this problem?

[bmalinconico]

@ilennert I have been using my fork in production for a few months now and it is working fine. Please give it a try and report back any issues you encounter and I'll make an effort to fix them.

The collaborators are not really responding to this PR...

[pedrofurtado]

Is there any workaround for this scenario?

[bmalinconico]

@pedrofurtado Please see my fork and report back issues with it.