eschutho commented on a change in pull request #13218: URL: https://github.com/apache/superset/pull/13218#discussion_r579438976
########## File path: superset-frontend/src/common/hooks/apiResources.ts ########## @@ -0,0 +1,180 @@ +/** + * 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 previous values should stay during refresh. + setResource(initialState); + + // when this effect runs, the endpoint 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, + }); + + 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]); Review comment: I'm trying to follow the logic of this function, which is nested in a few other functions, but basically trying to see if we need this useEffect block. Do you expect this method to be called multiple times with the same endpoint string as an argument? And you're saying that if that happens, you don't want to fetch the resource again, but just return the original one? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
