for..of on an array??

[ljharb]

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]

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]

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]

Yeah, you're right (again 😛), 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]

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

[leoasis]

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

[leoasis]

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]

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]

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

[ljharb]

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]

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).