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

Should mapStateToProps be called every time an action is dispatched? #291

Closed
dara76 opened this issue Feb 17, 2016 · 8 comments
Closed

Should mapStateToProps be called every time an action is dispatched? #291

dara76 opened this issue Feb 17, 2016 · 8 comments

Comments

@dara76
Copy link

dara76 commented Feb 17, 2016

I was under the impression that mapStateToProps is called every time an action is dispatched which causes the store to change.

In my simplified example below, this doesn't seem to be the case.

reducers.js

const foo = (state = true, action)  =>{
  switch (action.type) {
  case "foo" :
    return false;
  default :
    return state;
  }
}

const bar = (state = 0, action) => {
  switch (action.type) {
  case "bar" :
    return state + 1;
  default :
    return state;
  }
}


const rootReducer = combineReducers({
  foo,
  bar 
});

Container

import React, { Component } from 'react';
import { connect } from 'react-redux';

class Container extends Component {
  constructor(props) {
    super(props);
    this.onBarClick = this.onBarClick.bind(this);
  }

  onBarClick () {
    this.props.dispatch({
      type : "bar"
    }); 

    // I expect mapStateToProps to be called here 
    // which would allow me to check the value
    // of the updated props, and dispatch foo if the
    // condition is met

    if (this.props.isBarGreaterThanEqual3) {
      this.props.dispatch({
        type : "foo"
      }); 
    }   
  }

  render () {
    return (
      <div>
        <button onClick={this.onBarClick}>Click bar</button>
      </div>
    )   
  }
}

const mapStateToProps = (state) => {
  console.log("map state to props");
  return Object.assign({}, state, {
    isBarGreaterThanEqual3 : state.bar >= 3
  });
};

export default connect(mapStateToProps)(Container);

If it is wrong of me to expect mapStateToProps to be called after every dispatch, what is the recommended way to conditionally dispatch an action when the condition depends on the state of the store that has just been updated by another action?

@gaearon
Copy link
Contributor

gaearon commented Feb 17, 2016

It should be called. Please provide a runnable project reproducing this (on Github or something like http://jsbin.com). I’m confident we don’t have bugs related to this because the library is covered by tests and used in a ton of projects. So something is off with the way you set it up but it’s hard for me to say without running the code and reproducing the issue.

@dara76
Copy link
Author

dara76 commented Feb 18, 2016

I've put an example up at https://github.com/dara76/react-redux-issue.git

The app can be run with npm run dev-server which will make the app accessible at http://localhost:9999.

(I've changed the foo reducer to now to have initial state=true)

const foo = (state = true, action)  =>{
  switch (action.type) {
  case "foo" :
    return false;
  default :
    return state;
  }
}

In the app, I expect the "Waiting for foo" message to change after 3 clicks.

@gaearon
Copy link
Contributor

gaearon commented Feb 18, 2016

I think I (probably) see your point now. I haven’t run the app yet but from the source it looks like you expect this.props.isBarGreaterThanEqual3 to update instantaneously after you dispatch in the same event handler.

This is not how React works. In React, state changes (and Reacf Redux uses setState internally) are potentially asynchronous. This is because React batches update that happen during the same event handler. So calling dispatch will update the store state immediately but your components will be updated a bit later during the same tick, together.

Instead of assuming dispatch updates props synchronously, you can use componentWillReceiveProps to react to changes in the props when they happen.

  onBarClick() {
    this.props.dispatch({
      type : "bar"
    });
  }

  componentWillReceiveProps(nextProps) {
    if (
      !this.props.isBarGreaterThanEqual3 &&
      nextProps.isBarGreaterThanEqual3
    ) {
      this.props.dispatch({
        type : "foo"
      });
    }
  }

Note that also, in general, dispatching actions in response to store-dependant prop changes is an anti-pattern. If you want to react to an action, it is best to do so in reducer. If you want to calculate some state that depends on the store state, it is best to do this in a selector.

@gaearon gaearon closed this as completed Feb 18, 2016
@dara76
Copy link
Author

dara76 commented Feb 18, 2016

Thanks for looking at this.

Yeah, I was expecting the following flow

start handling click event->dispatch bar action->state is updated->mapStateToProps called->render called->check if this.props.isBarGreaterThanEqual3->dispatch foo action if it is->state is updated->mapStateToProps called->render called

but you explain that

React batches updates that happen during the same event handler.

which means the flow is actually more like

start handling click event->dispatch bar action->state is updated->check if this.props.isBarGreaterThanEqual3->dispatch foo action if it is->state is updated->mapStateToProps called->render called

So is it fair to say that mapStateToProps isn't necessarily called after every dispatch, but it is only called once per batch of dispatched actions?

I like the simplicity of this advice

If you want to react to an action, it is best to do so in reducer.
If you want to calculate some state that depends on the store state, it is best to do this in a selector.

Having said that, it's not clear to me how I would update foo via a reducer when bar reaches 3?

const bar = (state = 0, action) => {
  switch (action.type) {
  case "bar" :
    const newState = state + 1;

    if (newState >= 3) {
      // set foo somehow
    }   

    return newState;
  default :
    return state;
  }
};

const foo = (state = false, action)  =>{ 
  switch (action.type) {
  case "foo" :
    return true;
  default :
    return state;
  }
};

@gaearon
Copy link
Contributor

gaearon commented Feb 18, 2016

So is it fair to say that mapStateToProps isn't necessarily called after every dispatch, but it is only called once per batch of dispatched actions?

It is fair to say mapStateToProps is called after but not necessarily immediately after the dispatch. It is called when the component is about to re-render, which depends on whether React batches the updates or not. By default, React batches updates from event handlers.

Having said that, it's not clear to me how I would update foo via a reducer when bar reaches 3?

Since one depends on the state of the other, maybe they should just be a single reducer.

const reducer = (state = { counter: 0, foo: false }, action) {
  switch (action.type) {
  case "bar":
    return Object.assign({}, state, {
      counter: state.counter + 1,
      foo: state.counter === 2
       // depending on your use case, it could also be:
       // foo: !state.foo && state.counter === 2 
       // foo: !state.hasEverBeenFooBefore && state.counter === 2 
       // etc
    });
  case "foo" :
    return Object.assign({}, state, {
      foo: true
    });
  default:
    return state
  }
}

Another, simpler, option is to keep them separate, and compute the result as derived data:

const bar = (state = 0, action) => {
  switch (action.type) {
  case "bar" :
    return state + 1;
  default :
    return state;
  }
};

const foo = (state = false, action)  => { 
  switch (action.type) {
  case "foo" :
    return true;
  default :
    return state;
  }
};

const reducer = combineReducers({
  foo,
  bar
});

const isFooishEnough = (state) => {
  return state.foo || state.bar >= 3;
}

In this case, we defined a selector called isFooishEnough. You can call it from your mapStateToProps and use that value instead. In general, this is the preferred pattern: whatever can be computed from Redux store state, should not be there.

@dara76
Copy link
Author

dara76 commented Feb 18, 2016

Thanks for the explanations and insights, they're very helpful.

@harry-dev98
Copy link

very similar doubt, i guess which got clear now reading comments. Please someone correct me if i am wrong. So basically if not using redux, and using some event handler function i call more than one setState (class or function with hook component), then react collects all the batch of changes at the very end of the event handler and then re-renders the component.

@markerikson
Copy link
Contributor

@harry-dev98 Two things:

@reduxjs reduxjs locked as resolved and limited conversation to collaborators Jan 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants