Posts: Convert PostSelector to using posts Redux state

[aduth]

Related: #303, #2248

Edit: The purpose of this pull request has changed significantly. Refer to this gist for the original pull request content.

This pull request seeks to convert the existing <PostSelector /> component to make use of the global Redux post state. It introduces a new <QueryPosts /> component for managing requests to the REST API, and implements the prerequisite pagination behavior to the posts state.

Implementation notes:

An advantage of the new posts state is that previous post queries are cached, so if a user enters a search term, then clears their query, the component no longer needs to make another request for the original posts.

The <QueryPosts /> component is the first example of a proposed fetching components pattern in managing the fetching of data to be accessed via global application state by sibling components.

Testing instructions:

Confirm that there are no regressions with the parent page selector in the Calypso post editor, including infinite scroll pagination, search queries, and lack of results/pages.

1. Navigate to the Calypso page editor
2. Select a site, if prompted
3. Expand the Page Options sidebar accordion
4. Note that available pages are shown, ordered ascending by title, excluding the current page (if published)
5. Note that you can scroll the parent page selector for infinite scroll if many pages exist (more than 20)
6. Note that entering a search term limits the shown posts to the filtered search term

Also confirm that Mocha tests pass by running make test from the project root directory or directly from client/state/posts.

[mtias]

The component is the first example of a proposed fetching components pattern in managing the fetching of data to be accessed via global application state by sibling components.

I like this from the how-it-works side, and how it turns PostsTypeahead a drop-in app components that just works. Which is great. I'm a bit concern with the obscurity of it, though, specially since glancing at the internals of PostsTypeahead I can't quickly figure out who is actually dispatching the magic.

I think this could be solved (or at least improved) with better conventions and naming. For instance, PostsSearch should have something in its name that reflects the fact it doesn't render UI at all. Maybe we can pick some word to prefix these components, like <Query...>, so whenever you spot one of these you immediately know their purpose and role.

[mtias]

It'd be nice to support cross-site results here.

[aduth]

The intent of this pull request has changed significantly. I've left the original commits in case we want to recover some of the <Typeahead /> component, but the goal of this pull request is no longer to implement a new component but rather to convert the existing <PostSelector /> component to use Redux state.

This required a handful of changes to the posts Redux state to better manage pagination.

[aduth]

@timmyc : If you have a spare moment, would you mind taking a pass over the changes to the <PostSelector /> component?

[timmyc]

Reading the code it looks good, let me run some tests on the branch and see how it goes. One thought though, is the query key case-sensitive, i.e. would searching for "Hello" vs "hello" yield two different query keys and spawn multiple api calls?

If so would it be safe to downcase query strings before searching / creating the key?

[aduth]

One thought though, is the query key case-sensitive, i.e. would searching for "Hello" vs "hello" yield two different query keys and spawn multiple api calls?

That's a very good point. Another potential issue is that explicit requests for default values will not be smart about reusing prior results, e.g. {"search":"Hello"} vs {"search":"Hello","type":"post"}.

I'll take a look at improving how these keys are serialized to remove default values and lower-case. Thanks!

[aduth]

Pushed f0e0592 and 8df0cb5 to account for the two issues noted above.

[timmyc]

Changes look good, and good call on stripping out the default query values too.

One other question: Should/can client/my-sites/post-selector/pagination.jsx ultimately replace all use-cases of the post-list-fetcher?

[aduth]

Should/can client/my-sites/post-selector/pagination.jsx ultimately replace all use-cases of the post-list-fetcher?

Yeah, the plan is that <PostListFetcher /> can be deprecated by <QueryPosts />.

[timmyc]

Tested out the post-selector on the page editor and all seems well. It does seem ( though it could just be local env, or a slow connection ) that the initial load of the posts is slower than the old fetcher approach.

But this is LGTM and a big improvement over the list-approach.

[timmyc]

... definitely my internet connection ( as my last comment failed to post to github on the first try ) :)

