Rehydratation changes all store references on UPDATE_ACTION

[Jonnyprof]

When rehydrating store in lazy load feature, changes the references of all store object and this mean that all the subscriptions are fired without changes in the store. This subscriptions are fired several times if you have a preload strategy (such as feature modules are loaded).

I've created a minimal project to show this behaviour:
https://stackblitz.com/github/jonnyprof/ngrx-localstorage-test

I've created the project with preloadingStragegy on PreloadAllModules to show that fires 2 times with 2 lazy loading feature modules, but it happens without this strategy when you load the featured module (change the route).

Steps to reproduce:
1.- Go to https://stackblitz.com/github/jonnyprof/ngrx-localstorage-test
2.- Open the console
3.- Click on "Load global data". This fills the global state and save it on localStorage.
4.- Reload the page.

You can see on console that "receiving global data" is printed 3 times:

• First one by the initial rehydration from appModule
• Second and third by every lazy loaded feature (feature1 and feature2)

I think this is a serious bug, we have a production application with lots of lazy loaded modules and in some places the subscriptions are fired 10 times! If you dispatch actions that makes http request this is crazy

I've found that if we change the line

            nextState = merge({}, nextState, rehydratedState);

with

            nextState = merge(nextState, rehydratedState);

solves the problem, but I'm not sure if this can cause other issues.

[rafa-suagu]

please @btroncone can you check this?

[rafa-suagu]

@bufke can you check this please? We can work on it but we need some response to do the PR and merge it.

[rowandh]

I've been encountering issues related to this and ngrx-router-store. In my case, it's causing a guard that returns a url tree to fail to complete a navigation due to the store emitting changes before the navigation is complete.

Setting clone: false in the deepmerge options seems to fix it, but that of course negates the reason for using deepmerge in the first place.

I think a better design would be to revert back to using object.assign, and instead allow users to define their own reducer function for how to perform the merge of the local storage with their existing state.

[BBlackwo]

Closing this issue as you can now specify a custom mergeReducer to fix this.

See my stackblitz fork of the original by @Jonnyprof.

import { ActionReducer, MetaReducer } from '@ngrx/store';
import { localStorageSync } from 'ngrx-store-localstorage';
import merge from 'lodash.merge';

const INIT_ACTION = '@ngrx/store/init';
const UPDATE_ACTION = '@ngrx/store/update-reducers';

const mergeReducer = (state: any, rehydratedState: any, action: any) => { 
  if ((action.type === INIT_ACTION || action.type === UPDATE_ACTION) && rehydratedState) {
    state = merge(state, rehydratedState); // <-- this line was changed to not clone
  }
  return state;
};

export function localStorageSyncReducer(reducer: ActionReducer<any>): ActionReducer<any> {
  return localStorageSync({
    keys: ['global'],
    rehydrate: true,
    mergeReducer // <-- use in the config here
  })(reducer);
}

export const metaReducers: Array<MetaReducer<any, any>> = [localStorageSyncReducer];

[Jonnyprof]

@BBlackwo Thanks, it's great to override mergeReducer to workaround this issue, but I'm still thinking that the defaultMergeReducer is wrong, because changes the main functionality of ngrx.
I'd forked this repository and tested it for a while, and I think this should fix the issue.

export const defaultMergeReducer = (state: any, rehydratedState: any, action: any) => {
  const overwriteMerge = (destinationArray, sourceArray, options) => sourceArray;
  const options: deepmerge.Options = {
    arrayMerge: overwriteMerge
  };
  if (action.type === INIT_ACTION && rehydratedState) {
    state = deepmerge(state, rehydratedState, options);
  } else if (action.type === UPDATE_ACTION && rehydratedState) {
    Object.keys(rehydratedState).forEach(partialState => {
      state[partialState] = deepmerge(state[partialState] || {}, rehydratedState[partialState], options);
    });
  }

  return state;
};

Other option is use the merge from three shakeable lodash, for example lodash-es, but not sure if it does the correct merge array for you.

I can do a PR if you see it correct.

[bufke]

Just for context, we did originally use 'lodash.merge' but it wasn't getting updates (at least at the time) and people where opening security vulnerabilities about it that couldn't be fixed. deepmerge fixed that problem and is lighter weight. However it doesn't have the exact behaviour of lodash.merge. See #126

Both ideas where borrowed from vuex-persist which has to solve the same exact problem.

I looked into three shakeable lodash a long time ago and it seemed not feasible for the average human to get working. I don't know the current status.

My opinion would be to only consider switching the default if there was a very strong case for it. Going from lodash > deepmerge> lodash introduces breaking changes each time.

[Jonnyprof]

Yes, I know the historial lodash - deepmerge path, because of it I've used deepmerge in my solution.
We have a large application with lazy load modules depending on the page and connection, and if every time that we load an async module all the async pipes to the store are emitted, the loose of performance is huge due to changeDetection is fired for all the components.

[BBlackwo]

Hi @Jonnyprof thanks for the info and suggested code change.

If you're able to open a PR with your suggested code change as well as some unit tests I can take a look, but can't promise I'll merge it. It will need a lot of testing to see how it relates to other issues raised.

Some related issues:

• Issue rehydratedState is not recalculated on UPDATE_ACTION #120 and PR update-reducers fix #122 suggests removing the || action.type === UPDATE_ACTION check completely and only merge state on the INIT_ACTION.
• Rehydrating lazy loaded feaure modules Rehydrating lazy loaded feaure modules #133
• Bug with Lazy Loaded feature modules Bug with Lazy Loaded feature modules #93 (has a really long thread!)

[bappy004]

@BBlackwo looks like your comment in PR #122 (comment) is a good workaround.