Add redux comments store

[samuelclemens]

This PR lays some ground work for issues #2839, #967, and #792 .
Current status is that the code is mainly done, what left IMO, are some style adjustments and documentation.
Your opinion is appreciated.
#2839:

The comments pulled by date in DESCending order, while a tree of comments is being build in the store.
This is achieved by calling requestPostComments() action creator, subsequent calls will fetch the earlier (older) dated comments.
#967:

The comments are built in a tree-like structure, that allows us to see in each comment which are it's children and how many children the comment has.

It can be used something like that:

const commentChildrenIds = this.props.comments.getIn( [ this.props.commentId, 'children' ] ).toJS();

if ( commentChildrenIds.length > 0 ) {
    if ( this.state.viewReplies ) {
        return <ol>
                { commentChildrenIds.reverse().map( ( commentId ) => <Comment ... /> ) }
                </ol>
    } else {
        return <button onClick={ this.viewRepliesClickHandler.bind( this ) }> view { commentChildrenIds.length } replies</button>;
    }
}

#792: Polling can be done by invoking `pollComments()` action creator ( from `` maybe? ) after first `requestPostComments()`, it'll fetch the new comments ( comments that got approved and meet the time-frame and entirely new comments ) and remove comments that got unapproved.

It is very inefficient however, and it also can't detect edited comments.

No polling support in this PR.

[nb]

In which cases totalCommentsCount can be -1 (or less)?

[samuelclemens]

Most of the times the api will return found === -1 (don't think it can be less than -1, -1 is to indicate "unable/error" I guess.), since I use before filter on the request to fetch more comments, whenever a filter is used, api would not return the total comments count.
The only time it returns the count is when the request is first made without any filters.

[blowery]

We just landed a move to lodash 4.x, so we'll need to rework this to take that into account. See https://github.com/lodash/lodash/wiki/Changelog#compatibility-warnings for a list of changes.

[gwwar]

How about updateCommentState( state, id, updaterOrValue )? Since we're only using action to generate an id, it makes it clearer if we change the function signature to accept the id instead. It should make mocking the tests a bit shorter too.

[samuelclemens]

The reason I've done it like that is because I didn't want to call createCommentTargetId( action.siteId, action.postId ) in every reducer and capture the logic of getting the specific state (and the generation of it's ID) in a single place, It is after all a helper function =)

[gwwar]

If every handler uses the id, you can create the id before the switch statement.

[samuelclemens]

@gwwar Every reducer needs the id, not just every handler at the switch...
Because of the structure we chose.

[samuelclemens]

I'll elaborate a bit more.

If I understand you correctly, you're suggesting factoring out the id generation from the updateSpecificState function,

What it'll mean in my opinion, that in each reducer ( items, requests, totalCommentsCount ),
i'll have to repeat ID creation:

function items( state, action ) {
    const id = createCommentTargetId( action.siteId, action.postId ); // DRY
    switch ( action.type ) {
    ...
    }
}

in my opinion, it's better not to repeat.

[gwwar]

Passing exactly what the function needs, helps keep the intent clearer and makes refactoring easier. This is a helper function, so not a blocker here.

[gwwar]

I'm still 👀 I'll try to finish up tomorrow.

[samuelclemens]

@gwwar Thanks for reviewing that!

[blowery]

You could probably do the single item case with findIndex and updateIn. Where do you use the multi item case?

https://facebook.github.io/immutable-js/docs/#/List/findIndex
https://facebook.github.io/immutable-js/docs/#/Map/updateIn

[samuelclemens]

I don't use the multi case.
It's still 2 not chain-able method calls I need to encapsulate in some function.

You're suggesting remove the multi case?

[blowery]

You're suggesting remove the multi case?

If we're not using it, yeah. No reason to have it.

It's still 2 not chain-able method calls I need to encapsulate in some function.

Having the function is fine. Just thinking we can simplify it and use more idiomatic immutable methods.

Maybe for a name updateExistingIn? Given how Immutable works, this could really work for List or Map.. It's pretty much like updateIn, but it wouldn't create missing keys.

[samuelclemens]

Sure, I'll make that later tonight.
Update: done

[gwwar]

If we want to persist this state tree, we'll want to add a JSON Schema for each subtree. We can also defer this work by temporarily choosing to not persist state, and add it in a future PR.

case SERIALIZE:
    return {};
case DESERIALIZE:
    return Immutable.Map();

The documentation update is still open, but if you're interested you can read some of the reasoning here: #3267

Let me know if you need help with adding schemas.

[blowery]

I'd hold off on persisting this bit of the state tree for now.

[samuelclemens]

I've removed the serialization.

[gwwar]

👍 Great progress! Before merging

• Wait on Framework: Add create-selector library for memoized selectors #3545 or Framework: Use memoized selectors, and simplify state/posts #3522 to land. There may be a little bit of refactoring if Framework: Add create-selector library for memoized selectors #3545 lands.
• Don't persist the subtree, or add schemas https://github.com/Automattic/wp-calypso/blob/master/docs/our-approach-to-data.md#data-persistence--2754-

[blowery]

Looks like #3545 landed, so we should switch over to that from reselect.

[samuelclemens]

Migrated to #3545, blocked by #3627 or alternative.

[samuelclemens]

#3627 is merged, so this PR should be good to go imo.

[blowery]

Looks good to me. @gwwar any objections to landing this?

[gwwar]

👍 🚢

[blowery]

🚢 🎉 🎈