Feature/mergedeep rehydrate no mutation

[moniuch]

This is a PR that aims at solving long lasting bugs that users complained about in #110, #80, #65:

• destroy of initialState
• wrong code for partial (aka filtered) rehydration merge, which resulted in loss of parts of state
• mutability

The PR does not change existing dependencies (such as angular, ngrx store, etc), but rather aims at fixing the current state of things and make the library safe for our users ASAP. After the PR has been tested and approved, we can take it from there and perform upgrades as there is a couple of improvement needs, such as add storeFreeze in tests

The immutability, as I believe, has been achieved sufficiently by using spread operator, rather than by using a third-party library. It was tested on an app which has storeFreeze in place (as the last meta reducer):

[localStorageSyncReducer, storeFreeze];

In order to achieve the proper deep merge behavior, lodash.merge has been added. We can change it to immutable.js later, however there is a big footprint and some tree-shakeability complaints on their repo.

The PR adds two test cases for filtered scenarios, including a case where the state in save in separate chunks rather than in one combined piece.

Let's test it thoroughly before we merge.

[btroncone]

This lgtm from a cursory glance. Could someone more familiar with current project please take a look as well, thanks! @bufke

[moniuch]

@btroncone The main thing to decide upon is the new dependency on lodash.merge. I know that introducing dependencies might not be the optimal way, but in fact improper merging was source of the major bug. It actually solves the problem.

Feel free to edit the PR code if you have a better solution. I just wanted to come up with something that works.

On a side note: there is a couple of PRs hanging out there in this project, I bet each of them has valuable chunks of code. It would be good to review all of them and cherry-pick the best of all worlds.

This or that, let's test, and move on.

[JohnKis]

This PR looks good however there's a scenario that the solution doesn't cater for: If a key has been removed from nextState but still included in rehydratedState the merged nextState will include the deleted key. This is not ideal as you won't be able remove keys from your schema.

I had a quick look and couldn't find a way in lodash (nor immutable.js) that would allow merging to a destination object but limit the merge to existing properties. One approach could be using lodash mergeWith() with a customizer function that sets keys which don't exists on the destination to a unique value that can be filtered out later, however this doesn't scale very well for large state objects.

In my opinion, it'd be a better solution to recursively iterate over nextState and merge only existing keys. This raises another problem though which is removing deleted keys from local storage, but that's probably out of scope for this PR.

[moniuch]

@JohnKis Thanks for your valuable input. You are right that this scenario was kind of neglected and should be handled. I will try to look into solving it over the weekend.

The main purpose of this PR though is to fix the critical bugs which users suffer from. IMO it is better to proceed incrementally, ie. merge this code which I believe solves harmful bugs and then try to improve it by handling other case. Granted, the previous code might have been able to remove keys, but it had nasty bugs where the state was completely destroyed.

So the main questions are: would merging this PR be harmful to users? would it solve problems?

If we now choose to try to solve all imperfections that this code has, we will probably never move forward.

[JohnKis]

Agreed. The PR definitely improves the merging behaviour so I'm happy for it to go ahead. We just need to keep in mind that there are other areas that will need to be covered eventually.

[moniuch]

It would be of great help if you could create an issue with the specific case you have in mind.

[JohnKis]

Sure, I can create it. I'm wondering whether it's worth doing after this PR got merged, otherwise the issue won't make a lot of sense in the context of the current release.

[moniuch]

@JohnKis I believe there is never too many test cases :)

But not only that. As you said, my PR doesn't cover removing keys. Thus, after merging it, we will have a bug, so your test case will actually help solve it.

[JohnKis]

Created #115

[bufke]

I should have time to review this tomorrow and test it with my project.

[bufke]

This adds about 10kb to a production bundle ng build --prod. For comparison vuex-persist also uses lodash.merge. In their changelog they even seem to try using the smaller deepmerge, have issues, and revert back to lodash.merge. While it would be better not adding extra size, I think it's worth it.

I tested this with store-freeze and it works. Since it seems like there is high demand to make this work without regressions I'm going to merge this.

• If anyone hates the extra ~10kb they could submit a PR with something lighter and a large battery of tests to prove it's just as reliable.
• I'll close my other open PR and submit one that adds ngrx-store-freeze as a dev dependency. Add ngrx-store-freeze to testing #116
• Solving Rehydrate should do a deep merge only on existing keys #115 would be great but seems like no one is in favor of waiting to fix that before merging.

[rafa-suagu]

@bufke @btroncone when this fix will it be available ?

[btroncone]

I will publish 7.0.0 this morning, sorry for delay. 👍

[moniuch]

@btroncone @bufke

I regret to say that this PR only partially solves the problem.
The feature slices are still getting reduced down to rehydrated state (the modules don't have to be lazy loaded though).

My setup

I have a couple of feature modules feature1...featureX which are meant to be lazy loaded, but they are not just yet. So they contain the following code in their NgModule decorators:

StoreModule.forFeature('feature1', reducer)

and the reducer says:

export interface State extends fromRoot.State {
  feature1: Feature1State;
}

my localStorageSync config is:

{
keys: [{app: [...]}, {feature1: [...]}, {feature2: [...]} ],
rehydrate: true,
}

As long as don't include a featureX module, the state is rehydrated properly, but as soon as I place a feature slice in the config, the rehydrated state for that feature overrides its initialState.

Solution

For all non-app modules (slices) that you selectively save to local storage, you need to apply this solution - ie. explicitly handling UPDATE action case in the feature's reducer.

Conclusion

1. The code is bad, I don't have any other idea how to ensure that feature modules' reducers are run at least once before applying rehydrated state (from within the meta reducer code)

2. We need an example app in the repo for better testing.

3. Help needed.