Site-kit-wp: Modify apiFetch to only rely on server-preloaded data for the first request

[felixarntz]

Bug Description

We currently preload data for several API endpoints so that no requests on pageload are necessary:

• connection data for core/site store
• authentication data for core/user store
• module information for core/modules store
• settings for all modules/* stores with settings

There is a problem with that approach because the apiFetch middleware will always have these API requests resolve with this data, even later during the app lifecycle where things might have changed. For example, when you activate a module, core/user authentication data should be refetched. We don't use this anywhere yet so it's not critical, but in the current stage it will not work since it would still not issue an API request and instead rely on the server-preloaded data which at that point will be stale.

We should modify this behavior (e.g. by customizing the behavior of the middleware or potentially implementing our own version of it) so that it only uses the preloaded data for the initial request(s). It should just be one request, but in theory there could be a race condition where the data is requested multiple times. What would probably make sense is to "invalidate" the preloaded data after it is first used, potentially with some timeout of 500ms or so to cater for the race condition (maybe that's not even worth doing).

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

API preloaded data handling

• Server-preloaded data should only be used for apiFetch requests for the initial request(s). For subsequent requests, the data should be invalidated so that the actual API request will be fired.
  
  
  
  • Preloaded data should be invalidated as soon as it's used once for a request.
    
    
    
    
    
    • Theoretically this would be a problem if two similar requests were fired at about the same time, but the way that datastore resolvers work and the way we're using them make this practically irrelevant.
    
    
    
  
  
  • Since it could happen that no such request happens on pageload, we should set an arbitrary threshold of 1000ms (once we have a PR, let's test whether that value works well or whether we should adjust) after which preloaded data should get invalidated regardless of whether a request has been made that applies to it.
  
  
• Server-preloaded data should be used independently of whether a request uses caching or not (other than in the original IB and #1692); its only purpose is to avoid an API request right after pageload (which is common to be caused for all of the JS stores that rely on API requests where we preload data).

Module store adjustments

• The core/modules store should, as part of the setModuleActivation action rely on the core/user store's authentication infrastructure instead of just refetching a current API response.
• Module data should be preloaded on the server via the google-site-kit/v1/core/modules/data/list REST endpoint.

Implementation Brief

Custom preloading middleware

• Create a Site Kit specific implementation of @wordpress/api-fetch's preloadingMiddleware in assets/js/googlesitekit/api/middleware/preloading.js which exports a custom default createPreloadingMiddleware;
• In assets/js/api-fetch-shim.js, import createPreloadingMiddleware from our internal module, rather than the core non-shim module.

Custom preloading middleware implementation

• If there is preloaded data for a GET request, remove that data for future requests before returning it for this initial request.
  https://github.com/google/site-kit-wp/blob/9acf11d915c894b7a85e6016216cf4bc9767bd99/assets/js/googlesitekit/api/middleware/preloading.js#L56-L59
• When the middleware is invoked for the first time, set a timeout of 1000 ms
  
  
  
  • When the timeout callback is run, set a flag outside of the middleware which indicates that the middleware has been invalidated
  
  
  • On subsequent invocations, if the above flag is set, the middleware should immediately call next( options ), essentially bypassing its main logic
  
  

Module store adjustments

• In assets/js/googlesitekit/modules/datastore/modules.js:
  
  
  
  • Change the REFETCH_AUTHENTICATION control to be come a registry control, which calls the core/user store's fetchGetAuthentication().
  
  
  • Move yielding the REFETCH_AUTHENTICATION into the setModuleActivation action right after the fetchGetModules call in the clause, as it is right now unnecessarily duplicated.
  
  
• In the PHP Modules class, add google-site-kit/v1/core/modules/data/list to the list of preloaded route paths.

QA Brief

Changelog entry

• Fix usage of apiFetch with a custom middleware to only rely on preloaded data for initial requests on pageload.

[aaemnnosttv]

@felixarntz I think we should also consider that the preloading middleware is unaware of useCache. That is, useCache could be false, but it would still return the response from the preloaded data (if present) if the request was being made for the first time. We should probably ensure the preloaded data is bypassed if a request is made with { useCache: false }. This could be done in siteKitRequest by adding a timestamp query parameter to the URL automatically when useCache is false/disabled. This would bypass the preloading middleware and would also avoid a cached response from the server side as well, without the consumer needing to "know to add the timestamp", it could just be baked into the API client.

Ideally, the preloading would simply prime our cache so the above wouldn't be necessary, but I can't think of a good way to do that due to cache keys, async loading, etc.

Modifying the preloading middleware to be single-use per-endpoint would require that we implement our own version of it, which shouldn't be a problem, but I don't think that addresses all our needs. If we did add the timestamp query param, that would cause the preloaded endpoint to not be invalidated as the URLs wouldn't match exactly. We could check for the URL minus the query param instead.

I'll start an IB based on this; let me know if you agree 😄

[tofumatt]

Oh, I like the approach of using timestamps and integrating with our own caching utilities. This approach, including what's mentioned in https://github.com/google/site-kit-wp/issues/1611#issuecomment-638052631, sounds good to me. 👍

IB ✅

[felixarntz]

@aaemnnosttv I like the approach of using a timestamp when useCache is false, and by deleting the cached data upon returning it, we pretty much have the "one-time usage" we want. IB ✅

[aaemnnosttv]

QA ✅

Tested a preloaded path returned preloaded data on the first request only, resulting in 1 server request after the first response.

Tested that a request made with a timestamp query parameter deleted/invalidated the cache for a preloaded request causing the same request without the timestamp parameter to hit the server as well.

[aaemnnosttv]

@felixarntz this has been reverted - assigning to you to move/close.

[felixarntz]

@aaemnnosttv @tofumatt @ryanwelcher I've amended the ACs to also include the small module-related fixes previously part of #1725 and already implemented there (these changes can mostly be taken from #1732). The original ACs remain pretty much the same, I've just clarified further. I've also added an IB ready for review (the custom preloading middleware setup can reuse code from #1692).

[tofumatt]

We discussed the issues of the timeout vs having the data store emit an event, but I suppose thinking about it—how does the data store know when to invalidate things? I suppose it might be nice to invalidate the cache after X amount of time (say 1000ms after the FIRST bit of data is retrieved from the preloaded cache middleware), just so we don't prematurely invalidate the cached data and lose out on the performance wins.

But aside from that this largely sounds good, and I think if we have a better, less fragile solution later we can change this. So it might be nice if the timeout started after the first data load than on something like window.onload or something, but largely the IB here makes sense.

✅ from me.

[aaemnnosttv]

I suppose it might be nice to invalidate the cache after X amount of time (say 1000ms after the FIRST bit of data is retrieved from the preloaded cache middleware), just so we don't prematurely invalidate the cached data and lose out on the performance wins.

I had a similar thought as well. Let's initialize the timeout the first time the middleware runs 👍 It should only be set once though, so its timeoutID should be stored and the timeout only initialized if there isn't one.

There are a few related fixes that were in the PR for 1725 that may still be necessary (see https://github.com/google/site-kit-wp/pull/1732).

One key difference with this version of the middleware that I think is important to note is that there is no special handling of a timestamp query parameter, and no changes needed to siteKitRequest so it's a bit simpler in that regard.

I'll amend the IB with the detail of when the timeout is started, but otherwise I think this is good to go. (@felixarntz)

IB ✅

Edit
Rather than deleting the cache we can simply set a flag that the middleware is invalid which when set we can bypass the normal logic by calling next( options ) immediately. I think this is slightly cleaner and potentially safer than mutating data unnecessarily.

[aaemnnosttv]

@tofumatt @felixarntz I updated the section about the preloading middleware implementation - assigning back to you for final review.

[felixarntz]

@aaemnnosttv

When the middleware is invoked for the first time, set a timeout of 1000 ms

Just to clarify, you mean invoked as in it's just called, regardless of whether it actually finds preloaded data or not, right? That makes sense, just want to clarify it should not wait until preloaded data is used for the first time, since such a request _may_ just not happen on pageload (although probably in most cases for us it will anyway).

IB ✅

[aaemnnosttv]

Just to clarify, you mean invoked as in it's just called, regardless of whether it actually finds preloaded data or not, right?

Yes, correct 👍

[tofumatt]

Reviewed the updated IB here and it sounds great. Sounds like we're all in agreement so let's move this to execution 🙂

[felixarntz]

Moving this out of the release since it's late and this should get some more testing.

[eugene-manuilov]

QA :heavy_check_mark:. I have tested it and can confirm that apiFetch doesn't rely on preloaded data after initial load and re-fetches it when needed.