leoasis / redux-immutable-state-invariant

Redux middleware that detects mutations between and outside redux dispatches. For development use only.
MIT License
937 stars 37 forks source link

Does not seem to detect direct manipulation of a value #4

Closed omnidan closed 9 years ago

omnidan commented 9 years ago

I tried adding your library to my redux-undo-boilerplate and was testing it out by adding state++ in my counter reducer. Unfortunately, this library does not seem to detect this state manipulation.

You can test it out by cloning the enforce-immutable branch and running it with npm run dev: https://github.com/omnidan/redux-undo-boilerplate/tree/enforce-immutable

leoasis commented 9 years ago

You're not mutating the state there, since a number is immutable in Javascript natively. What you are mutating there is the local variable reference state that holds the state.

That is perfectly valid stuff to do inside a reducer, and it won't cause any problems with redux at all.

To give an example so you can see the difference:

// This sets the local state value to something new, but it's not a state mutation
state = {...state, foo: 1}

// This is a mutation, and this _will_ be detected by the library:
state.foo = 1

The example you point out in your repo, is the equivalent of doing this, which is not a mutation:

state = state + 1
// or, to make it even clearer:
const prevState = state
state = prevState++
// here, state now points to the previous value + 1, but prevState still holds the previous value.

Hope this helps you understand what is a state mutation and what is a variable reference mutation

omnidan commented 9 years ago

@leoasis interesting. I did not know that. Well that explains it then, so I'll close it. Thanks! :grin:

omnidan commented 9 years ago

@leoasis okay, I must be doing something really wrong. I tried what you told me to do, but it doesn't seem to detect that either: https://github.com/omnidan/redux-undo-boilerplate/commit/4203de947ae792657fcf3e48d6c73898b4e7a300

leoasis commented 9 years ago

Well, that seems to actually be a bug here. I'm taking a look and will push a fix. Thanks for reporting!

omnidan commented 9 years ago

@leoasis no problem :grin: let me know when it's fixed and I'll test it for you (or you can just use https://github.com/omnidan/redux-undo-boilerplate/commit/4203de947ae792657fcf3e48d6c73898b4e7a300)

leoasis commented 9 years ago

@omnidan Alright, pushed some changes to master, would you mind checking if that now works for you? I realized that I was checking the mutations taking into account the returning state as well as the previous, but it turns out that I only needed to check the reference and the snapshot of the previous state to see if they are different.

Anyway, it should be working, added some new tests and they all pass, but if it doesn't (or it breaks with something else) let me know!

omnidan commented 9 years ago

@leoasis huh, I can't seem to be able to install the master branch with npm - I tried:

npm install leoasis/redux-immutable-state-invariant
npm install leoasis/redux-immutable-state-invariant#master
npm install git://github.com/leoasis/redux-immutable-state-invariant
npm install git://github.com/leoasis/redux-immutable-state-invariant#master

None of these seem to work, they all say redux-immutable-state-invariant@1.1.0 node_modules/redux-immutable-state-invariant but then don't install it at all. Would you mind testing it with my boilerplate and releasing a new version? :grin:

omnidan commented 9 years ago

Actually they do install something, but it doesn't seem to have any source code, just an example folder, README.md, LICENSE.md and package.json :scream:

leoasis commented 9 years ago

Ah yes, you're right, since it gets it from the repo but it doesn't have the compiled files. Will test your repo locally and publish a release

omnidan commented 9 years ago

Ahh that makes sense. I was getting kinda confused about this :stuck_out_tongue: Thanks a lot!

leoasis commented 9 years ago

Tried it on your repo and seems to work now, released! https://github.com/leoasis/redux-immutable-state-invariant/releases. Let me know if you find something else!

Thanks again for filing the issue!

omnidan commented 9 years ago

@leoasis hm... I just installed 1.1.1 but it still does not seem to detect the mutation for me. Is there anything you changed in my code to make it work?

leoasis commented 9 years ago

Yes, it is working for me. Cloned your repo, went to the enforce-immutable branch, then did: npm install and npm run dev. Then I click the + button in the site and I see the mutation error.

omnidan commented 9 years ago

@leoasis Sorry - it was a stupid mistake on my side. I started it in production mode, it works fine in dev mode :stuck_out_tongue: This can be closed then!

Time to get some :coffee:

leoasis commented 9 years ago

Glad it worked, and thanks again for finding (and filing) this bug :D

omnidan commented 9 years ago

You're welcome :grin: Thanks for fixing it, I'll add your project to my boilerplate now.

EDIT: https://github.com/omnidan/redux-undo-boilerplate/tree/master#what-happens-if-i-mutate-the-state-directly

leoasis commented 9 years ago

Thanks for using it/mentioning :D