Framework: Add sync-handler wrapper

[retrofox]

This adds a wrapper to sync the data gotten from the REST-API GET responses.
The idea with this data storing is allow provide data when the user is offline.

This wrapper works like a middleware that intercepts every GET request from the user and does, in short, three actions:

1. Search the data locally in response immediately if this exists.
2. At the same time send the request to WordPress REST-API server.
3. Update the WP response locally

This approach is not ready to production. This feature is allowed through of its sync-handler config key.

[retrofox]

An screenshot of where and how we are storing the data.

[retrofox]

chrome console setting localStorage.debug to calypso:sync-handler*

[artpi]

I would be for splitting it into if {} else
We have 4 different permutations here, so I would do

if( config.isEnabled( 'sync-handler' ) ) {
const req... = ...
} else {
const....
}

and move it to line 16, then we dont have to duplicate the code in line 19 & 25

[retrofox]

The thing here is you also need to decide what request handler will be wrapped by sync-handler. It's a double nested condition. And the wpcom instantiation is different accordingly.

if ( config.isEnabled( 'oauth' ) ) {
  // ...
  if( config.isEnabled( 'sync-handler' ) ) {
    const req... = ...
  } else {
    const....
  }
} else {
    // ...
  if( config.isEnabled( 'sync-handler' ) ) {
    const req... = ...
  } else {
    const....
  }
}

I can add a const to get the config key to avoid call the config() function twice.

[artpi]

This ternary seems to be duplicated in line 20 and 26 :)
Move it to 17 maybe?

[skeltoac]

It isn't duplication: the requires are different.

Not sure if it's correct, though. The first block has lib/wpcom-xhr-wrapper both times. The second block has wpcom-proxy-request and then lib/wpcom-xhr-wrapper.

[retrofox]

yup. you're right.

[seear]

I've given this a quick test with /menus. Client menu data is updated with responses from POST requests, so everything works fine unless the page is refreshed, when an initial population is performed from stale GET data, meaning any previous modifications are not visible.

Maybe we should add a blacklist facility to allow us to indicate that certain API endpoints should never be cached?

[retrofox]

Maybe we should add a blacklist facility to allow us to indicate that certain API endpoints should never be cached?

Yes, in fact we did it in our first approach. a143572

And on the other hand we're trying to improve the performance when the local data must be updated and at the same time find a way to notify to client( calypso) of these changes.

[rralian]

Client menu data is updated with responses from POST requests, so everything works fine unless the page is refreshed, when an initial population is performed from stale GET data, meaning any previous modifications are not visible.

@retrofox I think the double-callback branch should be merged into this PR. The stale data issue is likely enough of a blocker that this PR should not be merged without it, even in development. The odd behavior with stale data is otherwise likely to confuse developers as they are working on their features. We should also add a blacklist mechanism if/when we find an endpoint for which the double-callback approach runs into problems.

[retrofox]

ok @rralian

[retrofox]

This is an example of a malfunction using the double callback. Repro: go to http://calypso.localhost:3000/menus/your-testing-site

[gwwar]

@retrofox you may want to rebase with master to work around build breakages.

[retrofox]

thanks @gwwar

[retrofox]

I can look into that. I'll see if it makes sense to do so in this PR or against master.

I really would like try to do it on this PR.

[gwwar]

It occurs to me that we may need schema validation for this cached GET request. While we require a version bump for an endpoint whenever attributes change or are removed, we allow attributes to be added to an endpoint for a given version.

Hmm, additive properties that we expect could be a mess. Since this is a generic low level cache, I'm not sure if we could target all the endpoints. Probably ~1d max age might make more sense.

[rralian]

👍 Looking good, but here's one case:

Ha! After a fair bit of debugging I figured out the issue was not quite what it seemed. The dirty cache to clear the post-list and the second callback where all working as expected. The issue is that receivePage always expected we would be appending new posts to the end of the list as we scroll down. So your post was getting added, just at the end of your post list, rather than the top. So we just needed to sort the list. Added a commit to do so. 😄

[rralian]

@retrofox I would prefer that we not add the reader endpoint until/unless we can confirm it doesn't have the same corner case issues we found for the post-list. Let's just keep this one simple, we can add the reader endpoints separately.

[retrofox]

@retrofox I would prefer that we not add the reader endpoint until/unless we can confirm it doesn't have the same corner case issues we found for the post-list. Let's just keep this one simple, we can add the reader endpoints separately.

👍

[rralian]

This is working well for me, though it looks like this needs to be rebased again. There are still some issues we need to figure out, but I think we can merge this now, especially considering this is limited to development. 👍

[retrofox]

rebased. waiting for circle cli as well.