Reviving nested states from localstorage results in loss of part of nested initial state values

[Matmo10]

Suppose I have this initial state:

{ 
   someKey: {}
   itemResults: {
          items: [], 
          totalItems: 0,
          isListView: true
    },
   someOtherKey: {}
}

and I have this config:

localStorageSync({
    keys: [{itemResults: ['isListView']}],
    rehydrate: true
})

and I have this state being revived from localStorage:

{ 
   itemResults: {
       isListView: false
   }
}

Which means during @ngrx/store/init, since we only do a shallow merge of keys using Object.assign, I end up with the following state:

{ 
   itemResults: {
       isListView: false
   }
}

So basically if you use a nested key or keys, then that slice of state overrides the whole slice of initialState for the top level key. I was thinking it'd be nice if I could just use lodash's merge to merge (or basically Object.assign nested keys also), but I don't want to bloat this library either. It would be nice if I could supply a function to the library to use to do the merge, then I could just use lodash.merge there instead of bundling it with the library.

Could we possibly add a config option to allow the user to provide a custom merge function? Basically something like this?

export const localStorageSync = (config: LocalStorageConfig) => (reducer: any) => {
    // ... code omitted

    const mergeFunction = config.customMergeFunction || ((state, rehydratedState) =>  Object.assign({}, state, rehydratedState));

    return function (state = rehydratedState, action: any) {
        
        if (action.type === INIT_ACTION && rehydratedState) {
            state = mergeFunction(state, rehydratedState);
        }
     
        // ... code omitted
    };
};

Any thoughts/objections, anyone?

---

EDIT:
Renaming the thread since it appears that there's a general consensus that a deep merge should be the default behavior here.

[ernestomancebo]

I get the idea, but where is the default state being placed? inside the customMergeFunction?

[Matmo10]

@ernestomancebo I'm not sure I understand your question

In my case, my merge function would basically do this:

config.customMergeFunction = function(state, rehydratedState) {
   return _.merge({}, state, rehydratedState);
}

but I don't think that was your question.

[Matmo10]

I think this is similar to, or the same as, issue #15

[Matmo10]

Thoughts, @btroncone ?

[adamkleingit]

I don't really understand how this library is usable without fixing this.
If I want to sync only part of the state, it makes sense that the rest of the initial state will not be changed by default.
Unless I'm missing something

[jondea]

I agree with @adamkleingit, merge (however it is implemented) is the behaviour that makes the most sense and should probably be the default.

For anyone looking for a quick fix, export your initialState and use a custom de/serializer to inject it. Here's an example where we want to save the user inside state.auth:

function authDeserializer(storedState:any) : fromAuth.State{
  let authState = {...fromAuth.initialState}
  if(storedState && storedState.user) authState.user = storedState.user;
  else                                authState.user = {...fromAuth.emptyUser};
  return authState
}

function authSerializer(state:fromAuth.State) : any{
  return {user: state.user}
}

export function localStorageSyncReducer(reducer: ActionReducer<any>): ActionReducer<any> {
  return localStorageSync({
    keys: [{auth:{serialize: authSerializer, deserialize: authDeserializer}}], 
    rehydrate: true
  })(reducer);
}

[tanyagray]

Thanks everyone for your input, I'm reviewing existing issues and PRs in my available time and see this as a high priority. Have tagged the issue for attention.

At this point I understand that the PR #15 is not specific enough, and the ideal solution would be an option to provide custom serialize and deserialize functions?

Would appreciate people's (more recent) thoughts on this as the issue has been quiet for a while.

[MattMorrisDev]

Still waiting on this, personally. Haven't bothered to upgrade one my projects to ngrx 4+ because of this issue. The custom merge function would allow us to fix it our own code, but it seems like it should just be baked into ngrx-store-localstorage itself.

[jondea]

@tanyagray the library already allows for custom serialize and deserialize functions. That's just the non-ideal stopgap solution I suggested to solve the problem.

The primary problem is that the library currently does not have the expected behaviour of doing a full, deep merge between initialState and rehydratedState. This should be the default behaviour. Whether or not the library allows for custom merge functions I think is extra.

[pedep]

@jondea Do you think simply adding deepmerge would be enough, or would you prefer another implementation?

       (action.type === INIT_ACTION || action.type === UPDATE_ACTION) &&
       rehydratedState
     ) {
-      state = Object.assign({}, state, rehydratedState);
+      state = deepmerge.all([state, rehydratedState]);
     }
     const nextState = reducer(state, action);
     syncStateUpdate(

[jondea]

It is probably enough for most people (me included), and it is a very sensible and natural feature.

Whether other people also want the added functionality to provide such a custom function, I don't know.

[magnattic]

Please add deep merging so you keep default properties that are missing from the localstorage. This is a common scenario for us where we add new properties to our state but recurring visitors still have an old state in their localstorage.

[Matmo10]

Yeah, just to clarify, I don't really care about the custom merging function itself. I just want the deep merge to be the default behavior. The custom merge function was just my way to accomplish that. Almost wondering if I should change the thread title at this point :)

[hhu94]

I was happy to find ngrx-store-localstorage today as it's exactly what I need, but I'm confused about why deep merge is not the default behavior. Took some head scratching before I came to github issues to find this issue filed.

Is there a reason why the change @pedep suggested hasn't been implemented yet? I can submit a PR if necessary.

[bufke]

#15 solves the problem for me - issues where I change my state tree shape and reviving it creates "impossible" states that lead to unpredictable behavior. I will continue testing it in my own app.

IMO we should add tests to prove effectiveness to the existing PR and merge it. I'd be happy to help add a test if this is the desired way we want to go. deepmerge would work too but I'd want to hear a pretty good reason why the project should add a npm dependency if we can get the same thing done with just a few lines of JS. I don't feel strongly about this. If someone added a test that deepmerge handled well and PR 15 didn't I'd be easily convinced.

On custom merging functions - I think that should be treated separately. We could merge this in first, close this issue and #80 and continue talking if we need additional customization options.

@tanyagray @btroncone let me know if you'd like me to do any chores like adding tests or comparing the PR with deepmerge. I'm happy to help.

[pedep]

Sorry its been a while - Actually thought i pushed a PR back when i made my original comments.

I actually just ended up hacking in a merge in the beginning of my reducers, so i kinda forgot about the issue

import { UPDATE, Action } from '@ngrx/store';

export class UpdateAction implements Action {
  readonly type = UPDATE;
}
export function reducer(state = initialState, action: auth.Actions|UpdateAction): State {
  if (action.type === UPDATE ) { state = {...initialState, ...state}; }
  // [... reducer things happening below ...]

I put up the snippet from earlier in the thread for review here #100

[sainture]

@pedep the above solution fixed my issue but still waiting for the library to have deep merge setup by default.

[vvolodin]

What is preventing this issue from being fixed for more than a year? Any help needed?

[bufke]

Does this fix solve this problem? #107

It feels to me there are many semi related issues about this.

[moniuch]

To those subscribed to this issue (myself very interested in it)
Deep merge should IMO definitely be a default behavior. As to why the merge does not honor the default state but instead overwites it with the local partial, I made a quick experiment and left a comment here.

#107 (comment)

[BBlackwo]

Closing this issue as it was fixed with #107.

For anyone interested, there's now a mergeReducer config if the default deep merge does not work in your use case.