Possible collision with prototype properties in combineReducers

Line 182 in 1133bd8

const previousStateForKey = state[key]

Won't this line cause a bug for keys which are on the object prototype, such as toString, even if non-enumerable?
In our case, it caused a bug on a very old browser which had the Spidermonkey watch property on the Object prototype. That's on us, but wouldn't the same bug occur with other prototype methods?

If my guess is correct, one fix would be

state = Object.create(null);

key is an element of finalReducerKeys, and const finalReducerKeys = Object.keys(finalReducers)

The Object.keys() static method returns an array of a given object's own enumerable string-keyed property names.

So I would assume... no?

Can you give an example?

#4215 was a similar related issue, and we closed it on the grounds that it's too much of an edge case for us to worry about.

Agreed. This is very unlikely to be hit and represents a code smell, IMHO.

Does using a property named "watch" really represent a code smell? I think this is a much less obvious issue than "constructor" in #4215 and doesn't seem particularly smelly to me. Additionally in #4215 a justification for closing the bug is "I'm not sure there's anything we should or even can do about this at the code level", but @rtm provided a potential workaround. I understand that it's not feasible to support every old browser, but it seems reasonable to me that a dictionary of key value pairs doesn't need to inherit from Object.prototype.

Redux is pretty generic JS, and so it should function in any reasonable JS environment. However, we specifically don't try to do any workarounds for browser issues, other than backwards-compiling the published artifacts to ES5 for IE11 support in the current 4.x release line.

Adding more code for these extremely rare edge cases, especially for hypothetical scenarios where very old browsers have weird behavior, is not something we want to get sucked into.

I don't think it's fair to dismiss it as hypothetical or rare as this topic takes up a substantial portion of the MDN Object documentation. But further reading into that page, I think developers are used to an object inheriting from Object.prototype and this change could do more harm than good. A Map may be more idiomatic but this issue is certainly not worth making such a breaking change.

Thanks for the quick and thorough response, I know maintaining such a large project is a sisyphean task and it's not feasible to support every bizarre environment. We certainly wish we didn't have to support these old Spidermonkey browsers either! Have a good one!

You say that this is a change in Object.prototype. Adding something to Object.prototype will not show up in Object.keys.

Please create a demonstration of the issue, like I asked.

For reference:

Object.prototype.foo = 2 
console.log(Object.keys({}))

will print Array []

(Apart from that: generally I am also leaning against protecting against a practice that is widely considered bad and is known for breaking things - but before I make my mind fully, I would like to have enough information to be able to understand the problem.)

A closer look at the code shows that the issue in question is not related to Object.keys. Instead, it's related to the new object called state, which is created as {}, which yields an object with prototype. Then, existing keys are looked up in that state object, and if they happen to be keys which are on the prototype, the lookup "succeeds" and the bug ensues.

This bug is not related to old browsers, or user error, or code smells. It occurs whenever any key is used that is on the Object prototype, whether enumerable or not. That includes, for example, toString.

Whether or not the decision by a user to use a key toString is a "code smell" or an "edge case" is a matter of judgment (as is whether being an "edge" case is a sufficient rationale for not dealing with a bug), but there's no obvious reason why it shouldn't work. The resulting behavior is also likely to be quite difficult to track down for those few unfortunate souls it affects. It would be a different matter if this was very hard to fix, or made the code more complex, or raised the risk of introducing additional bugs, but here it's just a matter of replacing {} with Object.create(null). So I'm frankly not quite sure where the pushback is coming from.

To summarize the objections:

This bug doesn't exist, because the code is using Object.keys. That objection is based on an incorrect reading of the code. I thought the same thing when I first looked at it. But actually the behavior in question does in fact exist, because state is a new object created in such a way that it includes an object prototype, and it is being referenced in a way (state[key]) which does not distinguish between whether the key is an own property or not.
This bug is an edge case, because only old browsers have the watch property. But actually it applies to any property on the object prototype, including those in modern browsers.
This bug is an edge case, because no one in their right mind would ever name a key toString or valueOf or hasOwnProperty. That seems a bit overbearing for us to decide what keys people can or can't use.

Then, existing keys are looked up in that state object

Yes, and existing keys are looked up with Object.keys, which will only return own properties, not inherited properties.
Also, this is not "existing keys on the object created within combineReducers, but "existing keys on the reducers object you pass in".
So, you have to use Object.create(null) in your own code containing the reducers.

I am highly confused what your bug is an please ask you for a reproduction. Your interpretation doesn't make any sense to me - it is missing important details.

This code is working perfectly fine:

import { combineReducers } from "redux";

Object.prototype.extraProperty = "foo";

const reducers = {}
reducers.a = () => 1;
reducers.b = () => 2;

console.log(combineReducers(reducers)(undefined, { type: "initial" }));
// logs `Object { a: 1, b: 2 }`, on further inspection the object has an inherited property `extraProperty` with value `"foo"`

reducers.extraProperty = () => 3;

console.log(combineReducers(reducers)(undefined, { type: "initial" }));
// logs `Object { a: 1, b: 2, extraProperty: 3 }`

=> this works... as expected?

Please explain the problem with an actual example instead of pointing at a position in our source code and making assumptions we cannot verify!