btroncone / ngrx-store-localstorage

Simple syncing between @ngrx store and local storage
MIT License
613 stars 118 forks source link

ERROR TypeError: Cannot assign to read only property 'auth' of object '[object Object]' #111

Open whisher opened 5 years ago

whisher commented 5 years ago

Hello there, I've got this ugly error using rehydrate: true with "@ngrx/store": "^7.0.0",

ERROR TypeError: Cannot assign to read only property 'auth' of object '[object Object]'

export function localStorageSyncReducer(
  reducer: ActionReducer<any>
): ActionReducer<any> {
  return localStorageSync({ keys: ['auth'], rehydrate: true })(reducer);
}
bufke commented 5 years ago

Is this only with version 7? ie it works fine for you previously?

whisher commented 5 years ago

Only for version 7 in version 6, it worked fine

bufke commented 5 years ago

@btroncone I might suggest reverting version 7. Seems like it breaks things and we don't have a handle on why that is. Unfortunately it "works for me".

rafa-suagu commented 5 years ago

@whisher you are using the https://github.com/brandonroberts/ngrx-store-freeze library in the same project?

whisher commented 5 years ago

@rafa-as Yes I've got ngrx-store-freeze in my project If I get rid of ngrx-store-freeze the app works. I've always used ngrx-store-freeze in my project along with this lib with no problem so I'm wandering what's going on?

victor-ca commented 5 years ago

112

victor-ca commented 5 years ago

hello, these particular lines are the culprit for this issue; image

but to fix that i have some questions regarding behavior, @btroncone could you please help me out on these:

  1. what should happen if we have an initial state and a hydrated state with type mismatch, for example
    
    let initial= { astring: 'test'};
    let hydratedState = { astring: [] };
    // i expected: final = { astring: 'test'}
    // actually: final = { astring: []} 

// quoting from tests: // localStorage starts out in a "bad" state. This could happen if our application state schema // changes. End users may have the old schema and a software update has the new schema.


2.  what should happen if we have an initial state and a hydrated state with additional props? now they are merged, is this the expected behavior?; ex:
```ts
 let initial= { a: 'test'};
 let hydratedState = { b: [] };
// now: final = { a: 'test', b: []} 

the lines above are for simplicity, here are the tests i actually used:

 it('merge initial state and rehydrated state', () => {
        // localStorage starts out in a "bad" state. This could happen if our application state schema
        // changes. End users may have the old schema and a software update has the new schema.
        let hydratedState = {
            ...initialState.state,
            oldstring: 'foo'
        };
        delete hydratedState.astring;
        localStorage.setItem('state', JSON.stringify(hydratedState));

        // Set up reducers
        const reducer = (state = initialState, action) => state;
        const metaReducer = localStorageSync({ keys: ['state'], rehydrate: true });
        const action = { type: INIT_ACTION };

        // Resultant state should merge the oldstring state and our initual state
        const finalState = metaReducer(reducer)(initialState, action);
        expect(finalState.state.astring).toEqual(initialState.state.astring);
        expect(finalState.state.oldstring).toBe('foo');
    });

    it('regression #111: should not mutate original state', () => {
        const origObj = {};
        let s1 = {
            state: {
                aobj: origObj
            }
        };
        let hydratedState = {
            aobj: {}
        };

        localStorage.setItem('state', JSON.stringify(hydratedState));
        const reducer = (state = s1, action) => state;
        const metaReducer = localStorageSync({ keys: ['state'], rehydrate: true });
        const action = { type: INIT_ACTION };

        // Resultant state should ignore astring due to type mismatch
        const finalState = metaReducer(reducer)(s1, action);
        expect(s1.state.aobj).toBe(origObj);
    });

    it('merge initial state and rehydrated state only if types match', () => {
        // localStorage starts out in a "bad" state. This could happen if our application state schema
        // changes. End users may have the old schema and a software update has the new schema.
        let s1 = {
            state: {
                astring: 'test'
            }
        };
        let hydratedState = {
                astring: [] // some array
        };

        localStorage.setItem('state', JSON.stringify(hydratedState));

        // Set up reducers
        const reducer = (state = s1, action) => state;
        const metaReducer = localStorageSync({ keys: ['state'], rehydrate: true });
        const action = { type: INIT_ACTION };

        // Resultant state should ignore astring due to type mismatch
        const finalState = metaReducer(reducer)(s1, action);
        // expect(finalState.state.astring).toEqual(finalState.state.astring); << succeeds because initial state is mutated
        expect(finalState.state.astring).toEqual('test'); // fails
    });
rafa-suagu commented 5 years ago

@rafa-as Yes I've got ngrx-store-freeze in my project I get rid of ngrx-store-freeze and the app works sorry for that :) quite annoying though :( You can close the issue sorry for that again

@whisher the error is not on the freeze side is on this library. Check this comment: https://github.com/btroncone/ngrx-store-localstorage/pull/107#issuecomment-453474894

btroncone commented 5 years ago

Is there agreement that it would be better to revert and cut a new release? If so I can have it out tonight.

victor-ca commented 5 years ago

Hello Brian, first of all thank you for the library, it saved us a lot of time. I think it makes sense to revert it; a solution for everyone might require deep-merge;

in my project i'm using a combo of deep merge and custom serializer, and during deserialization i ditch the stored state if it doesn't match appVersion; this way one doesn't have to deal with the decision when a certain state slice should or shouldn't applied

if its acceptable i could pull something similar in this repo:

// global appVersion
interface IFooSerialized extends IFoo {
version:any;
}
export function serialize(state: IFoo): IFooSerialized {
    return {
        ...state,
        version: appVersion
    }
}
export function deserialize(storedState: IFooSerialized): IFoo {
    if (!storedState || storedState.version !== appVersion) {
        return onboardInitialState;
    }
    let state = { ...storedState };
    // avoid app state pollution
    delete state.version;
    return state
}
btroncone commented 5 years ago

@victor-ca Thanks, if there are no strong objections I’m going to revert tonight as a 7.0 release. We can explore options like above as part of a next release. Does this work for you @bufke?

bufke commented 5 years ago

Working on a fix here https://github.com/btroncone/ngrx-store-localstorage/pull/113 up to you if we should just fix or revert it. It should be a rather simple fix but I'm getting that strange error with the unit test complicating it.