titouancreach / vuejs-redux

Flexible binding between Vue and Redux
57 stars 11 forks source link

mutation should not be allowed #14

Closed moparlakci closed 6 years ago

moparlakci commented 6 years ago

when I do a JavaScript map in the mapStateToProp Function it changes the original object in the redux store, bad bad bad...

I have something like this

`mapStateToProps(state) { // map userdetails to each post

            const userList = state.users.items;        
            const posts =  JSON.parse(JSON.stringify(state.posts.items)); // if I don't do JSON trick it will cause an infinite circular loop ==> then my browser crashes
            const postsWithUserDetails = posts.map((x) => {
                //console.log('post'); 
                x.user_details = userList[x.user] ?  userList[x.user] : userList[-1];

                if(x.comments.length > 0) { 

                    const comments = x.comments.map((y) => {
                        //console.log('comment');
                        y.user_details = userList[y.user] ? userList[y.user] : userList[-1];

                        return y; 
                    });  
                    x.comments = comments; 
                }

                return x; 
            });

            //   
            console.log('postsWithUserDetails', postsWithUserDetails); 

            return { 
                posts: postsWithUserDetails, 
            }`

Is there a better way for doing this ?

titouancreach commented 6 years ago

Hi, @moparlakci

The problem here is that you are doing mutation in mapStateToProps (you already noticed it). The point of mapStateToProps is just to "bind" part of the store to your UI component. Let's try to be change your function:


const getComments = (comments, userList) => {
  return comments.map((comment) => {
    return {
      ...comment, // inject the actual comment
      user_details: userList[comment.user] ? userList[comment.user] : userList[-1]; // override the user_details
    };  
};

const mapStateToProps = state => {
  const userList = state.users.items;        
  const posts =  state.posts.items // JSON tricks should not be necessary, the problem is elsewhere
  const postsWithUserDetails = posts.map((post) => { // I changed x to post (clearer for me :))
    return { // return a new object
      ...posts, // inject the actual content of post
      user_details: userList[post.user] ? userList[post.user] : userList[-1], // rewrite user_details
      comments: getComments(post.comments, userList)
    };
  });
  return {
     posts: postsWithUserDetails, 
  }
}

Not sure if it's exactly the result you are expected, but I don't now really how your state is internally. I just extracted your const comments = x.comments.map((y) {...} from the main loop because it easier to read. Now your function is pure, and you can use it in mapStateToProps and it's "good good good" :)

Note I used the object spread operator (this is a esNext feature). If, for any reason, you can't use it, you can use Object.assign instead.

Let me know if you have any issues,

Thanks for the interest you show in this project,

Titouan

moparlakci commented 6 years ago

Hi @titouancreach

Thanks for you detailed explanation, now it works very well! :)

I was having a circular loop which caused my browser to crash. (because the state was changing every second, and the view was updating etc etc etc..)

I thought 'map' was returning always a new object, because of that I didn't use the spread operator, it really confused me.

Now my question is, should I always use the spread operator first before I do a map with redux, I think I do.

I also use javascript 'filter' in my code, do this also change the state, then?

   `mapStateToProps(state) {
            /* state.commentCrud = [
                    {
                         post: 1,
                         message: 'this is a comment crud for post ID 1'       
                    },
                    ...
            ]*/
            return {
                newComment: state.commentCrud.filter(x => x.post === postFromProps._id)[0]
            }  
        }`
titouancreach commented 6 years ago

Map is a very simple function, if you do : [x, y, z].map(f) , the result is : [f(x), f(y), f(z)] . If f is the identity function, const f = x => x , the result is equal to your starter array. No new array is created .

Remember that your mutations were introduced by the assignation (i.e "=" operator). If you don't use it, you don't need to create a new object.

Your filter is good! You don't mutate anything (no "="). Since newComment contain your state (no new object). You should definitely not modify newComment.something in your UI component. Hopefully, Vue will warn you if you try to mutate a prop!

You can also replace your filter by the Javascript find function that do out of the box what your filter and [0] do!

Good luck!

titouancreach commented 6 years ago

I close this issue because it seems answered! Don't hesitate to reopen if needed 😀