Codecov Report

Merging #13218 (1a0f976) into master (2ce7982) will increase coverage by 19.89%.
The diff coverage is 69.96%.

@@             Coverage Diff             @@
##           master   #13218       +/-   ##
===========================================
+ Coverage   53.06%   72.95%   +19.89%     
===========================================
  Files         489      576       +87     
  Lines       17314    20772     +3458     
  Branches     4482     5375      +893     
===========================================
+ Hits         9187    15155     +5968     
+ Misses       8127     5494     -2633     
- Partials        0      123      +123     

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c787f46...0f3824e. Read the comment docs.

Hmm, interesting concept. We definitely lack some handy React hooks to fetch API resources. But I'd also like to point out the makeApi util already supports a processResponse option that allows you to transform the response.

It even infers the return type for you:

So we may be able to simplify the hooks a little bit. For example, useChartOwnerNames can be rewritten as:

function useAsyncData<T>(promise: Promise<T>) {
  const [result, setResult] = useState<{ data?: T; error": any }>();
  useEffect(() => {
    promise.then(data => setResult({ data }))
      .catch(error => setResult({ error }));
  }, [promise]);
  return { data, error };
}

function useChartOwnerNames(chartId: number) {
  const fetchChartOwnerNames = makeApi({
    method: 'GET',
    requestType: 'rison',
    endpoint: `/api/v1/chart/${chartId}`,
    processResponse: ({ result } : { result?: Chart }) =>
      results?.owners.map(owner =>
        `${owner.first_name || ''} ${owner.last_name || ''}`.strip() || owner.id) || [],
  );
  return useAsyncData(fetchChartOwnerNames({
    columns: ['owners.first_name', 'owners.last_name', 'owners.id']
  }));
}

If we add support for url match params for endpoint and default base payload (i.e. make column selection part of the base payload), we may even move fetchChartOwnerNames outside of the hook and have it become a general async API caller (and potentially export for other places to use).

@ktmud At first I did consider an approach like your useAsyncData example, but I found that there was more that could be done when using a url as a resource identifier instead of passing in an anonymous promise. This may not be obvious right now, since the hook doesn't yet have any extra bells and whistles attached, but this interface will allow us to add on some cool functionality later. Things like de-duplicating requests between components, utilizing local storage, caching results, streaming updates from the server or changes from elsewhere in the app, all without callers of the useApiResource hooks needing to change anything about their usage.

That's also why I opted not to use the processResponse param. I wanted to keep the core api fetching hook as single-minded as possible to maximize future de-duplication potential, and treat the transformation as a sort of composed view of that data.

I do wonder if those cool future add-ons might be better implemented inside the guts of makeApi, though 🤔

@suddjian I'm not against having hooks that do extra cool stuff. I think a powerful API resource manager would be very useful.

I do, however, believe there's value in hiding API endpoints and pre-defined transformations from end users and have a centralized, strongly typed API catalog like this.

makeApi naturally supports this, while the composable hooks approach may lead to more fragmentary code. There are also places where a general async function would be preferred, because hooks can only be used in functional components.

Re: dedupping requests and caching results, I do have some simple solution that I used in our internal Superset:

const promiseCache: Record<
  string,
  { timestamp: number; promise: Promise<any> }
> = {};

/**
 * Cache a promise maker, make sure it doesn't fire multiple times
 * @param api - the promise generator.
 * @param expires - cache TTL in seconds.
 * @param keyPrefix - prefix for the cache key.
 */
export function cached<T extends (...args: any[]) => Promise<any>>(
  api: T,
  {
    expires = 300,
    keyPrefix = '',
  }: {
    expires?: number;
    keyPrefix?: string;
  },
) {
  return function wrapped(...args: Parameters<T>): ReturnType<T> {
    const cacheKey = `${keyPrefix}-${JSON.stringify(args)}`;
    if (
      cacheKey in promiseCache &&
      promiseCache[cacheKey].timestamp + expires * 1000 > Date.now()
    ) {
      return promiseCache[cacheKey].promise as ReturnType<T>;
    }
    const promise = api(...args);
    promiseCache[cacheKey] = {
      promise,
      timestamp: Date.now(),
    };
    promise.catch(() => {
      // don't cache failed responses
      delete promiseCache[cacheKey];
    });
    return promise as ReturnType<T>;
  };
}

export const getValidMetrics = cached(
  makeApi<MinervaValidationApiPaylod, ControlValues['metrics']>({
    method: 'GET',
    endpoint: '/minerva/valid_metrics',
  }),
  { keyPrefix: 'metrics' },
);

It's a very simple wrapper on the generated API caller and handles both caching and deduplication (making the same API call will not fire a request twice).

For now, this is probably good enough for Superset, but if we are thinking of getting fancy with deduplication and caching by entity, I would also like to see us evaluating some existing solutions out there such as REST Hooks before committing to create our own solution for things like shared storage.

I do, however, believe there's value in hiding API endpoints and pre-defined transformations from end users and have a centralized, strongly typed API catalog like this.

makeApi naturally supports this, while the composable hooks approach may lead to more fragmentary code. There are also places where a general async function would be preferred, because hooks can only be used in functional components.

I like the catalog idea! That is very much compatible with this work. I'll add something like that for these hooks as well.

Almost all new frontend code is being written with function components, with some efforts underway to convert class components to functions, and it is also quite easy to write a HOC to patch in hook functionality to class components if needed, so relying on hooks for this sort of logic doesn't bother me at all. I'm actually pretty excited about the possibilities there.

For now, this is probably good enough for Superset, but if we are thinking of getting fancy with deduplication and caching by entity, I would also like to see us evaluating some existing solutions out there such as REST Hooks before committing to create our own solution for things like shared storage.

This is my first time reading about the REST hooks library, looks like a great option! I agree that we should wait to review it until we want to get fancy. Looks like it adds a few new abstractions, which may or may not be right for us when we reach that point. The hooks I've written here have a very simple interface so they should be easy to replace with a library later if we want to.

@suddjian are we still thinking of moving api responses into redux and how would that work with this?

Another library for consideration: https://react-query.tanstack.com/