reduxjs / redux

A JS library for predictable global state management
https://redux.js.org
MIT License
60.91k stars 15.27k forks source link

MapStateToProps not being called even after changing STATE in reducer. #1875

Closed shivateja-madipalli closed 8 years ago

shivateja-madipalli commented 8 years ago

Do you want to request a feature or report a bug?

I am retrieving data from Salesforce using JS remote Actions and populating data in React JS.

The structure of my application:

My component1.js:

import React from 'react';
const component1 = React.createClass({

  getInitialState() {
    return {
      basicData : {},
      interestData : {},
      accountingData : {},
      isLoading : true
    }
  },

  componentWillMount() {
    console.log('componentWillMount');
  },

  componentDidMount() {
    console.log('componentDidMount');
    this.props.action1();
  },

  componentWillReceiveProps(NextProps) {
    console.log('componentWillReceiveProps', NextProps);
  },

  componentWillUnmount() {
    console.log('componentWillUnmount');
  },

  render()  {
    console.log('RENDERING');
    return (
      <div>
        <p> Loading! </p>
      </div>
    )

  }

});

export default component1;

This is my container.js

import React, { Component } from 'react';
import { connect } from 'react-redux';
import RootComponent from '../components/component1.js';
import { action1 } from '../actions/actionCreators';

function mapStateToProps(globalState) {
  return {
    value1: globalState.value1,
    value2: globalState.value2
  };
}

const mapDispatchToProps = (dispatch) => {
  return {
     getData: () => {
        dispatch(getData())
    }
  }
}

export default connect(mapStateToProps, mapDispatchToProps)(component1);

Action creator:

export function action1() {
  return {
    type: 'RETRIEVE_REQUESTED_DATA',
    value1: getRecords()
  };
}

Reducer:

        case "RETRIEVE_REQUESTED_DATA":
          action.value1.then(function(data) {
            return testVar = { ...state, basicData: state.basicData, interestData: data, accountingData: state.basicData, isLoading: false};
          });

Though, I am returning altered state from my reducer, MapStateToProps in my container is not being invoked.

The server call returns a promise and that promise is handled in reducer here, once the data is retrieved then I am populating the data to my state.

My view to the issue,

Once data is retrieved from DB (Salesforce), state is altered thus, MapStateToProps should be invoked because it is connected to the Component with Connect.

Please shed some light.

I am very new to Redux and React not sure whether this is a issue or a usage question, pls excuse my ignorance.

markerikson commented 8 years ago

Yeah, this is definitely a usage question.

Also, you should absolutely not be using promises inside of a reducer. Not only does it break how reducers are supposed to behave, the return statement in that "RETRIEVE_REQUESTED_DATA" example doesn't do anything like what you want it to. You're effectively returning undefined from that reducer case.

shivateja-madipalli commented 8 years ago

Yeah! It was my mistake that I couldn't see it,

This is what I am doing currently,

case "RETRIEVE_REQUESTED_DATA": return {...state, basicData: state.basicData, interestData: state.interestData, accountingData: action.jsonValueOfAccountingPage, isLoading: false};

cubiccompass commented 8 years ago

Does the render function use this.state or this.props for data binding?

My understanding is that mapStateToProps() should replace the need for getInitialState(), and encourage use of this.props.

markerikson commented 8 years ago

@cubiccompass : Those are separate things.

getInitialState is the method used to initialize local component state with the React.createClass() component definition syntax. mapStateToProps is the method that React-Redux's connect function uses to extract pieces of data from the store and pass them to a component as props. Those are not mutually exclusive.

You can have a component that renders data using props retrieved from the Redux store, props passed down by a parent component, and local component state, all at once. How you write your own render functions is up to you. Example:

const MyComponent = React.createClass({
    getInitialState : function() {
        return {
            valueFromLocalComponentState : 42
        };
    }

    render : function() {
        const {propFromReduxState, propFromParent} = this.props;
        const {valueFromLocalComponentState) = this.state;

        return (
            <div>
                <div>Redux prop: {propFromReduxState}</div>
                <div>Parent prop: {propFromParent}</div>
                <div>State value : {valueFromLocalComponentState}</div>
            </div>        
        );
    }
});

const mapState = (state) => {
    return {
        propFromReduxState : state.someValue
    };
}

export default connect(mapState)(MyComponent);

// and in some parent component in another file
<MyComponent propFromParent={123} />
cubiccompass commented 8 years ago

Thanks @markerikson . That's an excellent example of binding to either state or props.

Per @shivateja-madipalli's original question

MapStateToProps in my container is not being invoked

I'm curious to know if the intent of render() was to bind with state or props; and if props, might the troubleshooting benefit from removing getInitialState (?)

markerikson commented 8 years ago

As far as I can tell, the original example is broken because the reducer is trying to A) use a promise, and B) do a return inside that promise. Thus, returning undefined.

Also, conceptually what render() does isn't "binding", so it's probably best to avoid that terminology.

cubiccompass commented 8 years ago

Thanks @markerikson

Related question, what would be the impact of solely using Redux store.subscribe in every component, rather than using react-redux connect() with mapStateToProps?

componentWillMount() {
    store.subscribe(() => {
        this.setState({value: store.getState().someProperty});
    });
},

It appears to accomplish the same thing, but maps Redux state to component state, instead of props.

The import of "store" seems to work fine in this context. But using the Provider approach throws exception "Could not find store".

<Provider store={store}>
  <Application />
</Provider>
// Child components throw "Could not find store"

(note: I'm having similar issue as OP. mapStateToProps event not firing)

jimbolla commented 8 years ago

Every component that connected to the store like this would setState on every store state change, which would destroy performance. It would also run into issues with child components rerendering before parents, which means its state may be out of sync with its props.

cubiccompass commented 8 years ago

React should re-render only if the state actually changes though, correct? At least, that's the perceived benefit of virtual DOM.

mapStateToProps(state) also receives the entire state tree. No?

Is there some conditional magic in react-redux connect that improves performance?

naw commented 8 years ago

To be clear --- there is nothing intrinsically wrong with subscribing to the redux store directly in a component, it's just that react-redux does extra things for you that you'll have to think about for yourself if you want to subscribe manually.

naw commented 8 years ago

React should re-render only if the state actually changes though, correct?

No. Last time I checked, every time you call setState, React will render (i.e. generate virtual DOM and compare it to the old virtual DOM). This is true even if your state doesn't change. If the state doesn't change, the old and new virtual DOM will be the same, so React won't have to manipulate the DOM (which is good), but it still has to run the comparisons to know that. You can "help" React avoid the comparisons with careful use of setState and/or shouldComponentUpdate.

Is there some conditional magic in react-redux connect that improves performance?

Yes, it has a fairly intelligent shouldComponentUpdate that checks object identity equality on each key of the object returned by mapStateToProps. You could do something similar in your example prior to calling setState to achieve similar performance gains.

cubiccompass commented 8 years ago

Thanks for clarifying. So something like this could be performant:

componentWillMount: function() {
    store.subscribe(() => {
        var data = store.getState();
        if(data.someProperty != this.state.someProperty){
            this.setState({someProperty: data.someProperty});
        }
    });
},

shouldComponentUpdate: function(nextProps, nextState) {     
    return (nextState.someProperty != this.state.someProperty);
},

Understood that connect() handles a lot of this already, but in a props context.

Checking equality for objects or collections would likely need some flavor of immutable JS.

If this approach doesn't grossly violate the Redux data flow, then I'm inclined to stick with it. Mapping state-to-state seems logically easier to comprehend.

markerikson commented 8 years ago

No, that's the anti-pattern that was described. Or rather: connect already does that step for you, so that your real component only re-renders when the mapped data actually changes.

I'm really not clear why you're pursuing this approach.

jimbolla commented 8 years ago

subscribing in componentWillMount will cause memory leaks if you're doing server-side-rendering. In that case, you have to use componentDidMount, which leads to cases where you also have to do a state compare and call forceUpdate if relevant store state has changed.

If you have nested connected components, and the child depends on props from the parent that were derived from store state, you'll run into issues with your state being updated before it receives new props, which can cause you issues in either the listener function or render.

jimbolla commented 8 years ago

I just recently rewrote connect myself and had to learn it's not as simple as one thinks. There's a lot of edge cases and performance considerations that you will ultimately run into that are already handled for you with connect. Unless you have a compelling case against using connect, reinventing the wheel now is just shooting yourself in the foot.

cubiccompass commented 8 years ago

I'm really not clear why you're pursuing this approach.

Similar to OP, connect is not working. Child components of <Provider store={store}> throw "Could not find store" exception.

But store is clearly in scope for the store.subscribe() use case mentioned above.

Connect possibly has a dependency on this.context.store?

jimbolla commented 8 years ago

Yes, connect looks in this.context.store or this.props.store for the store instance.

cubiccompass commented 8 years ago

I got this far before pivoting to an alternative solution. Not quite sure why state is not in scope for child components.

window.store = Redux.createStore(reducer, initialState);
ReactDOM.render(
   <Provider store={store}>
      <Application />
   </Provider>,
   document.getElementById('container')
);

Note: The environment I'm working in does not support RequireJS, so all child components are being defined at window scope.

window.Application = React.createClass({ })

window.store = Redux.createStore(reducer, initialState);

Not sure if this could be causing a module/scope conflict with react-redux (?)

(no server-side rendering btw @jimbolla).

markerikson commented 8 years ago

@cubiccompass : can you post some larger samples of code as a gist?

naw commented 8 years ago

It would also run into issues with child components rerendering before parents, which means its state may be out of sync with its props.

@jimbolla Would you be willing to clarify? I definitely agree state can be out of sync with props (and I appreciate you pointing that out because it's subtle), but I'm curious if it is actually feasible for the child to render before the parent?

If this were true, it seems that connect itself would be vulnerable to situations where a child connected component runs mapStateToProps against an old version of ownProps. In my experience, even when the child receives the store update before the parent, the parent renders before the child.

Isn't it precisely the fact that the child render doesn't run until after the parent that allows connect to postpone its call to mapStateToProps until after props have "caught up"?

I would appreciate your clarification, either confirming or correcting -- thanks!

jimbolla commented 8 years ago

@naw Because subscribing is done in componentWillMount, and that method fires from bottom-up, currently in react-redux 4.x (and fixed in the 5.0 beta) children subscribe to the redux store before their parents. This means, when store state changes, the children get notified first and rerender themselves. Then the parent component gets notified. This can lead to extra renders of the children where they have stale props because their parent hasn't been updated yet. The fix was to make children subscribe to their parents, and only toplevel connected components subscribe directly to the store. This ensures subscriptions always fire top to bottom.

cubiccompass commented 8 years ago

Where in the react event lifecycle, if any, does the connect event occur?

In the examples I'm following online, connect is called outside React components.

Does this lead to any race conditions or dependency issues?

markerikson commented 8 years ago

There is no "connect event".

You use the connect method to create a wrapper around your real component. When an instance of that wrapper component is mounted, it looks for the store using React's "context" feature.

An issue thread is really a bad place for tech support. I'd suggest doing some further reading, or going to a place where you can better ask questions.

If you haven't read it yet, you should really read http://redux.js.org/docs/basics/UsageWithReact.html . There's also a wide variety of tutorials available online, many listed at https://github.com/markerikson/react-redux-links/blob/master/redux-tutorials.md.

If you've got specific questions, come drop by the Reactiflux chat channels on Discord. The invite link is at http://www.reactiflux.com .

cubiccompass commented 8 years ago

OK. Case closed... I've resolved my issue. Thanks again for your patience and insightful responses, and apologies for inundating your inbox.

A few concepts that weren't clear to me upon first reading (or were misrepresented in other articles):

Looking forward to exploring this binding in more detail!

jimbolla commented 8 years ago

If you're interested in the lifecycle, you can explore the relevant source code

markerikson commented 8 years ago

@cubiccompass : one more bit of clarification:

It's not an "instance" of a component. It's defining a component type. As a rough parallel, check out this Java:

// defines the type
class MyClass {}

// create an instance of the type
MyClass mcInstance = new MyClass();

Or, to put it another way:

// define a factory that takes in component classes and wraps them up
const connectedComponentClassFactory = connect(mapState);

// Create a class type that wraps up Component1 instances
const ConnectedComponent1Class = connectedComponentFactory(Component1);

// Create a class type that wraps up Component2 instances
const ConnectedComponent2Class = connectedComponentFactory(Component2);

// define the factory, and immediately use it to wrap up Component3 instances
const AnotherConnectedComponent = connect(mapState)(Component3);

And yes, you should always return an object from mapState functions, because each key in the object will be translated into a prop for your "real" component.

naw commented 8 years ago

@jimbolla

This means, when store state changes, the children get notified first and rerender themselves. Then the parent component gets notified. This can lead to extra renders of the children where they have stale props because their parent hasn't been updated yet

Great, thanks for your clarification. I think what confused me is that when children get notified first, they don't always rerender themselves first --- in my experiments, the child didn't render until after the parent even though the child received state updates first.

I believe this is related to the fact that React defers setState updates under certain conditions, and doesn't defer under other conditions: http://www.bennadel.com/blog/2893-setstate-state-mutation-operation-may-be-synchronous-in-reactjs.htm

In other words, in v4, sometimes this problem manifests itself, and sometimes it doesn't, probably depending on what triggered the initial dispatch that causes the store to update. Thanks for fixing this in v5.

jimbolla commented 8 years ago

@naw Yeah, I think the children would usually render with correct values, because of the batching; but there was an issue with the children's mapStateToProps getting called once with fresh state but stale props, then called a second time with fresh state and fresh props.