[superset] branch api-resource-hooks created (now ea37f5e)

[su...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

suddjian pushed a change to branch api-resource-hooks
in repository https://gitbox.apache.org/repos/asf/superset.git.


      at ea37f5e  refactor: introduce api resource hooks, fix error owners

This branch includes the following new commits:

     new ea37f5e  refactor: introduce api resource hooks, fix error owners

The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.

[superset] 01/01: refactor: introduce api resource hooks, fix error owners

[su...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

suddjian pushed a commit to branch api-resource-hooks
in repository https://gitbox.apache.org/repos/asf/superset.git

commit ea37f5ed5aabb0fadcff7fd9b9317746b9951396
Author: David Aaron Suddjian <aa...@gmail.com>
AuthorDate: Thu Feb 18 01:31:37 2021 -0800

    refactor: introduce api resource hooks, fix error owners
---
 superset-frontend/src/chart/Chart.jsx              |  15 +-
 superset-frontend/src/chart/ChartErrorMessage.tsx  |  63 +++++++
 superset-frontend/src/common/hooks/apiResources.ts | 184 +++++++++++++++++++++
 .../dashboard/components/gridComponents/Chart.jsx  |   1 -
 .../src/explore/components/ExploreChartPanel.jsx   |   1 -
 5 files changed, 251 insertions(+), 13 deletions(-)

diff --git a/superset-frontend/src/chart/Chart.jsx b/superset-frontend/src/chart/Chart.jsx
index 3f1596c..8bda7a8 100644
--- a/superset-frontend/src/chart/Chart.jsx
+++ b/superset-frontend/src/chart/Chart.jsx
@@ -25,9 +25,9 @@ import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
 import { Logger, LOG_ACTIONS_RENDER_CHART } from '../logger/LogUtils';
 import Loading from '../components/Loading';
 import RefreshChartOverlay from '../components/RefreshChartOverlay';
-import ErrorMessageWithStackTrace from '../components/ErrorMessage/ErrorMessageWithStackTrace';
 import ErrorBoundary from '../components/ErrorBoundary';
 import ChartRenderer from './ChartRenderer';
+import { ChartErrorMessage } from './ChartErrorMessage';
 
 const propTypes = {
   annotationData: PropTypes.object,
@@ -49,9 +49,6 @@ const propTypes = {
   timeout: PropTypes.number,
   vizType: PropTypes.string.isRequired,
   triggerRender: PropTypes.bool,
-  owners: PropTypes.arrayOf(
-    PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
-  ),
   // state
   chartAlert: PropTypes.string,
   chartStatus: PropTypes.string,
@@ -152,17 +149,13 @@ class Chart extends React.PureComponent {
   }
 
   renderErrorMessage(queryResponse) {
-    const { chartAlert, chartStackTrace, dashboardId, owners } = this.props;
+    const { chartId, chartAlert, chartStackTrace, dashboardId } = this.props;
 
     const error = queryResponse?.errors?.[0];
-    if (error) {
-      const extra = error.extra || {};
-      extra.owners = owners;
-      error.extra = extra;
-    }
     const message = chartAlert || queryResponse?.message;
     return (
-      <ErrorMessageWithStackTrace
+      <ChartErrorMessage
+        chartId={chartId}
         error={error}
         subtitle={message}
         copyText={message}
diff --git a/superset-frontend/src/chart/ChartErrorMessage.tsx b/superset-frontend/src/chart/ChartErrorMessage.tsx
new file mode 100644
index 0000000..123d60f
--- /dev/null
+++ b/superset-frontend/src/chart/ChartErrorMessage.tsx
@@ -0,0 +1,63 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import {
+  useApiV1Resource,
+  useResourceTransform,
+} from 'src/common/hooks/apiResources';
+import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace';
+import { SupersetError } from 'src/components/ErrorMessage/types';
+import Chart from 'src/types/Chart';
+
+interface Props {
+  chartId: string;
+  error?: SupersetError;
+}
+
+function extractOwnerNames({ owners }: Chart) {
+  if (!owners) return null;
+  return owners.map(owner => `${owner.first_name} ${owner.last_name}`);
+}
+
+function useChartOwnerNames(chartId: string) {
+  const { result } = useResourceTransform(
+    useApiV1Resource<Chart>(`/api/v1/chart/${chartId}`),
+    extractOwnerNames,
+  );
+  return result;
+}
+
+/**
+ * fetches the chart owners and adds them to the extra data of the error message
+ */
+export const ChartErrorMessage: React.FC<Props> = ({
+  chartId,
+  error,
+  ...props
+}) => {
+  const owners = useChartOwnerNames(chartId);
+  // don't mutate props
+  const ownedError = error && {
+    ...error,
+    extra: { ...error.extra, owners },
+  };
+
+  return <ErrorMessageWithStackTrace {...props} error={ownedError} />;
+};
diff --git a/superset-frontend/src/common/hooks/apiResources.ts b/superset-frontend/src/common/hooks/apiResources.ts
new file mode 100644
index 0000000..5ae54a7
--- /dev/null
+++ b/superset-frontend/src/common/hooks/apiResources.ts
@@ -0,0 +1,184 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { makeApi } from '@superset-ui/core';
+import { useEffect, useMemo, useRef, useState } from 'react';
+
+export enum ResourceStatus {
+  LOADING = 'loading',
+  COMPLETE = 'complete',
+  ERROR = 'error',
+}
+
+/**
+ * An object containing the data fetched from the API,
+ * as well as loading and error info
+ */
+export type Resource<T> = LoadingState | CompleteState<T> | ErrorState;
+
+// Trying out something a little different: a separate type per status.
+// This should let Typescript know whether a Resource has a result or error.
+// It's possible that I'm expecting too much from Typescript here.
+// If this ends up causing problems, we can change the type to:
+//
+// export type Resource<T> = {
+//   status: ResourceStatus;
+//   result: null | T;
+//   error: null | Error;
+// }
+
+type LoadingState = {
+  status: ResourceStatus.LOADING;
+  result: null;
+  error: null;
+};
+
+type CompleteState<T> = {
+  status: ResourceStatus.COMPLETE;
+  result: T;
+  error: null;
+};
+
+type ErrorState = {
+  status: ResourceStatus.ERROR;
+  result: null;
+  error: Error;
+};
+
+const initialState: LoadingState = {
+  status: ResourceStatus.LOADING,
+  result: null,
+  error: null,
+};
+
+/**
+ * A general-purpose hook to fetch the response from an endpoint.
+ * Returns the full response body from the API, including metadata.
+ *
+ * Note: You likely want {useApiV1Resource} instead of this!
+ *
+ * TODO Store the state in redux or something, share state between hook instances.
+ *
+ * TODO Include a function in the returned resource object to refresh the data.
+ *
+ * A core design decision here is composition > configuration,
+ * and every hook should only have one job.
+ * Please address new needs with new hooks if possible,
+ * rather than adding config options to this hook.
+ *
+ * @param endpoint The url where the resource is located.
+ */
+export function useApiResourceFullBody<RESULT>(
+  endpoint: string,
+): Resource<RESULT> {
+  const [resource, setResource] = useState<Resource<RESULT>>(initialState);
+  const cancelRef = useRef<() => void>(() => {});
+
+  useEffect(() => {
+    // If refresh is implemented, this will need to change.
+    // The state will need to keep
+    setResource(initialState);
+
+    // when this effect runs, the endpoint function has changed.
+    // cancel any current calls so that state doesn't get messed up.
+    cancelRef.current();
+    let cancelled = false;
+    cancelRef.current = () => {
+      cancelled = true;
+    };
+
+    const fetchResource = makeApi<{}, RESULT>({
+      method: 'GET',
+      endpoint,
+    });
+
+    // pass {} to fetchResource, only because makeApi requires it.
+    // we should change makeApi, and then remove that param.
+    // TODO support request cancellation in makeApi
+    fetchResource({})
+      .then(result => {
+        if (!cancelled) {
+          setResource({
+            status: ResourceStatus.COMPLETE,
+            result,
+            error: null,
+          });
+        }
+      })
+      .catch(error => {
+        if (!cancelled) {
+          setResource({
+            status: ResourceStatus.ERROR,
+            result: null,
+            error,
+          });
+        }
+      });
+
+    // Cancel the request when the component un-mounts
+    return () => {
+      cancelled = true;
+    };
+  }, [endpoint]);
+
+  return resource;
+}
+
+/**
+ * For when you want to transform the result of an api resource hook before using it.
+ *
+ * @param resource the Resource object returned from useApiV1Resource
+ * @param transformFn a callback that transforms the result object into the shape you want.
+ * Make sure to use a persistent function for this so it doesn't constantly recalculate!
+ */
+export function useResourceTransform<IN, OUT>(
+  resource: Resource<IN>,
+  transformFn: (result: IN) => OUT,
+): Resource<OUT> {
+  return useMemo(() => {
+    if (resource.status !== ResourceStatus.COMPLETE) {
+      // While incomplete, there is no result - no need to transform.
+      return resource;
+    }
+    return {
+      ...resource,
+      result: transformFn(resource.result),
+    };
+  }, [resource, transformFn]);
+}
+
+// returns the result from fetching an api resource
+const innerResult = <T>(responseBody: { result: T }) => responseBody.result;
+
+/**
+ * A general-purpose hook to fetch a Superset resource from a v1 API endpoint.
+ * Handles request lifecycle and async logic so you don't have to.
+ *
+ * This returns the data under the "result" field in the API response body.
+ * If you need the full response body, use {useFullApiResource} instead.
+ *
+ * @param endpoint The url where the resource is located.
+ */
+export function useApiV1Resource<RESULT>(endpoint: string): Resource<RESULT> {
+  // get the api "result" field within the resource result
+  return useResourceTransform(
+    useApiResourceFullBody<{ result: RESULT }>(endpoint),
+    innerResult,
+  );
+}
diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx
index 7dd01e9..b3c7975 100644
--- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx
+++ b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx
@@ -361,7 +361,6 @@ export default class Chart extends React.Component {
             timeout={timeout}
             triggerQuery={chart.triggerQuery}
             vizType={slice.viz_type}
-            owners={slice.owners}
           />
         </div>
       </div>
diff --git a/superset-frontend/src/explore/components/ExploreChartPanel.jsx b/superset-frontend/src/explore/components/ExploreChartPanel.jsx
index 6962dfa..813b6a9 100644
--- a/superset-frontend/src/explore/components/ExploreChartPanel.jsx
+++ b/superset-frontend/src/explore/components/ExploreChartPanel.jsx
@@ -198,7 +198,6 @@ const ExploreChartPanel = props => {
           errorMessage={props.errorMessage}
           formData={props.form_data}
           onQuery={props.onQuery}
-          owners={props?.slice?.owners}
           queriesResponse={chart.queriesResponse}
           refreshOverlayVisible={props.refreshOverlayVisible}
           setControlValue={props.actions.setControlValue}