WIP: Fix for state mutation when rehydrating state

[bufke]

What does this solve?

Follow up for #107

We need to not mutate state when merging localstorage hydrated state and new initial state. This is bad in principle and breaks outright for users of ngrx-store-freeze.

How

• Add ngrx-store-freeze meta reducer to test
• Do not mutate state in the localStorage and initial state merge.

Open issues/questions

• We need more tests
• Should we use a "deep merge" function like from lodash?
• Do we need further improvements to the merge function even if not using lodash?

[bufke]

I wanted to add ngrx-store-freeze as a test to prove it fails without this change and passes with it. However I get this rather unexpected error.

node_modules/@ngrx/store/src/store_module.d.ts:1:42 - error TS2305: Module '"ngrx-store-localstorage/node_modules/@angular/core/index"' has no exported member 'InjectionToken'.

1 import { ModuleWithProviders, OnDestroy, InjectionToken, Injector } from '@angular/core';

[bufke]

Testing this in my own project, it appears ineffective. I still get the "Cannot assign to read only property 'x' of object '[object Object]'" error. So we need

• A better fix
• A better way to test than running it on my own project.

[moniuch]

I actaully tried to add another test for filtered (selective) states, based on my specific use case, but I didn't really know how to reliably test it. By reliably I mean - mimicking a real ngrx app behavior: rehydrate, then run a reducer along with this metareducer.

This test case for filtered states is different from the one already included, in that user designates incomplete feature slices, and in that the cherry-picked subslices (feature1 and feature2) reside as separate items in local storage, ie. localStorage is their common ancestor.

it('filtered - multiple keys at root - should properly revive partial state', function () {
        const s = new MockStorage();
        const skr = mockStorageKeySerializer;

        // state at any given moment, subject to sync selectively
        const nestedState = {
            app: { app1: true, app2: [1, 2], app3: { any: 'thing' } },
            feature1: { slice11: true, slice12: [1, 2], slice13: { any: 'thing' } },
            feature2: { slice21: true, slice22: [1, 2], slice23: { any: 'thing' } },
        };
        syncStateUpdate(nestedState, [
            { 'feature1': ['slice11', 'slice12'] },
            { 'feature2': ['slice21', 'slice22'] },
        ], s, skr, false);

        const raw1 = s.getItem('feature1');
        expect(raw1).toEqual(jasmine.arrayContaining(['slice11', 'slice12']));

        const raw2 = s.getItem('feature2');
        expect(raw2).toEqual(jasmine.arrayContaining(['slice21', 'slice22']));

        const rehydratedState: any = rehydrateApplicationState(['feature1', 'feature2'], s, skr, true);
        expect(rehydratedState).toEqual(jasmine.objectContaining({
            feature1: { slice11: true, slice12: [1, 2] },
            feature2: { slice21: true, slice22: [1, 2] },
        }));

        // initial state as declared in reducers
        const initialState = {
            app: { app1: false, app2: [], app3: {} },
            feature1: { slice11: false, slice12: [], slice13: {} },
            feature2: { slice21: false, slice22: [], slice23: {} },
        };

        const metaReducer: any = localStorageSync({
            keys: [
                { 'feature1': ['slice11', 'slice12'] },
                { 'feature2': ['slice21', 'slice22'] },
            ],
            rehydrate: true,
            storage: new MockStorage(),
        })((state, action) => ({ ...state }));

        // how do we test it?...
        expect(metaReducer(initialState, { type: 'actiontype' })).toEqual({
             app: { app1: false, app2: [], app3: {} },
             feature1: { slice11: true, slice12: [1, 2], slice13: {} },
             feature2: { slice21: true, slice22: [1, 2], slice23: {} },
         });
    });

[moniuch]

With regards to immutability, I think it is the returned meta reducer that needs it, I have been trying something like:

return function(state: any, action: any) {
    let nextState;

    /*
         Handle case where state is rehydrated AND initial state is supplied.
         Any additional state supplied will override rehydrated state for the given key.
         */

    if (action.type === INIT_ACTION && !state) {
      nextState = reducer({...state}, action);
    } else {
      nextState = {...state};
    }

    if (
        (action.type === INIT_ACTION || action.type === UPDATE_ACTION) &&
        rehydratedState
    ) {
      nextState = {...nextState, ...rehydratedState};
    }
    nextState = reducer({...nextState}, action);

    syncStateUpdate(
        nextState,
        stateKeys,
        config.storage,
        config.storageKeySerializer,
        config.removeOnUndefined,
        config.syncCondition
    );

    return nextState;
  };

It might be over-using spread operator for now (not sure nextState really needs it as it is an internal variable).

