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

Fixed combineReducers changeDetection logic(#3488) #3490

Merged
merged 2 commits into from
Aug 12, 2019

Conversation

anubhavgupta
Copy link
Contributor

Fixes following issue:

State doesn't update when replaceReducers is called with combineReducers and one of the reducers passed to it is removed.(#3488)

@netlify
Copy link

netlify bot commented Jul 31, 2019

Deploy preview for redux-docs ready!

Built with commit 2c24ecc

https://deploy-preview-3490--redux-docs.netlify.com

@timdorr
Copy link
Member

timdorr commented Aug 2, 2019

What if replaceReducers swaps out one reducer for another? I don't think length is sufficient for a check here.

@anubhavgupta
Copy link
Contributor Author

Are you suggesting about following use case?:

const bar = (state = { value: 'bar' } ) => state
const baz = (state = { value: 'baz' } ) => state

const originalCompositeReducer = combineReducers({ bar })
const store = createStore(originalCompositeReducer)
store.dispatch(ACTION)
const initialState = store.getState() // returns ->  { bar: { value: 'bar' }}

// replacing `bar` reducer with `baz` reducer
store.replaceReducer(combineReducers({ baz })) // returns ->  { baz: { value: 'baz' }}

const nextState = store.getState()
expect(nextState).not.toBe(initialState)

I have tried it out and it works as expected.
Should I add this test case in the PR?

@anubhavgupta
Copy link
Contributor Author

anubhavgupta commented Aug 12, 2019

@timdorr Here is an exhaustive test case. It verifies the change detection logic when we replace one reducer with another.

Expected has state changed results when we switch each combined reducer listed in the first column with the eachcombined reducer listed in the first row.

{ foo, bar } { foo, bar, baz } { foo, baz }
{ foo, bar } same state state changes state changes
{ foo, bar, baz } state changes same state state changes
{ foo, baz } state changes same state state changes
const foo = (state = { value: 'foo' }) => state
const bar = (state = { value: 'bar' }) => state
const baz = (state = { value: 'baz' }) => state

const combinedReducers = [
  combineReducers({ foo, bar }),      // reducer 1
  combineReducers({ foo, bar, baz }), // reducer 2
  combineReducers({ foo, baz }),      // reducer 3
];

// Expected `has state changed` results when we switch one reducer with another.
const hasStateChangedResults = [
  // eg: when we switch `reducer 1` with `reducer 2` the resulting state should change.
  // But if we replace `reducer 1` with `reducer 1`(same reducer) the resulting state should not change.
  //1->1  1->2  1->3
  [false, true, true],
  //2->1  2->2  2->3
  [true, false, true],
  //3->1  3->2  3->3
  [true, true, false]
];

// initialize store with any reducer
const store = createStore(combinedReducers[0])
store.dispatch(ACTION)

// run all test cases
for(let i=0;i < hasStateChangedResults.length; i++){
  for(let j=0; j < hasStateChangedResults[i].length; j++){
    store.replaceReducer(combinedReducers[i])
    const state1 = store.getState()
    store.replaceReducer(combinedReducers[j])
    const state2 = store.getState()
    expect(state1 !== state2).toBe(hasStateChangedResults[i][j])
  }
}

I have verified it with the PR code and it works.

@timdorr
Copy link
Member

timdorr commented Aug 12, 2019

OK, thanks for the exhaustive testing!

@timdorr timdorr merged commit 9c9a4d2 into reduxjs:master Aug 12, 2019
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.

3 participants