Store: patchState seems to mutate the state

[akisvolanis]

Versions

* ngxs: 2.0
* @angular/core: 5.2.8

I've build an ngxs-freeze-plugin for ngxs, mostly inspired from @ngrx-store-freeze meta reducer and it seems to work fine, except when using pactState from StateContext. The way you patch the state seems to mutate the current state slice, as the internal getValue function seems to return a value by reference, while passing obj as the default reduce value.

export const getValue = (obj: any, prop: string) =>
  prop.split('.').reduce((acc: any, part: string) => acc && acc[part], obj);

and then below, inside for loop, happens the mutation

let state = getState();
const local = getValue(state, metadata.depth);
for (const k in val) {
   local[k] = val[k];
}

One possible fix could be use a new copy while returning a value from getValue function like:

export const getValue = (obj: any, prop: string) =>
  prop.split('.').reduce((acc: any, part: string) => acc && acc[part], {...obj});

or even better pass a new copy while setting up the local slice inside for loop above.

ngxs-freeze-plugin issue demo

setState from StateContext seems to work as expected.

[AlgoTrader]

Both setState and patchState do not like deep changes. Selectors use distinctUntilChanged and to make everyone happy, mutate state so that refererences for updated objects are also updated.

The distinctUntilChanged operator will only emit when the next value is unique, based on the previously emitted value. In cases of numbers and strings, this means equal numbers and strings, in the case of objects, if the object reference is the same a new object will not be emitted.

[amcdnl]

Maybe we should add some docs for this.

[plattdl]

@amcdnl I think this would be nice. I am encountering this as well and would like some guidelines on how to approach large nested objects.

My first guess would be to separate that large object into many separate sub states but that would be more work than I would like if there is a way to call setState in a way that these changes are picked up easier. Right now I just have an action that updates the object as a whole from an HTTP call with a few small changes here and there client side during the life of the session.

[eikc]

me and a few friends really don't understand if this is correct behavior that patch state mutates the object state?

I made a small example where i use patch and set state with the ngxs logger plugin and the following screenshot just seems a little bit weird to us.

action test -> uses set state and the logger looks correctly when looking at prev/next state
action test2 -> uses patch state and the logger shows prev and next are the same, which shouldn't be the case i guess ?
action test3 -> uses set state again and looks correct

here is the stackblitz for the test: https://stackblitz.com/edit/ngxs-simple-6k1fkk?file=app%2Fapp.component.ts

Is that correct behavior for patch state?

[deebloo]

@Eikc that is a bug. it should not mutate the initial state

[deebloo]

Ill add tests around this and update

[eikc]

heh, I was about to ask if you needed help and if so could point me in the right direction to fix the issue, anyway if you need any help, ping me and i will give it a try :)

Thanks for a nice library ❤️ ❤️

[AlgoTrader]

Mutating state is a very common problem

const state = getState()
state.substate.model.fieldName = 123;
setState(state);

woops - substate is not changed, model is also not changed. selectors that select on substate and model see no changes - refs are the same

The correct code is

const state = getState()
state = {...state, substate: {...state.substate, model: {...state.substate.model, fieldName: 123}}}
setState(state);

The whole path from fieldName to root state object is now updated. But who wants to write code like this? I certainly do not like writing such 'immutable' code like the example above

[akisvolanis]

@AlgoTrader Totally agree with you. Writing code like this is a big mess. My preferred way, like redux encourages you, is to normalize my state and flatten it as much as possible.
Generally i want to avoid to mutate the state directly and try to treat state as if it were immutable.

[AlgoTrader]

We will end up with brain redux/ngrx. Normalizing is the painful process. There are alternatives with mutable state: vuex and mobx.

If state is restricted to plain objects, then deep compare is not difficult. But this is not the case, state may have general objects.

The problem is quite complex. Comparing refs is very efficient but requires immutable data structures

[blacksonic]

Is it still an issue with version 3.0? We are prior to decide between NGXS and NGRX.

[SudharsanSukumar]

Wondering the same.

[anbala]

Same here.

[amcdnl]

We are discussing a better approach for this internally. It’s not a easy solution to do this for deep objects correctly and performant.

[LucasFrecia]

Any update? Been a year and 3 months. NGXS is a great library but this mutations are a strong argument against it and for NGRX.

[splincode]

@LucasFrecia use immer -adapter

https://github.com/ngxs-labs/immer-adapter

[arturovt]

@LucasFrecia yes, you've got to use some external library that provides immutability with deep objects. You could use just immer. We also have state operators since 3.4!

[markwhitfeld]

This is a very old issue that got lost at the bottom of the list. We are cleaning up the issue list now.
This is no longer an issue. It was resolved over a year ago in PR #348 but was not closed.

On this topic:
Note that setting developmentMode: true in the root module options freezes the state to prevent mutation. Both setState and patchState do not mutate the state but rather make immutable object updates. Mutating the value returned by getState will throw an error in development mode. If you mutate it anyway and turn off development mode then your app will not behave as expected. It does not make sense to add additional code in production to prevent usages that are breaking the paradigm.

If a developer prefers to make changes using a mutable approach then they can use the @ngxs-labs/immer-adapter or even the produce method from immer passed to setState as a state operator.