fix loading state on watch list page + better handling for reloads

[alisonelizabeth]

This PR fixes the loading state on the watch list view, removing it from the table and replacing it with the SectionLoading component. I also updated the use_request hook so that the loading state will only be set on the initial request.

FYI @silne30

[elasticmachine]

Pinging @elastic/es-ui

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[cjcenizal]

Tested locally, code LGTM! Just had one suggestion.

[cjcenizal]

I think this hook would be more flexible if it managed and exposed the isInitialRequest state and allowed the consumer to decide how to react to it, instead of encapsulating this type of logic. For example, if this was the hook:

export const useRequest = ({
  path,
  method,
  body,
  interval,
  initialData,
  processData,
}: UseRequest) => {
  const [error, setError] = useState<null | any>(null);
  const [isLoading, setIsLoading] = useState<boolean>(false);
  const [isInitialRequest, setIsInitialRequest] = useState<boolean>(true);
  const [data, setData] = useState<any>(initialData);

  // Tied to every render and bound to each request.
  let isOutdatedRequest = false;

  const createRequest = async (isInitialRequest = true) => {
    // Set a neutral state for a non-request.
    if (!path) {
      setError(null);
      setData(initialData);
      setIsLoading(false);
      return;
    }

    setError(null);
    setIsLoading(true);
    setIsInitialRequest(false);
    setData(initialData);

    const { data: responseData, error: responseError } = await sendRequest({
      path,
      method,
      body,
    });

    // Don't update state if an outdated request has resolved.
    if (isOutdatedRequest) {
      return;
    }

    setError(responseError);
    setData(processData && responseData ? processData(responseData) : responseData);
    setIsLoading(false);
  };

  useEffect(
    () => {
      function cancelOutdatedRequest() {
        isOutdatedRequest = true;
      }

      createRequest();

      if (interval) {
        const intervalRequest = setInterval(createRequest, interval);

        return () => {
          cancelOutdatedRequest();
          clearInterval(intervalRequest);
        };
      }

      // Called when a new render will trigger this effect.
      return cancelOutdatedRequest;
    },
    [path]
  );

  return {
    error,
    isLoading,
    isInitialRequest,
    data,
    createRequest,
  };
};

Then in watch_visualization.tsx we could do something like this (warning, I haven't tested this)"

  const {
    isLoading,
    isInitialRequest,
    data: watchVisualizationData,
    error,
    createRequest: reload,
  } = getWatchVisualizationData(watchWithoutActions, visualizeOptions);

/* ... */

  if (isLoading && isInitialRequest) {
    return (
      <EuiEmptyPrompt
        title={<EuiLoadingChart size="xl" />}
        body={
          <EuiText color="subdued">
            <FormattedMessage
              id="xpack.watcher.sections.watchEdit.loadingWatchVisualizationDescription"
              defaultMessage="Loading watch visualization…"
            />
          </EuiText>
        }
      />
    );
  }

WDYT?

[alisonelizabeth]

@cjcenizal I like this idea. I think the one issue is that we also don't want to set the data back to initialData on subsequent requests.

[cjcenizal]

I agree. I tried to account for that with setIsInitialRequest(false); a few lines down inside of the createRequest function in the useRequest hook -- does that address the problem?

[alisonelizabeth]

I think it would still set the data to initialData every time (but I might be missing something).

setError(null);
setIsLoading(true);
setIsInitialRequest(false);
setData(initialData);

Also, we’re actually setting the initial state of isLoading to false, so I don’t think this check would work:

if (isLoading && isInitialRequest) {}

Not opposed to this approach, just trying to think through how it would work :)

[alisonelizabeth]

@cjcenizal I'm going to go ahead and merge this in as is. I will revisit if we end up needing it to be more flexible!

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[alisonelizabeth]

retest

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request