Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

State: Split post query fetching status from result mapping #3671

Merged
merged 2 commits into from
Mar 1, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Feb 29, 2016

Follow-up to #3487 (comment)

This pull request seeks to split the posts.queries state subtree, which currently tracks both fetching status and response results for a query, to separate subtrees (posts.queries and post.queryRequests). Benefits include:

  • Fetching status should not be persisted, whereas query results should. Separating them makes this more feasible.
  • Simplifies structure of state tree, removing need to check for whether query is "tracked" before return result from selector. Also leads to simpler-to-follow reducer logic and decreased nesting (fewer Object.assign).

Testing instructions:

Verify that there are no regressions, particularly around the <PostSelector /> component, and ensure that fetching status is properly tracked such to avoid unnecessary excessive requests.

Repeat testing instructions from #2350.

Ensure Mocha tests pass by running make test from client/state/posts.

Verify state persistence behaves as expected. See #3671 (comment) .

Follow-up tasks:

  • Rename posts.query to posts.queryResults
  • Rename posts.queriesLastPage to posts.queryLastPage
  • Add tests to ensure that reducer keys are included on the default export (easy to miss)

/cc @gwwar

@aduth aduth added Posts Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 29, 2016
@aduth aduth force-pushed the update/redux-post-queries-fetching branch from a029ded to 70ffb2e Compare March 1, 2016 16:52
@aduth
Copy link
Contributor Author

aduth commented Mar 1, 2016

@gwwar : This has been rebased to account for the merging of #3487, and also includes a posts query schema in 70ffb2e (validating JSON strings with RegEx is... fun 😉 ).

@aduth
Copy link
Contributor Author

aduth commented Mar 1, 2016

Notably, now that a schema is included, one should also test that schema persistence behaves correctly for query results.

  1. Start Node process with ENABLE_FEATURES=persist-redux make run
  2. Navigate to <PostSelector /> app components demo
  3. Enable persistence debugging by entering localStorage.debug = 'calypso:state' in your developer tools console
  4. Refresh the page
  5. Note that persisted state is restored, including posts.queries, without validation warnings

patternProperties: {
// Queries are JSON strings, optionally prepended by a site ID. We can
// assume that query strings contain no spaces, and only simple values.
'^(\\d+:)?\\{("[^"]*":("[^"]*"|\\d+|true|false),?)*\\}$': {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun 😄 Since we don't parse this later I'm not sure if we need to be this strict.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't parse this later I'm not sure if we need to be this strict.

My main concern is that there's a real possibility that we change the syntax of the keys. I suppose I could be a bit more lax about it, e.g. \{[^\}]*\} instead of the more complete JSON validation I have here.

@gwwar gwwar added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 1, 2016
@gwwar
Copy link
Contributor

gwwar commented Mar 1, 2016

👍 Changes look great. I like how the tree is being simplified. I verified that testing instructions worked, and also tested that the editor still loads properly when post/items data is thrown out. 🚢

@gwwar
Copy link
Contributor

gwwar commented Mar 1, 2016

Also, persist-redux is now enabled on dev/horizon. So to test the alternative pathway, it would be:

DISABLE_FEATURES=persist-redux make run

@aduth aduth force-pushed the update/redux-post-queries-fetching branch from 70ffb2e to a83c638 Compare March 1, 2016 19:08
@aduth
Copy link
Contributor Author

aduth commented Mar 1, 2016

@gwwar : Rebased a83c638 includes a slightly more relaxed property pattern. Also includes a test to ensure that invalid persisted state is discarded. Tested as well with disabled persistence. Will plan to merge shortly after tests pass.

aduth added a commit that referenced this pull request Mar 1, 2016
…tching

State: Split post query fetching status from result mapping
@aduth aduth merged commit 341cfd3 into master Mar 1, 2016
@aduth aduth deleted the update/redux-post-queries-fetching branch March 1, 2016 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants