fix(sqllab): perf regression on #21532 refactor (#21632)

apache · Oct 3, 2022 · 8d1b7ec · 8d1b7ec
1 parent 1574829
commit 8d1b7ec
Show file tree

Hide file tree

Showing 16 changed files with 408 additions and 257 deletions.
diff --git a/superset-frontend/spec/helpers/testing-library.tsx b/superset-frontend/spec/helpers/testing-library.tsx
@@ -47,7 +47,7 @@ type Options = Omit<RenderOptions, 'queries'> & {
   store?: Store;
 };
 
-function createWrapper(options?: Options) {
+export function createWrapper(options?: Options) {
   const {
     useDnd,
     useRedux,

diff --git a/superset-frontend/src/SqlLab/components/AceEditorWrapper/AceEditorWrapper.test.tsx b/superset-frontend/src/SqlLab/components/AceEditorWrapper/AceEditorWrapper.test.tsx
@@ -48,7 +48,7 @@ jest.mock('src/components/AsyncAceEditor', () => ({
 const setup = (queryEditor: QueryEditor, store?: Store) =>
   render(
     <AceEditorWrapper
-      queryEditor={queryEditor}
+      queryEditorId={queryEditor.id}
       height="100px"
       hotkeys={[]}
       database={{}}

diff --git a/superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx b/superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx
@@ -37,7 +37,7 @@ import {
   AceCompleterKeyword,
   FullSQLEditor as AceEditor,
 } from 'src/components/AsyncAceEditor';
-import { QueryEditor } from 'src/SqlLab/types';
+import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor';
 
 type HotKey = {
   key: string;
@@ -50,7 +50,7 @@ type AceEditorWrapperProps = {
   autocomplete: boolean;
   onBlur: (sql: string) => void;
   onChange: (sql: string) => void;
-  queryEditor: QueryEditor;
+  queryEditorId: string;
   database: any;
   extendedTables?: Array<{ name: string; columns: any[] }>;
   height: string;
@@ -61,15 +61,25 @@ const AceEditorWrapper = ({
   autocomplete,
   onBlur = () => {},
   onChange = () => {},
-  queryEditor,
+  queryEditorId,
   database,
   extendedTables = [],
   height,
   hotkeys,
 }: AceEditorWrapperProps) => {
   const dispatch = useDispatch();
 
-  const { sql: currentSql } = queryEditor;
+  const queryEditor = useQueryEditor(queryEditorId, [
+    'id',
+    'dbId',
+    'sql',
+    'functionNames',
+    'schemaOptions',
+    'tableOptions',
+    'validationResult',
+    'schema',
+  ]);
+  const currentSql = queryEditor.sql ?? '';
   const functionNames = queryEditor.functionNames ?? [];
   const schemas = queryEditor.schemaOptions ?? [];
   const tables = queryEditor.tableOptions ?? [];

diff --git a/superset-frontend/src/SqlLab/components/QueryLimitSelect/QueryLimitSelect.test.tsx b/superset-frontend/src/SqlLab/components/QueryLimitSelect/QueryLimitSelect.test.tsx
@@ -51,7 +51,7 @@ const defaultQueryLimit = 100;
 const setup = (props?: Partial<QueryLimitSelectProps>, store?: Store) =>
   render(
     <QueryLimitSelect
-      queryEditor={defaultQueryEditor}
+      queryEditorId={defaultQueryEditor.id}
       maxRow={100000}
       defaultQueryLimit={defaultQueryLimit}
       {...props}
@@ -67,12 +67,20 @@ describe('QueryLimitSelect', () => {
     const queryLimit = 10;
     const { getByText } = setup(
       {
-        queryEditor: {
-          ...defaultQueryEditor,
-          queryLimit,
-        },
+        queryEditorId: defaultQueryEditor.id,
       },
-      mockStore(initialState),
+      mockStore({
+        ...initialState,
+        sqlLab: {
+          ...initialState.sqlLab,
+          queryEditors: [
+            {
+              ...defaultQueryEditor,
+              queryLimit,
+            },
+          ],
+        },
+      }),
     );
     expect(getByText(queryLimit)).toBeInTheDocument();
   });
@@ -129,7 +137,9 @@ describe('QueryLimitSelect', () => {
         {
           type: 'QUERY_EDITOR_SET_QUERY_LIMIT',
           queryLimit: LIMIT_DROPDOWN[expectedIndex],
-          queryEditor: defaultQueryEditor,
+          queryEditor: {
+            id: defaultQueryEditor.id,
+          },
         },
       ]),
     );

diff --git a/superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx b/superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx
@@ -17,16 +17,16 @@
  * under the License.
  */
 import React from 'react';
-import { useSelector, useDispatch } from 'react-redux';
+import { useDispatch } from 'react-redux';
 import { styled, useTheme } from '@superset-ui/core';
 import { AntdDropdown } from 'src/components';
 import { Menu } from 'src/components/Menu';
 import Icons from 'src/components/Icons';
-import { SqlLabRootState, QueryEditor } from 'src/SqlLab/types';
 import { queryEditorSetQueryLimit } from 'src/SqlLab/actions/sqlLab';
+import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor';
 
 export interface QueryLimitSelectProps {
-  queryEditor: QueryEditor;
+  queryEditorId: string;
   maxRow: number;
   defaultQueryLimit: number;
 }
@@ -79,19 +79,12 @@ function renderQueryLimit(
 }
 
 const QueryLimitSelect = ({
-  queryEditor,
+  queryEditorId,
   maxRow,
   defaultQueryLimit,
 }: QueryLimitSelectProps) => {
-  const queryLimit = useSelector<SqlLabRootState, number>(
-    ({ sqlLab: { unsavedQueryEditor } }) => {
-      const updatedQueryEditor = {
-        ...queryEditor,
-        ...(unsavedQueryEditor.id === queryEditor.id && unsavedQueryEditor),
-      };
-      return updatedQueryEditor.queryLimit || defaultQueryLimit;
-    },
-  );
+  const queryEditor = useQueryEditor(queryEditorId, ['id', 'queryLimit']);
+  const queryLimit = queryEditor.queryLimit || defaultQueryLimit;
   const dispatch = useDispatch();
   const setQueryLimit = (updatedQueryLimit: number) =>
     dispatch(queryEditorSetQueryLimit(queryEditor, updatedQueryLimit));

diff --git a/superset-frontend/src/SqlLab/components/RunQueryActionButton/RunQueryActionButton.test.tsx b/superset-frontend/src/SqlLab/components/RunQueryActionButton/RunQueryActionButton.test.tsx
@@ -41,13 +41,13 @@ jest.mock('src/components/Select/AsyncSelect', () => () => (
 ));
 
 const defaultProps = {
-  queryEditor: defaultQueryEditor,
+  queryEditorId: defaultQueryEditor.id,
   allowAsync: false,
   dbId: 1,
   queryState: 'ready',
-  runQuery: jest.fn(),
+  runQuery: () => {},
   selectedText: null,
-  stopQuery: jest.fn(),
+  stopQuery: () => {},
   overlayCreateAsMenu: null,
 };
 
@@ -57,95 +57,104 @@ const setup = (props?: Partial<Props>, store?: Store) =>
     ...(store && { store }),
   });
 
-describe('RunQueryActionButton', () => {
-  beforeEach(() => {
-    defaultProps.runQuery.mockReset();
-    defaultProps.stopQuery.mockReset();
-  });
+it('renders a single Button', () => {
+  const { getByRole } = setup({}, mockStore(initialState));
+  expect(getByRole('button')).toBeInTheDocument();
+});
 
-  it('renders a single Button', () => {
-    const { getByRole } = setup({}, mockStore(initialState));
-    expect(getByRole('button')).toBeInTheDocument();
-  });
+it('renders a label for Run Query', () => {
+  const { getByText } = setup({}, mockStore(initialState));
+  expect(getByText('Run')).toBeInTheDocument();
+});
 
-  it('renders a label for Run Query', () => {
-    const { getByText } = setup({}, mockStore(initialState));
-    expect(getByText('Run')).toBeInTheDocument();
-  });
+it('renders a label for Selected Query', () => {
+  const { getByText } = setup(
+    {},
+    mockStore({
+      ...initialState,
+      sqlLab: {
+        ...initialState.sqlLab,
+        unsavedQueryEditor: {
+          id: defaultQueryEditor.id,
+          selectedText: 'select * from\n-- this is comment\nwhere',
+        },
+      },
+    }),
+  );
+  expect(getByText('Run selection')).toBeInTheDocument();
+});
 
-  it('renders a label for Selected Query', () => {
-    const { getByText } = setup(
-      {},
-      mockStore({
-        ...initialState,
-        sqlLab: {
-          ...initialState.sqlLab,
-          unsavedQueryEditor: {
-            id: defaultQueryEditor.id,
-            selectedText: 'FROM',
-          },
+it('disable button when sql from unsaved changes is empty', () => {
+  const { getByRole } = setup(
+    {},
+    mockStore({
+      ...initialState,
+      sqlLab: {
+        ...initialState.sqlLab,
+        unsavedQueryEditor: {
+          id: defaultQueryEditor.id,
+          sql: '',
         },
-      }),
-    );
-    expect(getByText('Run selection')).toBeInTheDocument();
-  });
+      },
+    }),
+  );
+  const button = getByRole('button');
+  expect(button).toBeDisabled();
+});
 
-  it('disable button when sql from unsaved changes is empty', () => {
-    const { getByRole } = setup(
-      {},
-      mockStore({
-        ...initialState,
-        sqlLab: {
-          ...initialState.sqlLab,
-          unsavedQueryEditor: {
-            id: defaultQueryEditor.id,
-            sql: '',
-          },
+it('disable button when selectedText only contains blank contents', () => {
+  const { getByRole } = setup(
+    {},
+    mockStore({
+      ...initialState,
+      sqlLab: {
+        ...initialState.sqlLab,
+        unsavedQueryEditor: {
+          id: defaultQueryEditor.id,
+          selectedText: '-- this is comment\n\n     \t',
         },
-      }),
-    );
-    const button = getByRole('button');
-    expect(button).toBeDisabled();
-  });
+      },
+    }),
+  );
+  const button = getByRole('button');
+  expect(button).toBeDisabled();
+});
 
-  it('enable default button for unrelated unsaved changes', () => {
-    const { getByRole } = setup(
-      {},
-      mockStore({
-        ...initialState,
-        sqlLab: {
-          ...initialState.sqlLab,
-          unsavedQueryEditor: {
-            id: `${defaultQueryEditor.id}-other`,
-            sql: '',
-          },
+it('enable default button for unrelated unsaved changes', () => {
+  const { getByRole } = setup(
+    {},
+    mockStore({
+      ...initialState,
+      sqlLab: {
+        ...initialState.sqlLab,
+        unsavedQueryEditor: {
+          id: `${defaultQueryEditor.id}-other`,
+          sql: '',
         },
-      }),
-    );
-    const button = getByRole('button');
-    expect(button).toBeEnabled();
-  });
+      },
+    }),
+  );
+  const button = getByRole('button');
+  expect(button).toBeEnabled();
+});
 
-  it('dispatch runQuery on click', async () => {
-    const { getByRole } = setup({}, mockStore(initialState));
-    const button = getByRole('button');
-    expect(defaultProps.runQuery).toHaveBeenCalledTimes(0);
-    fireEvent.click(button);
-    await waitFor(() => expect(defaultProps.runQuery).toHaveBeenCalledTimes(1));
-  });
+it('dispatch runQuery on click', async () => {
+  const runQuery = jest.fn();
+  const { getByRole } = setup({ runQuery }, mockStore(initialState));
+  const button = getByRole('button');
+  expect(runQuery).toHaveBeenCalledTimes(0);
+  fireEvent.click(button);
+  await waitFor(() => expect(runQuery).toHaveBeenCalledTimes(1));
+});
 
-  describe('on running state', () => {
-    it('dispatch stopQuery on click', async () => {
-      const { getByRole } = setup(
-        { queryState: 'running' },
-        mockStore(initialState),
-      );
-      const button = getByRole('button');
-      expect(defaultProps.stopQuery).toHaveBeenCalledTimes(0);
-      fireEvent.click(button);
-      await waitFor(() =>
-        expect(defaultProps.stopQuery).toHaveBeenCalledTimes(1),
-      );
-    });
-  });
+it('dispatch stopQuery on click while running state', async () => {
+  const stopQuery = jest.fn();
+  const { getByRole } = setup(
+    { queryState: 'running', stopQuery },
+    mockStore(initialState),
+  );
+  const button = getByRole('button');
+  expect(stopQuery).toHaveBeenCalledTimes(0);
+  fireEvent.click(button);
+  await waitFor(() => expect(stopQuery).toHaveBeenCalledTimes(1));
 });