Framework: Use memoized selectors, and simplify state/posts

[gwwar]

Update: This PR is closed because posts.sitePosts was never in use.

This PR shows how we can use reselect 'lib/create-selector' (#3545) to create memoized selectors. Why do this?

• Selectors can compute derived data, allowing Redux to store the minimal possible state.
• Selectors are efficient. A selector is not recomputed unless one of its arguments change.
• Selectors are composable. They can be used as input to other selectors.

Also see: http://redux.js.org/docs/recipes/ComputingDerivedData.html

My primary motivation is to keep our state as minimal as possible, and to move any derived data into memoized selectors. This avoids a lot of complicated use cases in persistence where data may get out of sync with derived data, like a list of posts versus a map of posts indexed by siteId and postId.

This PR removes sitePosts from the redux tree and replaces it with a memoized selector.

Testing Instructions

The posts tree is currently used in the page editor to back the parent page selector.

• Make sure that your blog has some pages
• Navigate to the editor to add a new page
• Click on Page Options to make sure we can see parent page options
  
• Check that the redux tree looks correct
• Editor behaves normally

cc @aduth @blowery @dmsnell @rralian @artpi @retrofox

[artpi]

If we want to get reaaaaly fancy (not that it matters :) ) :

createSelector( getPosts, posts => keyBy( posts, 'global_ID' ) );

[gwwar]

I initially went for that, but it goes past the 80 char limit

[artpi]

I actually started implementing memoizing a bit, but you seem to be doing it better than I started :) Cool!

[artpi]

I didnt go deep into the reSelect library, but do we have a way to move createSelector call to the vicinity of connect ?
What I mean is, I liked the pure syntax of selectors and createSelector seems like some magical wrapper, just like connect.
Does it make sense or not so much?

[artpi]

I guess, not so much, because memoization would not be shared between different components....

[gwwar]

I think it makes most sense to keep it in the selectors file. If something seems like magic, we should take the time to read the source 😄 https://github.com/reactjs/reselect/blob/master/src/index.js

[aduth]

In my mind, this function would return an array of posts for a particular site ID, passed as an argument. This does not seem straight-forward to achieve in reselect as they (intentionally) do not make it easy to create memoized selectors which accept arguments. Have you thought through how this might work? Perhaps a separate function which calls on the memoized site posts selector (getSitePosts, getPostsBySiteId)?

To the earlier discussion, I'd like to think that reselect selectors could live alongside our "pure functions" approach, but some of the opinions of the library clash with ours enough that I worry about the confusion it might cause to treat them as the same.

[gwwar]

In my mind, this function would return an array of posts for a particular site ID, passed as an argument

How about if we rename this to getPostsBySiteIdAndPostId. This was intended to replace the sitePosts section of the redux tree. Memoized selectors are not intended to be passed arbitrary elements, but we they can be composed in other memoized selectors or used directly in a simple one.

Perhaps a separate function which calls on the memoized site posts selector (getSitePosts, getPostsBySiteId)?

Exactly:

export const getSitePostsBySiteId() = createSelector( 
    getPosts,
    ( posts ) => {
        return groupBy( posts, 'site_ID');
    }
);
export function getSitePosts() {
   return getSitePostsBySiteId( state )[ siteId ];
}

but some of the opinions of the library clash with ours enough that I worry about the confusion it might cause to treat them as the same.

Perhaps if we think of the memoized selectors as things that could live in the redux tree, but happen to be computed.
A simple wrapper

function getSitePosts( state ) {
   return state.posts.sitePosts;
}

versus:

function getSitePosts( state )  = createSelector( 
   getPosts,
   ( posts ) => {
       //...
   }
}

Still has the same usage.

getSitePosts( state );

[aduth]

I like the general idea that sitePosts not have to be saved in the state tree, as it's essentially a duplication of data which can be derived from state.posts.items.

[aduth]

With #3545 having been merged, would be curious to see how this would look now with our own pattern of selectors. Specifically, I still think it's a good idea to rebase this pull request with the revisions to moving sitePosts out of Redux state and described as a memoized selector.

[aduth]

Can we add tests for this?

[gwwar]

:) sure, added in 560da3c

[aduth]

Bordering on a bit too clever, but I tried rewriting this with a few more Lodash helpers:

export const getPostsBySiteIdAndPostId = createSelector(
    ( state ) => {
        return reduce( state.posts.items, ( result, post, globalId ) => {
            return setWith( result, [ post.site_ID, post.ID ], globalId, Object );
        }, {} );
    },
    ( state ) => [ state.posts.items ]
);

https://lodash.com/docs#reduce
https://lodash.com/docs#setWith

[gwwar]

A tad clever, but straightforward enough. Added in c209df2

[aduth]

Testing instructions probably don't apply to these changes, as queries is responsible for mapping the post selector results to global IDs. From what I can tell, the getSitePost selector is not currently used in the application.

[gwwar]

From what I can tell, the getSitePost selector is not currently used in the application.

If it's not in use, is there any value to keeping this? I can remove sitePosts from the redux tree and remove the new memoized selector. There's a few other places where we can test adding memoized selectors. Some of the selectors in state/sharing/publicize/selectors would be good candidates.

[aduth]

We should add a beforeEach in the describe to ensure each test runs with a fresh cache:

beforeEach( () => {
    getPostsBySiteIdAndPostId.memoizedSelector.cache.clear();
} );

[gwwar]

If we want to add cache tests, I would suggest adding cases for a clean cache, and also other ones to test that the cache break works.

[aduth]

If we want to add cache tests, I would suggest adding cases for a clean cache, and also other ones to test that the cache break works.

I don't think we need to test caching, as we can assume this is already working as expected via tests in lib/create-selector/test/index.js . My point is more that each test should be run in isolation, and we cannot be certain of that unless we reset the memoized function back to its original state between each test.

[aduth]

If it's not in use, is there any value to keeping this?

Generally, I'd say no, that we should go ahead and remove it if it's not in use. I'm trying to recall if it was planned to be used in the near future, though if that's the case, it's easy enough to re-add. If you do decide to simply remove it from the state, I'd recommend preserving the history (not squashing to a single commit) so we can refer back later.

[gwwar]

Generally, I'd say no, that we should go ahead and remove it if it's not in use.

😄 That was my feeling too. I'll go ahead and close this PR and open up a fresh one to remove sitePosts.