[mtias]

Can we make siteId determine whether to fetch single site or all sites?

[aduth]

Can we make siteId determine whether to fetch single site or all sites?

I think that's a reasonable enhancement, though also requires additional action creators in the sites state to handle fetching posts from /me endpoints. Seems appropriate for a subsequent pull request.

[aduth]

One "issue" with how persistent the cache is here is that there's never a point at which the cache is invalidated. For example, if I were to publish a page and then start drafting another, I'd not be presented with the option to choose the first page as the parent page unless I triggered a full-page refresh.

I think this will be an interesting challenge moving forward across Redux stores. Especially as we consider persisting the Redux state to browser storage, we'll want to think through cache-busting techniques.

This pull request is quite large already, but I've considered a few options for resolving this particular need:

• Invoke an action, either at the start of a new session, or upon routing navigation, that signals that caches should be busted
• Create a separate reducer key tracking a timestamp at which a particular query result was received, and in the <QueryPosts /> logic for deciding to fetch, it should consider whether the result is stale given a defined TTL
• Capture actions which would invalidate the cache (e.g. POST_PUBLISH) and either destroy the cache or update the cache to include the modifying item
  • Publishing actions are not currently handled in Redux. See related proposal at Framework: Proposed mechanism for replaying Flux actions to Redux #2167

The first of these is probably the most minimal approach if we were to try to tackle a solution in this pull request.

[aduth]

Added reset action in 32703e5, dispatched when navigating to the editor route in b06b979.

Appreciate the reviews and apologies again for how large this pull request became. I'll plan to merge this in the morning unless either of you'd like more time to look over the latest changes.

[blowery]

How does lowercasing the query string work for non-English / far east queries? Maybe it's worth investigating using localeCompare to build a sorted array of queries instead of relying on lowercase.

[aduth]

How does lowercasing the query string work for non-English / far east queries?

The lower-casing doesn't affect the actual query to the REST API, but instead is used only as a reference for lookup. As long long as it results in a consistent and unique string, there shouldn't be a problem. We could/should probably be using toLocaleLowerCase() at the very least, however.

[rralian]

Added reset action in 32703e5, dispatched when navigating to the editor route in b06b979.

@aduth calypso core is working on allowing for creating a new post while offline in the desktop for our meetup project. As part of this we were planning to persist global state, including the posts-list data. And we were thinking we would store some sort of ephemeral post object with some "offline" flag into the posts-list redux store. That way we could show the new post within the context of the draft drawer. But if we're busting the posts-list cache when you hit the editor, that sorta negates the plan. Could we instead trigger this on a post create, update, or delete? I understand we'd need to merge #2167 or something like it in order to sniff out those flux actions and replay them as redux actions. Are there other concerns with that approach that I'm missing?

[aduth]

@rralian : Summarizing our chat conversation:

• Calling an action to clear the query cache on routing is a bandaid, and should be replaced by a better, more predictable mechanism, such as upon a post being published.
  • These actions are currently dispatched in Flux, and as such are not easy to hook in to
  • We also need to consider how to invalidate caches that are "stale" from previous sessions
• I worry about the implications of including the in-progress post data in state.posts / state.posts.items.
  • It'll be difficult to remediate this with the REST API, though perhaps this is a challenge we should be taking on, so as to treat posts as equals regardless of source.
  • Further, it's not as obvious how to reset a post to its original state in the application if revisions are abandoned.
  • Instead, I envisioned the transient post state, whether for a new post or editing an existing, living as a separate object consisting of the changes, likely at state.ui.editor.editedPost. This is similar to the approach I'm taking with media at Media: Populate advanced editing with parsed media #2541. This is easy to abandon and merge with "persisted" post state at state.posts.
  • Challenges with this latter approach: can one have multiple in-progress drafts (perhaps editedPosts is a map)? How do we retrieve all posts for display in draft drawer, draft or otherwise (since they'd be split between state.posts and state.ui)?