You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "codyml (via GitHub)" <gi...@apache.org> on 2023/02/02 00:19:37 UTC
[GitHub] [superset] codyml commented on a diff in pull request #22043: feat: add tabs to edit dataset page
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org