department-of-veterans-affairs / va.gov-cms

Editor-centered management for Veteran-centered content.
https://prod.cms.va.gov
GNU General Public License v2.0
99 stars 70 forks source link

Static Data Files: Add support for JSON:API #9178

Closed ryguyk closed 2 weeks ago

ryguyk commented 2 years ago

See #8199. We should add support for queries to be written in JSON:API (in addition to GraphQL).

This was largely done - at least workably - in initial work on #8199, but then scrapped due to growing scope. Part of that growing scope is the page size limitation (50 results). In itself, this should not be too difficult to handle. This can be accomplished doing something like below:

const query = async (proxyFetch, headers, baseUrl, query, handleQueryResponse, data = [], included = []) => {
  const queryUrl = query.includes(baseUrl) ? query : `${baseUrl}${query}`;
  const response = await proxyFetch(queryUrl, {
    headers,
    method: 'get',
    mode: 'cors',
  });

  const handledResponse = await handleQueryResponse(response);
  const { data: responseData, included: responseIncluded, links } = handledResponse;
  if (responseData) {
    data = [...data, ...responseData];
  }
  if (responseIncluded) {
    included = [...included, ...responseIncluded];
  }

  // Results limited to 50 per page. Recursively append results from `links.next` until all are fetched.
  if (links?.next?.href) {
    return query(proxyFetch, headers, baseUrl, links.next.href, handleQueryResponse, data, included);
  } else {
    return {
      data,
      included
    }
  }
};

module.exports = {
  query,
}

However, this has some drawbacks. First, the paged results are fetched in series rather than in parallel. It'd be better to grab the total results on the first fetch, then fetch the remaining pages in parallel, constructing queries based on the total-results count.

Second, this will seemingly result in duplicated entries in the included array. It might be useful to remove these duplicates, but it might also not be necessary. If the logic to reference these included items is to simply find the first matching item, it might be wasted processing time to worry about removing duplicates.

It will likely be worthwhile to explore existing node.js libraries for dealing with JSON:API. If none are suitable, a small custom library should likely be developed.

swirtSJW commented 2 years ago

Yah pagination was out of scope of the original. Was supposed to be a copy and move image But that original scope assumed that specifying the number of results in the query actually worked and was not hard coded to 50. Sorry you had to make that discovery Ryan.

wesrowe commented 1 year ago

@swirtSJW @jilladams @ryguyk – a few questions related to sprint planning:

  1. Is this a priority we should keep in focus?
  2. If so, is it likely to get onto Ryan's sprint list?
  3. How many points?
ryguyk commented 1 year ago
  1. I don't see a pressing need for this. I'd say we would want to have a very clear need in the form of new KISS endpoints and/or a very clear vision of the future use of JSON:API. At least part of the idea here is to support JSON:API so that KISS queries could more easily be ported in the future. Currently, we don't have that future (especially with Accelerated Publishing on hold). Additionally, usage of KISS has actually decreased from two files to one (both files are actually still generated in content-build, but, as of recently, one is no longer needed).
  2. If there is a need for this, I can likely accommodate. Front-end work on the Facilities team is looking a little thin for the remainder of Q1. I'll likely fill in those gaps with some Drupal work, but it's worth noting.
  3. This is not a small lift. 8-13 as I think about it, tending towards 13.
Agile6MSkinner commented 2 weeks ago

Per latest comments and considering age of the ticket, I'm closing.