Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor combineReduer's Unexpected State Shape Warning #1118

Merged
merged 1 commit into from
Dec 13, 2015

Conversation

jridgewell
Copy link
Contributor

The shape of the previous state is irrelevant, what we’re actually
testing is that state has the same shape as our reducer object
finalReducers.

Also fixes the algorithm to be O(n) by using #hasOwnProperty vs doing
an array search O(n2).

@jridgewell jridgewell force-pushed the combineReducers-unexpected-shape branch from b88deec to 5bce135 Compare December 9, 2015 21:28
@acdlite
Copy link
Collaborator

acdlite commented Dec 10, 2015

This looks good to me. 👍 for using hasOwnProperty especially.

@acdlite
Copy link
Collaborator

acdlite commented Dec 10, 2015

Someone else should review this too, though, since I'm not as familiar with this module.

@@ -15,8 +15,8 @@ function getUndefinedStateErrorMessage(key, action) {
)
}

function getUnexpectedStateKeyWarningMessage(inputState, outputState, action) {
var reducerKeys = Object.keys(outputState)
function getUnexpectedStateShapeWarningMessage(inputState, reducer, action) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument should probably be called reducers, not reducer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@gaearon
Copy link
Contributor

gaearon commented Dec 12, 2015

I think @ellbee should remember this better than me.
Basically we need to make sure we don't regress on the warning we introduced in #715 (comment) and #720.

@ellbee
Copy link
Contributor

ellbee commented Dec 12, 2015

I have just been playing with this, and I am pretty sure that this behaves the same as what is currently there.

@gaearon
Copy link
Contributor

gaearon commented Dec 12, 2015

Leaving this for you to review then 👍

The shape of the previous state is irrelevant, what we’re actually
testing is that state has the same shape as our reducer object
`finalReducers`.

Also fixes the algorithm to be O(n) by using `#hasOwnProperty` vs doing
an array search.
@jridgewell jridgewell force-pushed the combineReducers-unexpected-shape branch from 5bce135 to e5aabde Compare December 12, 2015 19:24
@ellbee
Copy link
Contributor

ellbee commented Dec 13, 2015

So, the changes in this PR are passing the finalReducer instead of outputState, the switch to hasOwnProperty and passing an empty object when initial state is not specified. I did some testing and they look good. Thanks @jridgewell.

ellbee added a commit that referenced this pull request Dec 13, 2015
…hape

Refactor combineReduer's Unexpected State Shape Warning
@ellbee ellbee merged commit c581573 into reduxjs:master Dec 13, 2015
@gaearon
Copy link
Contributor

gaearon commented Jan 25, 2016

Out in 3.0.6, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants