1

I'd been working with React & Redux for some time when a work colleague saw some code I wrote and commented on it.

SomeComponent.js

class SomeComponent extends Component {

  async componentDidMount() {
    await this.props.fetchPosts();  

    if (this.props.posts.length < 1)
      return navigateTo( /* someOtherPlace */);
  }

  render() {
    return (
        <>
          {this.props.posts.map(
            (postData, i) => <Post key={i} {...postData}/>
          )}
        </>
    );
  }
}

const mapStateToProps = ({ posts }) => ({
  posts: posts.list,
  isFetching: posts.isFetching
});

export default connect(mapStateToProps, { fetchPosts })(SomeComponent);

actions/posts.js

export const fetchPosts = () => async dispatch => {
  dispatch(requestPosts());
  let posts;
  try {
    posts = (await api.get('/posts')).data
  } catch (e) {
    posts = e;
  }
  dispatch(receivePosts(posts));
}

He basically said that I shouldn't be awaiting for fetchPosts() action, instead I should just call it, and let it update props, re-render and perform the conditional navigation in componentDidUpdate which when he said it, it totally made sense to me.

But now I keep asking myself if what I was doing was really that bad, potencially buggy or just a bad practice that added more complexity.

He didn't mention the reasons why it was wrong other than it wasn't the React way of doing it.

EDIT: Added code snippet showing that the approach actually does work and doesn't perform faulty reads.

Edit Await in componentDidMount

Franch
  • 621
  • 4
  • 9
  • 22

1 Answers1

0

So there is small issue in your case

async componentDidMount() {
    await this.props.fetchPosts();  

    if (this.props.posts.length < 1)
      return navigateTo( /* someOtherPlace */);
}

Here await will wait till the fetchPosts is completed provided it returns a promise. Now fetchPosts will dispatch an action that will only result in the props being updated and another render triggered to update the posts in the component. So even if you wait for fetchPosts to complete the posts are not updated in the same render cycle and hence using this.props.posts.length won't return you the result corresponding to the latest posts update in redux store. The result being that you are unnecessarily waiting for the fetchPosts to complete and performing a check which will lead to a faulty result

Better approach is to work like

class SomeComponent extends Component {

  componentDidMount() {
    this.props.fetchPosts();  
  }

  componentDidUpdate(prevProps) {
     if(this.props.posts !== prevProps.posts && this.props.posts.length < 1) {
          return navigateTo( /* someOtherPlace */);
     }
  }

  render() {
    return (
        <>
          {this.props.posts.map(
            (postData, i) => <Post key={i} {...postData}/>
          )}
        </>
    );
  }
}
Shubham Khatri
  • 270,417
  • 55
  • 406
  • 400
  • I added a small code that shows that actually no faulty result happens, check the console output of it. – Franch Dec 13 '18 at 14:59