codyml commented on code in PR #22043:
URL: https://github.com/apache/superset/pull/22043#discussion_r1093843914
##########
superset-frontend/src/views/CRUD/data/hooks.ts:
##########
@@ -81,35 +81,65 @@ export function useQueryPreviewState<D extends
BaseQueryObject = any>({
/**
* Retrieves all pages of dataset results
*/
-export const UseGetDatasetsList = async (filters: object[]) => {
- let results: DatasetObject[] = [];
- let page = 0;
- let count;
-
- // If count is undefined or less than results, we need to
- // asynchronously retrieve a page of dataset results
- while (count === undefined || results.length < count) {
- const queryParams = rison.encode_uri({ filters, page });
- try {
- // eslint-disable-next-line no-await-in-loop
- const response = await SupersetClient.get({
- endpoint: `/api/v1/dataset/?q=${queryParams}`,
- });
+export const useGetDatasetsList = () => {
+ const [datasets, setDatasets] = useState<DatasetObject[]>([]);
+
+ const getDatasetsList = useCallback(async (filters: object[]) => {
+ let results: DatasetObject[] = [];
+ let page = 0;
+ let count;
- // Reassign local count to response's count
- ({ count } = response.json);
+ // If count is undefined or less than results, we need to
+ // asynchronously retrieve a page of dataset results
+ while (count === undefined || results.length < count) {
+ const queryParams = rison.encode_uri({ filters, page });
+ try {
+ // eslint-disable-next-line no-await-in-loop
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ });
- const {
- json: { result },
- } = response;
+ // Reassign local count to response's count
+ ({ count } = response.json);
- results = [...results, ...result];
+ const {
+ json: { result },
+ } = response;
- page += 1;
- } catch (error) {
- addDangerToast(t('There was an error fetching dataset'));
- logging.error(t('There was an error fetching dataset'), error);
+ results = [...results, ...result];
+
+ page += 1;
+ } catch (error) {
+ addDangerToast(t('There was an error fetching dataset'));
+ logging.error(t('There was an error fetching dataset'), error);
+ }
}
- }
- return results;
+
+ setDatasets(results);
+ return results;
+ }, []);
+
+ const datasetNames = datasets?.map(dataset => dataset.table_name);
+
+ return { getDatasetsList, datasets, datasetNames };
+};
+
+export const useGetDatasetRelatedObjects = (id: string) => {
+ const [usageCount, setUsageCount] = useState(0);
+
+ const getDatasetRelatedObjects = () =>
Review Comment:
This function is redefined on each render, triggering extra effect runs and
thereby extra API calls in `EditDataset/index.tsx`. If you wrap it in a
`useCallback`, it'll only call the API when `id` changes, which I think is what
we want.
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -82,35 +76,31 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
- const [datasets, setDatasets] = useState<DatasetObject[]>([]);
- const datasetNames = datasets.map(dataset => dataset.table_name);
+ const [editPageIsVisible, setEditPageIsVisible] = useState(false);
const encodedSchema = dataset?.schema
? encodeURIComponent(dataset?.schema)
: undefined;
- const getDatasetsList = useCallback(async () => {
+ const { getDatasetsList, datasets, datasetNames } = useGetDatasetsList();
Review Comment:
Non-blocking, but I think this hook could also be rewritten to encapsulate
the get-dataset-list process a little better so there's less back-and-forth
between the hook and the main function body. I think you could rename it
something like `useDatasetsList`, move the schema encoding and the effect
inside of the hook, and have it return just `datasets` and `datasetNames`, so
lines 80-96 would be replaced with a one-liner like this:
```javascript
const { datasets, datasetNames } = useDatasetsList(dataset?.db,
dataset?.schema);
```
##########
superset-frontend/src/views/CRUD/data/hooks.ts:
##########
@@ -81,35 +81,65 @@ export function useQueryPreviewState<D extends
BaseQueryObject = any>({
/**
* Retrieves all pages of dataset results
*/
-export const UseGetDatasetsList = async (filters: object[]) => {
- let results: DatasetObject[] = [];
- let page = 0;
- let count;
-
- // If count is undefined or less than results, we need to
- // asynchronously retrieve a page of dataset results
- while (count === undefined || results.length < count) {
- const queryParams = rison.encode_uri({ filters, page });
- try {
- // eslint-disable-next-line no-await-in-loop
- const response = await SupersetClient.get({
- endpoint: `/api/v1/dataset/?q=${queryParams}`,
- });
+export const useGetDatasetsList = () => {
+ const [datasets, setDatasets] = useState<DatasetObject[]>([]);
+
+ const getDatasetsList = useCallback(async (filters: object[]) => {
+ let results: DatasetObject[] = [];
+ let page = 0;
+ let count;
- // Reassign local count to response's count
- ({ count } = response.json);
+ // If count is undefined or less than results, we need to
+ // asynchronously retrieve a page of dataset results
+ while (count === undefined || results.length < count) {
+ const queryParams = rison.encode_uri({ filters, page });
+ try {
+ // eslint-disable-next-line no-await-in-loop
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ });
- const {
- json: { result },
- } = response;
+ // Reassign local count to response's count
+ ({ count } = response.json);
- results = [...results, ...result];
+ const {
+ json: { result },
+ } = response;
- page += 1;
- } catch (error) {
- addDangerToast(t('There was an error fetching dataset'));
- logging.error(t('There was an error fetching dataset'), error);
+ results = [...results, ...result];
+
+ page += 1;
+ } catch (error) {
+ addDangerToast(t('There was an error fetching dataset'));
+ logging.error(t('There was an error fetching dataset'), error);
+ }
}
- }
- return results;
+
+ setDatasets(results);
+ return results;
+ }, []);
+
+ const datasetNames = datasets?.map(dataset => dataset.table_name);
+
+ return { getDatasetsList, datasets, datasetNames };
+};
+
+export const useGetDatasetRelatedObjects = (id: string) => {
+ const [usageCount, setUsageCount] = useState(0);
+
+ const getDatasetRelatedObjects = () =>
Review Comment:
Also, non-blocking but if you're working on it anyway, I think it might
provide better encapsulation if you renamed the hook to something like
`useDatasetRelatedCounts`, brought the effect inside of the hook, and returned
only the counts (just `usageCount` for now, but with the possibility to expand
to more counts later).
##########
superset-frontend/src/views/CRUD/data/dataset/DatasetLayout/index.tsx:
##########
@@ -45,17 +46,19 @@ export default function DatasetLayout({
datasetPanel,
rightPanel,
footer,
+ editPageIsVisible,
}: DatasetLayoutProps) {
return (
<StyledLayoutWrapper data-test="dataset-layout-wrapper">
{header && <StyledLayoutHeader>{header}</StyledLayoutHeader>}
<OuterRow>
- <LeftColumn>
- {leftPanel && (
- <StyledLayoutLeftPanel>{leftPanel}</StyledLayoutLeftPanel>
- )}
- </LeftColumn>
-
+ {!editPageIsVisible && (
+ <LeftColumn>
+ {leftPanel && (
+ <StyledLayoutLeftPanel>{leftPanel}</StyledLayoutLeftPanel>
+ )}
+ </LeftColumn>
+ )}
Review Comment:
Since `leftPanel` will be `null` if `editPageIsVisible` is true, could we
avoid passing `editPageIsVisible` as a prop and instead simplify to this?
```suggestion
{leftPanel && (
<LeftColumn>
<StyledLayoutLeftPanel>{leftPanel}</StyledLayoutLeftPanel>
</LeftColumn>
)}
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
