elements icon indicating copy to clipboard operation
elements copied to clipboard

Add explicit conditional fetching in dev-portal fetching hooks

Open mpodlasin opened this issue 4 years ago • 3 comments

Currently hooks in dev portal allow for conditional fetching, but only by not passing certain parameters, like nodeSlug etc.

This is bad, because:

a) It's implicit and undocumented. Users will literally have to look at docs or source code to realise that's how it works. b) It's not properly expressed in types, where many parameters are required. You can still pass an empty string to a parameter for example, but it's almost a hack/trick.

For context, here is the source code of one of our hooks (note how enabled parameter is controlled):

export function useGetNodeContent({
  nodeSlug,
  projectId,
  branchSlug,
}: {
  nodeSlug: string;
  projectId: string;
  branchSlug?: string;
}) {
  const { platformUrl, platformAuthToken } = React.useContext(PlatformContext);

  return useQuery(
    ['useNodeContent', nodeSlug, projectId, branchSlug, platformUrl, platformAuthToken],
    () => getNodeContent({ nodeSlug, projectId, branchSlug, platformUrl, platformAuthToken }),
    { enabled: nodeSlug && projectId ? true : false }, // HEEEEEEREEEEEE!!!
  );
}

AC

To all of our hooks related to fetching (using react-query) add a second "options" parameter, which has an optional enabled flag, allowing to explicitly enable/disable fetching.

For example a call to a hook should look like this:

useGetNodeContent(parameters, { enabled: false })

In this case the fetch would not happen.

mpodlasin avatar Jul 07 '21 07:07 mpodlasin

This is missing the use-case. What would we use it for, and is that use-case important enough to justify not implementing it on the consumer side (by using the fetcher function directly) instead?

marcelltoth avatar Jul 16 '21 07:07 marcelltoth

Yeah, this one can be iceboxes or even closed for now.

For some time I felt I needed it in "redirect from old links" story, but I managed without it.

Although there are places in platform where I believe we are fetching with those hooks even though v7 flag is off.

Would make it easier to not fetch in those cases, instead of creating new components just to make a conditional.

mpodlasin avatar Jul 16 '21 07:07 mpodlasin

Also note my issue - we already have that functionality, but introduced in the worst possible way - undocumented, untyped, difficult to find out, unless you literally read the source code.

mpodlasin avatar Jul 16 '21 07:07 mpodlasin