Skip to content
This repository has been archived by the owner on Jan 10, 2018. It is now read-only.

How strict is the requirement for immutability? #290

Closed
LMFinney opened this issue Dec 8, 2016 · 8 comments
Closed

How strict is the requirement for immutability? #290

LMFinney opened this issue Dec 8, 2016 · 8 comments

Comments

@LMFinney
Copy link

LMFinney commented Dec 8, 2016

I've just started converting one of our apps to use ngrx/store, and I'm hung up a bit on how strict to be with having an immutable store. Does that requirement mean that everything that is in the store must be strictly and deeply immutable? Or just that I shouldn't modify anything that's in the store in a way that would have the side effect of modifying the store? Or somewhere in between?

For example, when looking at one of the reducers in the example app, one of the values in the State is entities, which is, essentially, a Dictionary<Book>. I know that none of the selectors return entities directly, but getSelected returns a Book from entities without any sort of defensive copying, even though Book itself isn't immutable.

So, couldn't a user grab the selected Book and modify values within the store as a side effect? If so, how is immutability being maintained? I know the example app uses storeFreeze to ensure no problems during development, but this seems to be a possible problem.

In my code, I have an interface that's something like interface Foo {index: number; name: string;}, and I want to put an array or list of them in the store. My initial instinct was to just put my Foo[] directly in the store, and then I thought that I should have the store actually keep Immutable.List<Foo> (converting to and from List in the reducer and selector). But then I realized that Immutable.List is a shallow copy. Should I take further steps to make my store deeply immutable, even though the example app doesn't do that?

Am I overthinking this?

Thanks

@kylecordes
Copy link
Contributor

kylecordes commented Dec 8, 2016

Over here we have been a long way down this road. The eventual answer is that you will have endless debugging pain if you allow code that mutates something in the store. Certainly a freeze during development to try to prevent writing code that does this is a good idea. If any application code get something from the store and mutates it, or if reducers sometimes mutate data, obtaining predictable easily understood behavior from the rest of the system downstream from their becomes at least as difficult, possibly more so, than before you started using Store at all.

So I don't know about overthinking per se, but the point can hardly be made strongly enough: don't mutate data that's in the store.

(By the way, TypeScript 2.1 just shipped, and it brings the object spread operator, which saves considerable Objects.assign() syntax tedium when implementing code in reducers and elsewhere to avoid in-place mutation!)

(The more I work with TS the more I'm reminded that sometime in the future it is likely will be writing code in languages that have immutability built in. I've done some work and some of those, and it is much nicer to have the language protecting you from this versus coding standards, libraries, etc.)

@sledorze
Copy link

sledorze commented Dec 8, 2016

One may also use a Lense library to help with that..

@stochris
Copy link

stochris commented Dec 15, 2016

How useful would it be for the ngrx/store itself to deep freeze your state in development mode ?

On paper, that sound awesome, because you can avoid certain scenarios, like accidentally modifying the store properties, without adding any complexity to your application ( same models, just freeze them )

@fxck
Copy link

fxck commented Dec 15, 2016

@MikeRyanDev
Copy link
Member

Freezing state in development with something like ngrx-store-freeze is the recommended path to take if you want to guarantee there are no unnecessary state mutations.

@kylecordes
Copy link
Contributor

@fxck Ideally "freeze in dev mode" would ship in the box, on by default - rather than being a separate add-on to discover. But it's a tradeoff of course, vs keeping the core as small as possible.

@ericmarcos
Copy link

Hi @kylecordes, what if I have a very deep structure in my store. Something like "organization": { "projects": [{ "cases": [{ "messages": [{...}] }] }] } and I have an action that just changes a property of a message. Do I have to deepcopy everything just for that? Or maybe I'm structuring my state in a wrong way?

@michahell
Copy link

michahell commented Aug 3, 2017

@ericmarcos very good question. I'm running into a similar question, however an even easier use-case: I store a list of things, say, Things[], and when one Thing has a property change I emit an Action that leads a reducer to store an updated list of Thing[]:

Should I only clone the Thing that changed and update the Thing[] list with that, or should I clone the full Thing[] list ? I'm currently returning state.map where state instanceOf Thing[] because Array.map returns a new list. It is unclear to me whether this is completely wrong, if there is a better way or if this is the way I should have done it.

[ edit ] I now see here that subreducers are the way to go: https://stackoverflow.com/questions/41217604/ngrx-update-single-item-in-a-list-of-items
still, the below ngFor tirade applies 😃

This question even extends to the way Angular's ngFor change detection works (and using a trackBy function complicates that even further!): when I clone every item in Thing[] and have my reducer return a new list of cloned Things, Angular will think it's a brand new list (which it technically is) and run change-detection for all items in the list: They will also be brand new, and as such, old list items get removed and new ones get added to the DOM.

Suppose you have a ThingComponent for each Thing in the ngFor list.
In that component, ngOnChanges will fire. But here's the thing: the SimpleChanges passed to ngOnChanges will never contain previousValues, because the whole list got replaced, and so there is previous value: everything is brand new, from Angulars perspective.

The only way to combat this as far as I'm researching now, is to use a custom trackBy function that compares to a certain prop on every Thing that did not change (but was cloned). Say you don't have a unique ID to compare Things to, what then ?

This question applies:
https://stackoverflow.com/questions/39428351/how-to-prevent-dom-replacement-in-angular2-ngfor-async-in-angularfire2

I had to do quite a bit of reading and understanding before I understood what was happening...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants