5
\$\begingroup\$

I am creating a blog using React, MongoDB, Express and Node. I have three components: App, List, and Item. The item is a blog post; the list is a list of the blog posts, and the app includes a place to enter text and submit it. I will eventually add more functionality, but I want to determine if I am adhering to best practices for React (I doubt I am).

So in App, I getInitialState with an array of posts (posts) and a string of text for the input (postbody). I used the componentDidMount to make an AJAX GET request to my database, so the user can see all the posts.

To handle entering text I just made a simple handleChange function which updates the state of postbody.

I also have a handleClick function, which grabs this.state.postbody and then POSTs it database. However the same function also makes a separate GET request of the database to update the state of the posts array. This doesn't seem right! Shouldn't that be handled some other way and updated automatically? * This is the primary question I have. *

Also, please let me know if I need to break the components down further, or if I am violating best practices using React (e.g. changing state in the wrong place, or using props incorrectly).

var React = require('react');

var Item = React.createClass({
  render: function() {
    return (
      <div>
        <h2>{this.props.postbody}</h2>
      </div>
    )
  }
})

var List = React.createClass({

  render: function() {
    return (
      <div>
        {this.props.array.map(function(post) {

          return (
            <Item postbody={post.postbody}></Item>
          )
        })}
      </div>
    )
  }
})

var App = React.createClass({

  getInitialState: function() {
    return {
       posts: [],
       postbody: ''
    }
  },

  componentDidMount: function() {
    $.ajax({
      type: 'GET',
      url: '/api/blogPosts',
      success: function(data) {
        this.setState({posts: data});
      }.bind(this)
    })
  },

  handleClick: function() {

    event.preventDefault();

    var blogPost = this.state.postbody;

    $.ajax({

      type: 'POST',
      url: '/api/blogPosts',
      data: { postbody: blogPost }
    });

    $.ajax({
      type: 'GET',
      url: '/api/blogPosts',
      success: function(data) {
        this.setState({posts: data});
      }.bind(this)
    })
  },

  handleChange: function(event) {

    this.setState({ postbody: event.target.value})
  },

  render: function() {
    return (
      <div>
        <form action="/api/blogPosts" method="post">
            <input onChange={this.handleChange} type="text" name="postbody"></input>
            <button type="button" onClick={this.handleClick}>Submit</button>
        </form>
        <List array={this.state.posts} />
      </div>
    )
  }
})
\$\endgroup\$
4
\$\begingroup\$

I'll start from what I perceive as "top" and work my way down.

<div>
  <form action="/api/blogPosts" method="post">
    <input onChange={this.handleChange} type="text" name="postbody"></input>
    <button type="button" onClick={this.handleClick}>Submit</button>
  </form>
  <List array={this.state.posts} />
</div>

You'd probably want to abstract the <form> into its own component. You can then expose a submit event, essentially just a callback handed down to a prop on the component. Your form component will call that callback, passing in the details you need for your AJAX.

Additionally, List and array don't really tell me anything. What kind of list is it? What's in the array? Better name them properly before you start scratching heads and lose hair.

Also, <input> doesn't need a closing tag.

// Better
<div>
  <PostForm onSubmit={this.handleSubmit} />
  <Posts posts={this.state.posts} />
</div>

componentDidMount: function() {
    $.ajax({
        type: 'GET',
        url: '/api/blogPosts',
        success: function(data) {
            this.setState({
                posts: data
            });
        }.bind(this)
    })
},

handleClick: function() {

    event.preventDefault();

    var blogPost = this.state.postbody;

    $.ajax({
        type: 'POST',
        url: '/api/blogPosts',
        data: {
            postbody: blogPost
        }
    });

    $.ajax({
        type: 'GET',
        url: '/api/blogPosts',
        success: function(data) {
            this.setState({
                posts: data
            });
        }.bind(this)
    })
},

I see that your GET request is redundant here. You can put that in a separate function, say loadPosts. Your functions don't really tell me anything either. Sure, they're handling a click, or respond to a mount but what are they doing?

Also, I see you're using jQuery. POST and GET requests have shorthand versions, $.post and $.get respectively.

When you handle the click, you call two AJAX operations in sequence. That sequence isn't guaranteed, especially when network latency is introduced. By the time you launched the GET, the POST might not have finished yet, and your GET return an incomplete list. It could also be that your POST succeeded, but GET failed. Usually, a POST request returns an updated version of the resource. Use that to just patch in the new data instead of retrieving the entire list.

Also, it's recommended you use the Promise interface of jQuery operations. tl;dr: use then instead of success, error, done or fail.

Since you're on React, I assume you use a transpiler setup. You can take advantage of new ES6 syntax, like arrow functions, shorthand literal assignment, shorthand literal functions

componentDidMount(){
  this.loadPosts();
},
onSubmit(post){
  this.submitPost(post);
},
loadPosts(){
  return $.get('/api/blogPosts').then(posts => this.setState({posts}));
},
submitPost(postBody){
  return $.post('/api/blogPosts', {postBody}).then(post => this.state.posts.push(post));
}

Now if your API can't handle returning the updated resource from a POST, you can simply call your GET after the POST suceeds.

submitPost(postBody){
  $.post('/api/blogPosts', {postBody})
   .then(() => this.loadPosts())            
}

var List = React.createClass({

  render: function() {
    return (
      <div>
        {this.props.array.map(function(post) {

          return (
            <Item postbody={post.postbody}></Item>
          )
        })}
      </div>
    )
  }
})

Usually when I see nested functions, I just move out the function into the class instead of inline. Much cleaner, and less visual clutter. Also, Item doesn't really tell me what it is. What kind of item is this? If this is a post, then it should be named Post or BlogPost.

var List = React.createClass({
  createPost(post){
    return <BlogPost postbody={post.postbody}></BlogPost>
  },
  render() {
    return (
      <div>
        {this.props.array.map(this.createPost)}
      </div>
    )
  }
});
\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your input. I'm having one issue. I've updated the form as you suggested and passed handleSubmit down to the PostForm as a prop. However, when I press submit, a long pause occurs and then the console outputs: Uncaught RangeError: Maximum call stack size exceeded. \$\endgroup\$ – jro Nov 20 '15 at 8:54
  • \$\begingroup\$ I console.log(postbody) on the first line of the submitPost function and it outputs this: SyntheticMouseEvent {dispatchConfig: Object, dispatchMarker: ".13vq11sfpq8.0.0.1", nativeEvent: MouseEvent, target: button, currentTarget: button…} . It then times out. I'm also wondering how just based on that function it knows to grab the text inside the textbox. \$\endgroup\$ – jro Nov 20 '15 at 9:17
  • \$\begingroup\$ The only solution I can think of is this: onSubmit: function() { this.submitPost(this.state.postBody); }. And then of course passing down the handleChange function down to the PostForm component. So as someone types it calls on the handleChange function, which then updates the state in the main component. Then once the user clicks submit, the onSubmit function just grabs the input from the state (this.state.postBody). What would be a better solution? \$\endgroup\$ – jro Nov 20 '15 at 9:33
  • \$\begingroup\$ @JonRoby Idea was to actually avoid updating the state with onChange and instead, use submit to update it all at once. It's not like anything else is dependent on a live copy of the text. What you're technically doing is two-way binding (updating state upwards) which is what React and Flux tries to avoid. Consider using Flux if you start to share state. \$\endgroup\$ – Joseph Nov 20 '15 at 13:06
  • \$\begingroup\$ When I do it the way I had before, I have the problems mentioned in the first two comments I made. Do you know why this might be occurring? \$\endgroup\$ – jro Nov 20 '15 at 22:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.