Try replacing lodash with deepmerge

[bufke]

Alternative fix for #123

This change gets the project on a better maintained package. lodash.merge is no longer maintained and suffers from a potential security vulnerability. lodash itself would increase the bundle size to an unacceptable level. Switching to deepmerge actually reduces the webpack bundle size by about 10k compared to lodash.merge.

Previously it was decided to avoid deepmerge due to observed issues when used with the project vuex-persist. This seems only to be unwanted array concatenation instead of replacement. It's easy to avoid this using options set to deepmerge, which this PR demonstrates.

Still this is a significant change and we cannot guarantee the behaviour will exactly match lodash.merge. If we decide to go with deepmerge, I recommend releasing a beta release and asking users to try this out before declaring it stable.

[btroncone]

This seems like a fair option to me. If others agree, I would be ok with cutting a beta release to test it out. 👍

[KlapTrap]

@btroncone We'll help out testing it whenever you're ready.

[btroncone]

@bufke Is this ready for beta?

[bufke]

Yes ready for beta. Hopefully we can get more feedback before releasing as stable.

[bufke]

@btroncone we have 3 👍 and no comments about any issues. Are we as ready as we'll ever be? This DEF merits a major version bump as the chance for very subtle differences is high. I may look into the Angular Universal pull request today too if we do a release any time soon.

[btroncone]

Cool, I’ll merge this in tonight and bump to v8.0. Thanks! 👍

[btroncone]

Merging 👍

[StephenCooper]

The change to deepmerge means that objects in the state tree loose their function definitions. I have confirmed this behavior difference here .

export class MyClass {
  constructor(public a: string){}
  public getValue(){
    return this.a;
  }
}

export class State {
   toHydrate: MyClass;
}

Lodash Merge

var loState = {} as State;
loState = merge({}, loState, {toHydrate: new MyClass("Lodash merge")});
loState.toHydrate.getValue(); 
// returns Lodash merge

Deepmerge

var deepState = {} as State;
deepState = deepmerge(deepState, { toHydrate:new MyClass("Deep merge")});
deepState.toHydrate.getValue(); 
//Error: deepState.toHydrate.getValue is not a function

This breaks our code as an object in the initial state tree no longer has its methods after a re-hydration of a different property.

I can work around it by re-creating the objects myself on the @ngrx/store/update-reducers action within my own reducer.

case UPDATE: {
      return {
        ...state,
        toHydrate: new MyClass(state.toHydrate.a)
      }

I may also look at using a selector that creates the object and then just store a plain old javascript object in the store which would avoid this issue with deepmerge.

export const getClass = createSelector(
    getFeatureState,
    state => new MyClass(state.a)
);

Edit:
From: Redux FAQ

It is highly recommended that you only put plain serializable objects, arrays, and primitives into your store. It's technically possible to insert non-serializable items into the store, but doing so can break the ability to persist and rehydrate the contents of a store, as well as interfere with time-travel debugging.

I will be updating our code to follow this advice and so deepmerge behavior is acceptable. :)

[bufke]

This might be worth documenting. We could retroactively update the changlog about a known breaking change. @StephenCooper feel free to open a Pull Request clarifying the behavior if you have time.

[bufke]

Added findings to changelog