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

State is saved in past on INIT action #102

Closed
nivek91 opened this issue Jun 23, 2016 · 15 comments
Closed

State is saved in past on INIT action #102

nivek91 opened this issue Jun 23, 2016 · 15 comments

Comments

@nivek91
Copy link

nivek91 commented Jun 23, 2016

Hi, I was trying to reproduce the example of todos-undo, using beta 8, when I came across this bug.
beta8
After init, the state is modified (in the past object there is a copy of the state), so when I was doing,
canUndo: state.todos.past.length > 0,
you could start the app by undoing.

Then I switched to beta 2, and this changed:
beta2
The past object is empty after INIT.

So this should be documented? Or the example should be updated to use beta8?

Thanks

@pl12133
Copy link
Collaborator

pl12133 commented Jun 24, 2016

Thanks for reporting this @nivek91, any chance you could post the code causing this? Or the printout of the console with debug: true enabled in your undoable reducer, those would both help me track this down a little better.

@nivek91
Copy link
Author

nivek91 commented Jun 24, 2016

Archive.zip

To run it
npm install
npm start
open http://localhost:3000

The component is Counter

Let me know if this helps @pl12133

@pl12133
Copy link
Collaborator

pl12133 commented Jun 24, 2016

Thanks @nivek91 that was very helpful.

It appears that the counter reducer is not valid as a Redux reducer: If your counter reducer is passed an action that is not supposed to handle, it must return state, unmodified.

function handleChangeIncrement(state=1, action) {
  switch(action.type) {
    case CHANGE_INC:
      return +action.value;
    default:
      return state;
  }
}

function handleCounter(state=0, action, incrementValue) {
  switch(action.type) {
    case INC:
      return state + incrementValue;
    default:
      return state;
  }
}

function counter(state = {}, action) {
  let incrementValue = handleChangeIncrement(state.incrementValue, action);
  let counterValue = handleCounter(state.counterValue, action, incrementValue); // note: b depends on a for computation
  return { incrementValue, counterValue };
}

Because you use return { ... } to return a new object literal every time your reducer is called, your reducer will always return a new state. This is not proper in Redux, and causes this to happen:

  { incrementValue: 1, counterValue: 0 } === { incrementValue: 1, counterValue: 0 } // false

Redux-undo expects all the contracts of a valid Redux reducer to hold true, so this is not related to redux-undo :) Here is a nice list of educational Redux links to help you learn more about this. Closing for now, if you edit your reducer and this is still happening, I will gladly re-open this issue.

@pl12133 pl12133 closed this as completed Jun 24, 2016
@nivek91
Copy link
Author

nivek91 commented Jun 24, 2016

Thanks for answering this quickly @pl12133 .

I'm creating the reducers this way based on this: @gaearon reduxjs/redux#749 (comment)

This is the snipped:
`function a(state, action) { }
function b(state, action, a) { } // depends on a's state

function something(state = {}, action) {
let a = a(state.a, action);
let b = b(state.b, action, a); // note: b depends on a for computation
return { a, b };
}`

Am I doing something wrong, or the snippet is wrong?

Lots of thanks!

@peteruithoven
Copy link
Collaborator

The snippet is right, but it was a demonstration of a selector (something to retrieve data), not a reducer.

@nivek91
Copy link
Author

nivek91 commented Jun 24, 2016

How would write a reducer that depends on other reducer's state?

@peteruithoven
Copy link
Collaborator

That's quite a general question, which makes it hard to answer. I would recommend documentation on reducer composition, it's mentioned in the reducer docs and in some talks of: https://egghead.io/courses/getting-started-with-redux

In your case using combineReducers might help.

@nivek91
Copy link
Author

nivek91 commented Jun 24, 2016

I'm already using combineReducers. Can you show with my example how would you do it?

@pl12133
Copy link
Collaborator

pl12133 commented Jun 24, 2016

I haven't tried this out, but generally you just add extra information onto the action being passed around:

function handleChangeIncrement(state=1, action) {
  switch(action.type) {
    case CHANGE_INC:
      return +action.value;
    default:
      return state;
  }
}

function handleCounter(state=0, action) {
  switch(action.type) {
    case INC:
      let { incrementValue } = action;
      return state + incrementValue;
    default:
      return state;
  }
}

function counter(state = {}, action) {
  let incrementValue = handleChangeIncrement(state.incrementValue, action);
  state = handleCounter(state.counterValue, {
    type: action.type,
    incrementValue
  };
  return state;
}

With this reducer, passing in a probe action will not return a new state.

@nivek91
Copy link
Author

nivek91 commented Jun 28, 2016

Hi @pl12133 , first, thanks for answering!

I just tested the code you posted (there was a missing ')' ), and now my initial state is being ignored:

image

Any other ideas?

@pl12133
Copy link
Collaborator

pl12133 commented Jun 28, 2016

I'm not sure I see the issue there, the reducer seems to be working as expected. After @@INIT, it is returning an initial state of 0, so I don't know what is being "ignored". If you would like to see a functioning undoable counter reducer there is an example available at redux-undo-boilerplate. You may want to work on Redux fundamentals before jumping into higher order reducers.

@nivek91
Copy link
Author

nivek91 commented Jun 28, 2016

Hi! @pl12133 , its not working because the initial state should be:
present: { incrementValue: 1, counterValue: 0 },
and not, present: 0
My counter state is compound { incrementValue: 1, counterValue: 0 }, and not a number.. Do you get me?

Thanks for taking the time

@pl12133
Copy link
Collaborator

pl12133 commented Jun 29, 2016

Sure, if you would like to shape your state that way then you can have your functions do that. If you are just getting in to Redux I would recommend the Egghead.io tutorials to learn about creating your own reducers. Here is the code from above combined into one reducer:

const initialState = {
  incrementValue: 1,
  counterValue: 0
};
function counter(state = initialState, action) {
  switch(action.type) {
    case CHANGE_INC:
      return Object.assign({}, state, {
        incrementValue: +action.value
      })
    case INC:
      return Object.assign({}, state, {
        counterValue: state.counterValue + state.incrementValue
      })
    default:
      return state;
  }
}

@nivek91
Copy link
Author

nivek91 commented Jun 29, 2016

Thats what I was trying to do, but I wanted to split the logic into 2 reducers (or sub reducers), one depending on the other one. It seems that splitting them is not possible? I already watched those tutorials, but they dont show how to share state (using different reducers)

@pl12133
Copy link
Collaborator

pl12133 commented Jun 29, 2016

That may be possible, but it does not seem like a good idea for one reducer to rely upon another because that is creating a side effect. Avoiding side effects is good because it makes a Redux reducer very easy to reason about. Here is what the doc's say about it:

We’ll explore how to perform side effects in the advanced walkthrough. For now, just remember that the reducer must be pure. Given the same arguments, it should calculate the next state and return it. No surprises. No side effects. No API calls. No mutations. Just a calculation.

If your reducer needs some extra data, you can pass it in on the action object. Having reducers rely on other reducers is an advanced topic, and you should first practice reducer composition in a realistic app before worrying about that.

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

No branches or pull requests

3 participants