Framework: Introduce top-level Redux state directory with sites example

[aduth]

Update: This pull request was originally targeted toward re-working sites-list, but it has since grown to include changes for adopting a new folder structure approach with regards to our Redux global state. Specifically, all files related to the global state tree are now contained in a new top-level client/state directory. The directory reflects the structure of the global state object itself, e.g. state.sites in the global state maps to files contained in client/state/sites.

Many other guidelines are yet to be developed. See #1378 for progress in crafting a recommendations document.

---

Blocks #756

This pull request seeks to initiate the effort to refactor the existing sites-list module to reflect our current data management recommendations. The changes herein are minimal in that the only features ported are actions to set a selected site and to receive a site. A follow-up pull request intends to dispatch these actions in the My Sites siteSelection controller middleware function, thus making the selected site available from within global Redux state.

Refer to the module README.md for more information.

Testing instructions:

There are no user flows which will cause the added modules to be imported. Therefore, there are no effective changes in the application. Verify instead that the included tests pass by running make test from the shared/lib/sites directory.

[aduth]

See #756 as an example of how this is intended to be used. #756 is the follow-up pull request mentioned in the original message here.

[blowery]

actually returns an object?

[aduth]

actually returns an object?

Whoops, bad copy-paste. Fixed in 8d8efb9.

[jkudish]

This looks good and works well for me, tested via #756

[aduth]

After some ideation with @mtias , I've force-pushed a rebase with some significant revisions to how we approach a Redux global state. As such, the purpose of this pull request has changed to be a proposed structuring of the global state tree, with sites state being an example implementation therein.

A summary of changes:

• All files related to the global state tree are now contained in a new top-level client/state directory. The directory reflects the structure of the global state object itself, e.g. state.sites in the global state maps to files contained in client/state/sites.
• Management of behavior related to the currently selected sites has moved to a sub-tree of the global state concerned with user-interface related state (client/state/ui). The intent of this sub-tree is to capture all state that should be preserved across page transitions. Not all UI state is appropriate for the global state tree, and using a React component's own state still makes sense in many situations where the long-lived persistence is not necessary.

Many other guidelines are yet to be developed, particularly around:

• Container components
• Data fetching (including asynchronous actions)
• Scope of "selectors" (resolving references, derived/computed properties)

Open questions:

• In this new state structure, should we have a canonical set of available action types in a single file, or should these remain the concern of each sub-tree? A single file is likely to result in regular merge conflicts, though might help to serve as a helpful reference.

[lezama]

We should put the initial state on a separate variable as in the precedent test.
I think we could check deep-freeze( https://github.com/substack/deep-freeze ) on it as a good practice when testing reducers.

[aduth]

We should put the initial state on a separate variable as in the precedent test.
I think we could check deep-freeze( https://github.com/substack/deep-freeze ) on it as a good practice when testing reducers.

In ec191a9, I've changed the tests such that the original value is always frozen using Object.freeze. deep-freeze is a fine option too, though in this case I know that my object is not deep, so Object.freeze is sufficient.

[lezama]

Thanks for putting this in place @aduth, it is a great redux example!

[lezama]

In other parts of the codebase we are using keymirror, should we do the same here?

[blowery]

I'd really like to stop using keymirror if we can. I don't see any real advantage over const refs.

One of the points of using it is that some optimizers can crush the keys — see facebook/react#1639 (comment). This means that the action values we'll see in prod will be nonsense, which removes one of the great things about redux, the ability to replay the action queue and make sense of what happened.

[lezama]

Thanks for the response @blowery, I now agree that we should stop using it :)

[aduth]

Maybe getSelectedSite should receive two params with state.sites and state.ui.selectedSite instead of the entire state object?

@lezama : I believe this was touched on briefly in our last hangout, though I think we should standardize on always passing the entire state object to a selector, for the following reasons:

• Consistency
• Readily accessible, especially in the context of react-redux connect mapStateToProps or by calling getState() on the store instance
• We should anticipate that there are cases when cross-tree referencing is suitable. For example, I might expect that a theoretical posts/selectors.js would include a getPostComments() function which would need to pull from both the posts and comments root state keys.

In case that last point isn't entirely clear, here's an example tree and implementation:

{
    posts: {
        items: {
            100: {
                comments: [ 1 ]
            }
        }
    },
    comments: {
        items: {
            1: {
                content: 'Hello, World!'
            }
        }
    }
}

export function getPostComments( state, postId ) {
    return state.posts.items[ postId ].comments.map( ( commentId ) => {
        return state.comments.items[ commentId ];
    } );    
}

[artpi]

@aduth, @mtias : I will change this in #1393 to export not the function, but singleton - already instantiated store.
context.store is not helpful at all and deep in some component that is not reduxified yet I need to call notices.

I dont want to reduxify the whole app yet, but notices are created from virtually anywhere, so I kinda need something to import to actually create a notice.

[aduth]

I will change this in #1393 to export not the function, but singleton - already instantiated store.

I disagree with this move. It's important that stores are instantiated on the fly. This goes back to server-side rendering concerns about having a singleton instance being reused between requests. On the server-side, we'll need to create a new store for every request. If you need access to the store from elsewhere in the app, you should be able to pass it along from the route context value, without the need for pulling it in via module import.

[aduth]

A more practical example: If a component at an arbitrary point in the render hierarchy needs to display a notice, it's a good opportunity to use react-redux connect's second argument, mapDispatchToProps.

import React, { Component } from 'react';
import { bindActionCreators } from 'redux';
import { connect } from 'react-redux';
import { newNotice } from 'state/notices/actions';

class MyComponent extends Component {
    render() {
        return (
            <button onClick={ this.props.showNotice }>
                Show Notice
            </button>
        );
    }
}

export default connect( null, ( dispatch ) => {
    return bindActionCreators( {
        showNotice: newNotice
    }, dispatch );
} )( MyComponent );

[artpi]

That is correct, but we have:

194 matches across 84 files

Not every single one of this 84 files is a component, but I'm guessing that would mean connecting most of the components in the whole of calypso.
I can do that, but do we want to reduxify it so much?

[artpi]

And as for Server-Side rendering:
What sense does keeping an application state make for SSR? How do we match state of store with the url to serve a page with a different state?
I'm kind of confused by this, because I was quite certain SSR needs only to match the URL and will not need to mutate the store.

[artpi]

Please, lets continue here :)
#1393 (comment)

[artpi]

Conclusion for future generations:
@aduth convinced me and we will try to connect components using react-redux.

[aduth]

It seems we're pretty well-settled on the conventions here and it's important to not let this block other ongoing Redux efforts. I've rebased and squashed the branch and will plan to merge shortly after tests pass.

[artpi]

Awesome!

[mtias]

🎉