search

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

for..of on an array?? #12

Closed ljharb closed 8 years ago

ljharb commented 8 years ago

Here https://github.com/leoasis/redux-immutable-state-invariant/blob/master/src/trackForMutations.js#L45 you're using for..of on Object.keys, which is an array. Why not a normal for loop? The transpiled output would be much cleaner, and would work on browsers and node versions that don't have Symbol.iterator available.

leoasis commented 8 years ago

Yeah, I believe you're right. I guess we just need a for .. in in the keys object and we'd get the same behavior.

ljharb commented 8 years ago

Using for..in will iterate over the entire prototype, which is not what you want. You should use a for loop over Object.keys(keys), or wrap the for..in body in if (Object.prototype.hasOwnProperty.call(keys, key)) {

leoasis commented 8 years ago

Yeah, you're right (again :stuck_out_tongue:), will make that change

EDIT: Actually in that code, we're only attaching properties to an empty object literal, making for..in do what we want: https://github.com/leoasis/redux-immutable-state-invariant/blob/master/src/trackForMutations.js#L37

ljharb commented 8 years ago

That doesn't guard against someone adding enumerable properties to Object.prototype, however. for..in is never safe without a guard.

leoasis commented 8 years ago

Ok, makes sense. Will change it to use a regular for with Object.keys(keys) to avoid weird bugs

leoasis commented 8 years ago

Actually, a simpler fix would be to just make key = Object.create(null); so that we're sure that it doesn't have anything else besides its own properties

ljharb commented 8 years ago

Object.create(null) isn't compatible with ES3 tho, so you'd be breaking anyone that still wants to be IE 8 compatible (like Airbnb). Object.keys is definitely the best way.

leoasis commented 8 years ago

Object.keys is also ES5, so if you target ES3 you'll need a polyfill as well.

ljharb commented 8 years ago

Totally! But at least that one is faithfully shimmable :-) You can also use the object-keys module if you want to obviate the need for users to polyfill.

leoasis commented 8 years ago

Since this is just a dev only tool, I'm not so sure we really need to support ES3, since most development environments use modern browsers. Having said that, I'll use Object.keys since it's more understandable than Object.create(null).