isPlainObject() util returns false for objects without prototypes

[rgrove]

Object.create(null) produces a plain object whose prototype is null. Lodash's isPlainObject() correctly treats this as a plain object, but the isPlainObject() helper introduced in #936 does not. This broke our app, since we have a number of selectors which return prototypeless objects.

PR to follow.

[markerikson]

Can you give examples of these selectors? Why are they written that way? What use cases are you working with, and how are things breaking? Are you sure this is really an issue on the React-Redux side, or are you actually seeing the use of the same function in the Redux core?

The codebase only uses isPlainObject for a helper function called verifyPlainObject, which is A) only used in dev mode, and B) realistically only used to verify that the default merged props result is itself a plain object. This shouldn't be affecting cases where you're returning these objects inside your mapState results, or something similar.

[rgrove]

Can you give examples of these selectors?

A mapStateToProps function that returns an object created with Object.create(null) will cause the following error to be thrown in dev mode:

TypeError: Cannot convert object to primitive value
    at verifyPlainObject ([redacted]/node_modules/react-redux/lib/utils/verifyPlainObject.js:15:114)
    at Function.detectFactoryAndVerify ([redacted]/node_modules/react-redux/lib/connect/wrapMapToProps.js:70:81)
    at mapToPropsProxy ([redacted]/node_modules/react-redux/lib/connect/wrapMapToProps.js:53:46)
    at impureFinalPropsSelector ([redacted]/node_modules/react-redux/lib/connect/selectorFactory.js:16:23)
    at Object.runComponentSelector [as run] ([redacted]/node_modules/react-redux/lib/components/connectAdvanced.js:38:25)
    at Connect.initSelector ([redacted]/node_modules/react-redux/lib/components/connectAdvanced.js:195:23)
    at new Connect ([redacted]/node_modules/react-redux/lib/components/connectAdvanced.js:133:15)
    at processChild ([redacted]/node_modules/react-dom/cjs/react-dom-server.node.development.js:2677:14)
    at resolve ([redacted]/node_modules/react-dom/cjs/react-dom-server.node.development.js:2643:5)
    at ReactDOMServerRenderer.render ([redacted]/node_modules/react-dom/cjs/react-dom-server.node.development.js:2979:22)
    at ReactDOMServerRenderer.read ([redacted]/node_modules/react-dom/cjs/react-dom-server.node.development.js:2950:25)
    at renderToString ([redacted]/node_modules/react-dom/cjs/react-dom-server.node.development.js:3369:25)
    [redacted]

Why are they written that way? What use cases are you working with, and how are things breaking?

1. Object.create(null) is sometimes preferable to {} when using an object as a dictionary, since it's not prone to collisions with key names like "hasOwnProperty" that exist on Object.prototype.

2. To reduce GC pressure in our app, we have an emptyObject module that exports Object.freeze(Object.create(null)) for use anywhere we need to return an immutable empty object. This allows us to avoid creating throwaway objects. We sometimes return this object from selectors that need to return an empty object.

The codebase only uses isPlainObject for a helper function called verifyPlainObject, which is A) only used in dev mode, and B) realistically only used to verify that the default merged props result is itself a plain object. This shouldn't be affecting cases where you're returning these objects inside your mapState results, or something similar.

I'm afraid it does affect these cases, and it causes exceptions to be thrown in dev mode, which makes development...difficult.

Are you opposed to the fix in PR #1075? If so, why?

[rgrove]

Noticed you added an additional question in an edit:

Are you sure this is really an issue on the React-Redux side, or are you actually seeing the use of the same function in the Redux core?

Yes, this is an issue in React-Redux. Given that Redux uses an identical stripped-down isPlainObject() util, the issue probably exists there too, but Redux was not the cause of the breakage in our app in this case.

[markerikson]

Thanks for the additional info. You're right - that is from React-Redux, as it's attempting to verify that the initial value returned from mapState is a plain object.

This implementation of isPlainObject was originally written for the Redux core, when we noted that Lodash's _.isPlainObject was relatively slow. @timdorr wrote this version. I did ask at the time if and why people might use Object.create(null), and we concluded it was a niche use case: reduxjs/redux#2599 (comment) . This is legitimately the first I've heard of someone actually doing it :)

I'm not opposed to the change in principle - looks like it's really adding just the one-line check, plus there was a formatting change? I'll let @timdorr make the call on that, since he wrote this function.

However, I am kinda confused on the stack trace you showed, for two reasons:

• It shows verifyPlainObject.js at line 15, which in the published version of verifyPlainObject.js is a closing curly brace as far as I can tell.
• The actual error listed is TypeError: Cannot convert object to primitive value, and I don't know what would actually be causing that error to be thrown (and that's a totally different thing than printing a dev warning about it not being a plain object).

What version of React-Redux are you using? Have you recently changed that?

[rgrove]

I'm not opposed to the change in principle - looks like it's really adding just the one-line check, plus there was a formatting change?

Hmm. If there was a formatting change it was unintentional, but I'm not seeing it. Could you point it out in a comment on the PR? The change is more than just a single-line change; I was trying to both preserve the original behavior of the function and cover this new use case with minimal perf impact.

It shows verifyPlainObject.js at line 15, which in the published version of `verifyPlainObject.js is a closing curly brace as far as I can tell.

Whoops, sorry. I inadvertently copied and pasted a stack trace from a modified version of the file. I had added a console.log() above the line where the error occurred. Line 14 is the correct line in the published source.

The actual error listed is TypeError: Cannot convert object to primitive value, and I don't know what would actually be causing that error to be thrown (and that's a totally different thing than printing a dev warning about it not being a plain object).

The attempt to print a dev warning is itself causing this error by attempting to concatenate value with a string, which doesn't work when value has no toString() prototype function.

[markerikson]

Eh, I guess it was the way I was viewing the diff. Thanks for the explanation on the error there. (Side question: is there any tweak we can make to that logic so it doesn't crash if there's no toString() available?)

The change seems reasonable to me. However, we need to do more than just merge it straight to master. That's currently got our 6.0-beta code in it. We're going to need to set up a 5.x branch and merge this into that branch too for a 5.1.1 release, and we should make the same change to the Redux core to keep the behavior in sync.

@timdorr , can you deal with the branching and merging aspects on this one?

[timdorr]

Yeah, I'll poke it with a stick tomorrow.

[markerikson]

@rgrove : the one thing I didn't get an answer to earlier - just for reference, what version of React-Redux are you on, and did you happen to upgrade recently?

[rgrove]

@markerikson Sorry. We encountered this bug while trying to upgrade from 5.0.7 to 5.1.0.

[markerikson]

Okay, yeah, that would be the first release that had this changed version of isPlainObject. Got it.

[timdorr]

Cherry picking into 5.1.1.

[timdorr]

This is also out as 6.0 beta 3: https://github.com/reduxjs/react-redux/releases/tag/v6.0.0-beta.3