[bufke]

Yeah the problem is doing this state[key] = {...state, whatever}. It's still assigning a value to a read only object. I was reading this - lodash would probably solve it. The alternatives mentioned are pretty funny - like JSON.parse(JSON.stringify

It should be really easy to add the freeze meta reducer to the test but I get that store_module.d.ts error. Feels like it must be something really silly.

[moniuch]

@bufke, this assignment state = will always violate immutability. That is why in my snippet above I am using another variable, cloned state, namely nextState. State must not be re-assigned and lodash is no remedy for that.

[victor-ca]

@moniuch isn't it that only state[key] = value violates immutability? state = value shouldn't, since we get a new state, not mutating the existing;

i have experimented and have a working branch tested with store-freeze and local tests;

however whats troubling is that rehydration doesn't deep-merge and if it would reconciliation is troublesome; ex:

const initial = { a: { b: { Foo: {}, C: [] } } }
const hydrated = { a: { b: { Bar: {}, C: {} } } }
const next = { a: { b: { /* Foo or Bar or Both: {}? also how we deal with different types of C */ } } }

[moniuch]

@victor-ca yes, you are right, I overlooked that. state[key] is also a violation. On a frozen object it is impossible to execute.

As per deep merge, I totally agree - and that is why in #107 I commented on the code using lodash merge as a quick tool for experiment purposes.

Basically I can identify the following problems we have currently and they get mixed around in our discussions:

1. immutability violations
2. overriding instead of deep merging. My concern is that we provide rehydratedState (which is incomplete when state is undefined) as a fallback when state is undefined.
3. no good unit test pattern
4. no testing app for integration tests (another repo) with lazy loaded modules and store-freeze in place
5. too few test cases

[victor-ca]

@moniuch yep, yep
1, 3 and 4 should be doable with immutable.js

regarding 2: maybe we should leave the issue to library users :) (it can be tackled with custom deserializers);

[moniuch]

How can 2 be solved if we effectively distort the state, so to speak?

[bufke]

Updating other packages fixes the freeze issue I had before. Check out the only slightly updated test - it reproduces the mutation issue.

Not that I'm against other changes to the tests :)

I'd be fine with some incrementalism here. If we fix the specific mutation issue without adding an external package that should solve the original merge state issue and the new freeze issue. I think it's reasonable to expect custom deserializers at some point, but basic usage should "just work".

[bufke]

This fixes the unit test

const slice = Object.assign({}, state[key], rehydratedState[key]);
state = {...state, key: slice};

We need more test cases though. I need to head out for a bit, should be around again in maybe 5 hours or so.

[moniuch]

@btroncone @bufke @victor-ca

I had to fix the code to make it work at least with my setup so I cloned the original function and worked on the code. As I am not able to come up with a proper PR right now, I am sending you just a small portion (just the returned function) to let you play with it, perhaps run the test.

My code basically:

• removes fallback state = rehydratedState in the signature, as it makes no sense, and is contradictory to subsequent ifs checking for falsy state, which then obviously never pass through.
• makes sure we run the reducer when the state arrives undefined, so to get the complete state
• uses spread operator to avoid mutability

Note: as I use lodash in my project, you will find merge from that library to achieve deep merge. It could be replaced with mergeDeep from immutable.js if the project decides so. I think this kind of dependency is a legit one to have - useful and well tested.

Please play with this code, see if you see any problems. If the comments are positive. I will come up with PR later in the evening European time.

index.ts

import * as merge from 'lodash/merge';

// ...unchanged code - snipped for brevity...

  return function (state, action: any) {
    let nextState;

    // If state arrives undefined, we need to let it through the supplied reducer
    // in order to get a complete state as defined by user
    if ((action.type === INIT_ACTION) && !state) {
      nextState = reducer(state, action);
    } else {
      nextState = { ...state };
    }

    if ((action.type === INIT_ACTION || action.type === UPDATE_ACTION) && rehydratedState) {
      nextState = merge({}, nextState, rehydratedState);
    }

    nextState = reducer(nextState, action);

    if (action.type !== INIT_ACTION) {
      syncStateUpdate(
        nextState,
        stateKeys,
        config.storage,
        config.storageKeySerializer,
        config.removeOnUndefined,
        config.syncCondition,
      );
    }

    return nextState;
  };

My usage, just in case:

export function localStorageSyncReducer(reducer: ActionReducer<any>): ActionReducer<any> {
  return myLocalStorageSync({
    keys: [{ 'app': ['language'] }, { 'cart': ['tokens'] }],
    rehydrate: true,
  })(reducer);
}

[bufke]

Closing in favor of #114