You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "justinpark (via GitHub)" <gi...@apache.org> on 2023/08/21 21:50:33 UTC
[GitHub] [superset] justinpark opened a new pull request, #25047: chore(sqllab): typescript for getInitialState
justinpark opened a new pull request, #25047:
URL: https://github.com/apache/superset/pull/25047
### SUMMARY
Part 2 of SQLLab SPA migration: [detail link](https://docs.google.com/document/d/1opyNuaw-CEAD9tG6fx_zqNcZwe3_soQgsnSJTP4mQkk/edit?usp=sharing)
This commit migrates the existing getInialState helper to typescript to be compatible with SPA entrypoint with `/api/v1/sqllab/`. (This commit also adjusts the existing SqlLab types and dependent types)
This commit also introduces `useSqlLabInitialStateQuery` which fetches `/api/v1/sqllab/` and will be used with the updated getInitialState helper in the next page component integration.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
### TESTING INSTRUCTIONS
npm run type
npm run test
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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
[GitHub] [superset] justinpark commented on a diff in pull request #25047: chore(sqllab): typescript for getInitialState
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #25047:
URL: https://github.com/apache/superset/pull/25047#discussion_r1303341992
##########
superset-frontend/src/SqlLab/reducers/getInitialState.ts:
##########
@@ -18,12 +18,20 @@
*/
import { t } from '@superset-ui/core';
import getToastsFromPyFlashMessages from 'src/components/MessageToasts/getToastsFromPyFlashMessages';
+import type { BootstrapData } from 'src/types/bootstrapTypes';
+import type { InitialState } from 'src/hooks/apiResources/sqlLab';
+import type {
+ QueryEditor,
+ UnsavedQueryEditor,
+ SqlLabRootState,
+ Table,
+} from 'src/SqlLab/types';
-export function dedupeTabHistory(tabHistory) {
+export function dedupeTabHistory(tabHistory: string[]) {
return tabHistory.reduce(
(result, tabId) =>
result.slice(-1)[0] === tabId ? result : result.concat(tabId),
- [],
+ tabHistory.slice(0, 0),
Review Comment:
Designed for Implicit typing. `[] <- unknown[], tabHistory.slice(0,0) <- string[]`
--
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
[GitHub] [superset] justinpark commented on a diff in pull request #25047: chore(sqllab): typescript for getInitialState
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #25047:
URL: https://github.com/apache/superset/pull/25047#discussion_r1304607563
##########
superset-frontend/src/hooks/apiResources/sqlLab.ts:
##########
@@ -0,0 +1,75 @@
+/**
+ * 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 { QueryResponse } from '@superset-ui/core';
+import { api, JsonResponse } from './queryApi';
+
+export type InitialState = {
+ active_tab: {
+ id: number;
+ user_id: number;
+ label: string;
+ active: boolean;
+ database_id: number;
+ schema?: string;
+ table_schemas: {
+ id: number;
+ table: string;
+ description: {
+ columns?: {
+ name: string;
+ type: string;
+ }[];
+ dataPreviewQueryId?: string;
+ } & Record<string, any>;
+ schema?: string;
+ tab_state_id: number;
+ database_id?: number;
+ expanded?: boolean;
+ }[];
+ sql: string;
+ query_limit?: number;
+ latest_query?: QueryResponse | null;
+ autorun?: boolean;
+ template_params?: string | null;
+ hide_left_bar?: boolean;
+ saved_query?: { id: number } | null;
+ extra_json?: object;
+ };
+ databases: object[];
+ queries: Record<string, QueryResponse & { inLocalStorage?: boolean }>;
+ tab_state_ids: {
+ id: number;
+ label: string;
+ }[];
+};
+
+const queryValidationApi = api.injectEndpoints({
+ endpoints: builder => ({
+ sqlLabInitialState: builder.query<InitialState, void>({
+ providesTags: ['SqlLabInitialState'],
+ query: () => ({
+ endpoint: `/api/v1/sqllab/`,
+ headers: { 'Content-Type': 'application/json' },
+ transformResponse: ({ json }: JsonResponse) => json.result,
+ }),
+ }),
+ }),
+});
+
+export const { useSqlLabInitialStateQuery } = queryValidationApi;
Review Comment:
updated
--
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
[GitHub] [superset] justinpark commented on a diff in pull request #25047: chore(sqllab): typescript for getInitialState
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #25047:
URL: https://github.com/apache/superset/pull/25047#discussion_r1300688779
##########
superset-frontend/src/SqlLab/reducers/getInitialState.ts:
##########
@@ -140,11 +136,12 @@ export default function getInitialState({
* hasn't used SQL Lab after it has been turned on, the state will be stored
* in the browser's local storage.
*/
- if (
- localStorage.getItem('redux') &&
- JSON.parse(localStorage.getItem('redux')).sqlLab
- ) {
- const { sqlLab } = JSON.parse(localStorage.getItem('redux'));
+ const localStorageData = localStorage.getItem('redux');
+ const sqlLabCacheData = localStorageData
Review Comment:
Stores the parsed object here to save the additional JSON.parse operation at line 147
--
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
[GitHub] [superset] justinpark commented on a diff in pull request #25047: chore(sqllab): typescript for getInitialState
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #25047:
URL: https://github.com/apache/superset/pull/25047#discussion_r1303399352
##########
superset-frontend/src/hooks/apiResources/sqlLab.ts:
##########
@@ -0,0 +1,75 @@
+/**
+ * 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 { QueryResponse } from '@superset-ui/core';
+import { api, JsonResponse } from './queryApi';
+
+export type InitialState = {
+ active_tab: {
+ id: number;
+ user_id: number;
+ label: string;
+ active: boolean;
+ database_id: number;
+ schema?: string;
+ table_schemas: {
+ id: number;
+ table: string;
+ description: {
+ columns?: {
+ name: string;
+ type: string;
+ }[];
+ dataPreviewQueryId?: string;
+ } & Record<string, any>;
+ schema?: string;
+ tab_state_id: number;
+ database_id?: number;
+ expanded?: boolean;
+ }[];
+ sql: string;
+ query_limit?: number;
+ latest_query?: QueryResponse | null;
+ autorun?: boolean;
+ template_params?: string | null;
+ hide_left_bar?: boolean;
+ saved_query?: { id: number } | null;
+ extra_json?: object;
+ };
+ databases: object[];
+ queries: Record<string, QueryResponse & { inLocalStorage?: boolean }>;
+ tab_state_ids: {
+ id: number;
+ label: string;
+ }[];
+};
+
+const queryValidationApi = api.injectEndpoints({
+ endpoints: builder => ({
+ sqlLabInitialState: builder.query<InitialState, void>({
+ providesTags: ['SqlLabInitialState'],
+ query: () => ({
+ endpoint: `/api/v1/sqllab/`,
+ headers: { 'Content-Type': 'application/json' },
+ transformResponse: ({ json }: JsonResponse) => json.result,
+ }),
+ }),
+ }),
+});
+
+export const { useSqlLabInitialStateQuery } = queryValidationApi;
Review Comment:
I'm okay to rename as you suggested. btw xxxQuery is a naming convention of rdk query hooks.
--
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
[GitHub] [superset] justinpark commented on a diff in pull request #25047: chore(sqllab): typescript for getInitialState
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #25047:
URL: https://github.com/apache/superset/pull/25047#discussion_r1303343998
##########
superset-frontend/src/SqlLab/types.ts:
##########
@@ -33,16 +33,19 @@ export interface QueryEditor {
id: string;
dbId?: number;
name: string;
- schema: string;
+ title?: string; // keep in optional for backward compatibility
+ schema?: string;
autorun: boolean;
sql: string;
- remoteId: number | null;
+ remoteId?: number | null;
Review Comment:
I think backend passed in `null` value instead of omitting the key.
--
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
[GitHub] [superset] justinpark merged pull request #25047: chore(sqllab): typescript for getInitialState
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark merged PR #25047:
URL: https://github.com/apache/superset/pull/25047
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #25047: chore(sqllab): typescript for getInitialState
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #25047:
URL: https://github.com/apache/superset/pull/25047#discussion_r1304819583
##########
superset-frontend/src/SqlLab/reducers/getInitialState.ts:
##########
@@ -199,11 +197,12 @@ export default function getInitialState({
tabHistory: dedupeTabHistory(tabHistory),
tables: Object.values(tables),
queriesLastUpdate: Date.now(),
- unsavedQueryEditor,
+ editorTabLastUpdatedAt,
queryCostEstimates: {},
+ unsavedQueryEditor,
},
messageToasts: getToastsFromPyFlashMessages(
- (common || {}).flash_messages || [],
+ (common || {})?.flash_messages || [],
Review Comment:
```suggestion
(common || {}).flash_messages || [],
```
--
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
[GitHub] [superset] justinpark commented on a diff in pull request #25047: chore(sqllab): typescript for getInitialState
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #25047:
URL: https://github.com/apache/superset/pull/25047#discussion_r1303789007
##########
superset-frontend/src/SqlLab/reducers/getInitialState.ts:
##########
@@ -18,12 +18,20 @@
*/
import { t } from '@superset-ui/core';
import getToastsFromPyFlashMessages from 'src/components/MessageToasts/getToastsFromPyFlashMessages';
+import type { BootstrapData } from 'src/types/bootstrapTypes';
+import type { InitialState } from 'src/hooks/apiResources/sqlLab';
+import type {
+ QueryEditor,
+ UnsavedQueryEditor,
+ SqlLabRootState,
+ Table,
+} from 'src/SqlLab/types';
-export function dedupeTabHistory(tabHistory) {
+export function dedupeTabHistory(tabHistory: string[]) {
return tabHistory.reduce(
(result, tabId) =>
result.slice(-1)[0] === tabId ? result : result.concat(tabId),
- [],
+ tabHistory.slice(0, 0),
Review Comment:
replaced by generic type
--
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
[GitHub] [superset] justinpark commented on a diff in pull request #25047: chore(sqllab): typescript for getInitialState
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #25047:
URL: https://github.com/apache/superset/pull/25047#discussion_r1300683041
##########
superset-frontend/src/SqlLab/components/TableElement/index.tsx:
##########
@@ -49,16 +50,6 @@ export interface Column {
type: string;
}
-export interface Table {
Review Comment:
migrate to SqlLab/types
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #25047: chore(sqllab): typescript for getInitialState
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #25047:
URL: https://github.com/apache/superset/pull/25047#discussion_r1302909802
##########
superset-frontend/src/SqlLab/reducers/getInitialState.ts:
##########
@@ -18,12 +18,20 @@
*/
import { t } from '@superset-ui/core';
import getToastsFromPyFlashMessages from 'src/components/MessageToasts/getToastsFromPyFlashMessages';
+import type { BootstrapData } from 'src/types/bootstrapTypes';
+import type { InitialState } from 'src/hooks/apiResources/sqlLab';
+import type {
+ QueryEditor,
+ UnsavedQueryEditor,
+ SqlLabRootState,
+ Table,
+} from 'src/SqlLab/types';
-export function dedupeTabHistory(tabHistory) {
+export function dedupeTabHistory(tabHistory: string[]) {
return tabHistory.reduce(
(result, tabId) =>
result.slice(-1)[0] === tabId ? result : result.concat(tabId),
- [],
+ tabHistory.slice(0, 0),
Review Comment:
Why did you use `slice(0,0)`?
##########
superset-frontend/src/SqlLab/types.ts:
##########
@@ -33,16 +33,19 @@ export interface QueryEditor {
id: string;
dbId?: number;
name: string;
- schema: string;
+ title?: string; // keep in optional for backward compatibility
+ schema?: string;
autorun: boolean;
sql: string;
- remoteId: number | null;
+ remoteId?: number | null;
Review Comment:
Do you explicitly need `null` or can we use `undefined`?
```suggestion
remoteId?: number;
```
##########
superset-frontend/src/SqlLab/types.ts:
##########
@@ -53,6 +56,21 @@ export type toastState = {
noDuplicate: boolean;
};
+export type UnsavedQueryEditor = Partial<QueryEditor> & {
+ id?: QueryEditor['id'];
+};
+
+export interface Table {
+ id: string;
+ dbId: number;
+ schema: string;
+ name: string;
+ queryEditorId: QueryEditor['id'];
+ dataPreviewQueryId?: string | null;
Review Comment:
Do you explicitly need `null` or can we use `undefined`?
```suggestion
dataPreviewQueryId?: string;
```
##########
superset-frontend/src/hooks/apiResources/sqlLab.ts:
##########
@@ -0,0 +1,75 @@
+/**
+ * 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 { QueryResponse } from '@superset-ui/core';
+import { api, JsonResponse } from './queryApi';
+
+export type InitialState = {
+ active_tab: {
+ id: number;
+ user_id: number;
+ label: string;
+ active: boolean;
+ database_id: number;
+ schema?: string;
+ table_schemas: {
+ id: number;
+ table: string;
+ description: {
+ columns?: {
+ name: string;
+ type: string;
+ }[];
+ dataPreviewQueryId?: string;
+ } & Record<string, any>;
+ schema?: string;
+ tab_state_id: number;
+ database_id?: number;
+ expanded?: boolean;
+ }[];
+ sql: string;
+ query_limit?: number;
+ latest_query?: QueryResponse | null;
+ autorun?: boolean;
+ template_params?: string | null;
+ hide_left_bar?: boolean;
+ saved_query?: { id: number } | null;
+ extra_json?: object;
+ };
+ databases: object[];
+ queries: Record<string, QueryResponse & { inLocalStorage?: boolean }>;
+ tab_state_ids: {
+ id: number;
+ label: string;
+ }[];
+};
+
+const queryValidationApi = api.injectEndpoints({
+ endpoints: builder => ({
+ sqlLabInitialState: builder.query<InitialState, void>({
+ providesTags: ['SqlLabInitialState'],
+ query: () => ({
+ endpoint: `/api/v1/sqllab/`,
+ headers: { 'Content-Type': 'application/json' },
+ transformResponse: ({ json }: JsonResponse) => json.result,
+ }),
+ }),
+ }),
+});
+
+export const { useSqlLabInitialStateQuery } = queryValidationApi;
Review Comment:
Maybe rename to `useSqlLabInitialState` or even `useInitialState`? We can always understand the full context given the package.
##########
superset-frontend/src/hooks/apiResources/sqlLab.ts:
##########
@@ -0,0 +1,75 @@
+/**
+ * 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 { QueryResponse } from '@superset-ui/core';
+import { api, JsonResponse } from './queryApi';
+
+export type InitialState = {
+ active_tab: {
+ id: number;
+ user_id: number;
+ label: string;
+ active: boolean;
+ database_id: number;
+ schema?: string;
+ table_schemas: {
+ id: number;
+ table: string;
+ description: {
+ columns?: {
+ name: string;
+ type: string;
+ }[];
+ dataPreviewQueryId?: string;
+ } & Record<string, any>;
+ schema?: string;
+ tab_state_id: number;
+ database_id?: number;
+ expanded?: boolean;
+ }[];
+ sql: string;
+ query_limit?: number;
+ latest_query?: QueryResponse | null;
+ autorun?: boolean;
+ template_params?: string | null;
Review Comment:
Do you explicitly need `null` or can we use `undefined`?
```suggestion
template_params?: string;
```
##########
superset-frontend/src/SqlLab/types.ts:
##########
@@ -33,16 +33,19 @@ export interface QueryEditor {
id: string;
dbId?: number;
name: string;
- schema: string;
+ title?: string; // keep in optional for backward compatibility
Review Comment:
```suggestion
title?: string; // keep it optional for backward compatibility
```
##########
superset-frontend/src/hooks/apiResources/sqlLab.ts:
##########
@@ -0,0 +1,75 @@
+/**
+ * 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 { QueryResponse } from '@superset-ui/core';
+import { api, JsonResponse } from './queryApi';
+
+export type InitialState = {
+ active_tab: {
+ id: number;
+ user_id: number;
+ label: string;
+ active: boolean;
+ database_id: number;
+ schema?: string;
+ table_schemas: {
+ id: number;
+ table: string;
+ description: {
+ columns?: {
+ name: string;
+ type: string;
+ }[];
+ dataPreviewQueryId?: string;
+ } & Record<string, any>;
+ schema?: string;
+ tab_state_id: number;
+ database_id?: number;
+ expanded?: boolean;
+ }[];
+ sql: string;
+ query_limit?: number;
+ latest_query?: QueryResponse | null;
+ autorun?: boolean;
+ template_params?: string | null;
+ hide_left_bar?: boolean;
+ saved_query?: { id: number } | null;
Review Comment:
Do you explicitly need `null` or can we use `undefined`?
```suggestion
saved_query?: { id: number };
```
##########
superset-frontend/src/SqlLab/reducers/getInitialState.ts:
##########
@@ -140,11 +136,12 @@ export default function getInitialState({
* hasn't used SQL Lab after it has been turned on, the state will be stored
* in the browser's local storage.
*/
- if (
- localStorage.getItem('redux') &&
- JSON.parse(localStorage.getItem('redux')).sqlLab
- ) {
- const { sqlLab } = JSON.parse(localStorage.getItem('redux'));
+ const localStorageData = localStorage.getItem('redux');
Review Comment:
It seems `redux` is only being used by SQL Lab files. Maybe we could extract this value to a constant and rename it to something more significant? `redux` gives the impression that the whole redux state is being persisted to the local storage but in fact is only the `sqlLab` path.
##########
superset-frontend/src/SqlLab/types.ts:
##########
@@ -53,6 +56,21 @@ export type toastState = {
noDuplicate: boolean;
};
+export type UnsavedQueryEditor = Partial<QueryEditor> & {
+ id?: QueryEditor['id'];
Review Comment:
Why do we need to redefine the `id` given that `Partial<QueryEditor>` makes all properties of `QueryEditor` optional?
##########
superset-frontend/src/hooks/apiResources/sqlLab.ts:
##########
@@ -0,0 +1,75 @@
+/**
+ * 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 { QueryResponse } from '@superset-ui/core';
+import { api, JsonResponse } from './queryApi';
+
+export type InitialState = {
+ active_tab: {
+ id: number;
+ user_id: number;
+ label: string;
+ active: boolean;
+ database_id: number;
+ schema?: string;
+ table_schemas: {
+ id: number;
+ table: string;
+ description: {
+ columns?: {
+ name: string;
+ type: string;
+ }[];
+ dataPreviewQueryId?: string;
+ } & Record<string, any>;
+ schema?: string;
+ tab_state_id: number;
+ database_id?: number;
+ expanded?: boolean;
+ }[];
+ sql: string;
+ query_limit?: number;
+ latest_query?: QueryResponse | null;
Review Comment:
Do you explicitly need `null` or can we use `undefined`?
```suggestion
latest_query?: QueryResponse;
```
--
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
[GitHub] [superset] justinpark commented on a diff in pull request #25047: chore(sqllab): typescript for getInitialState
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #25047:
URL: https://github.com/apache/superset/pull/25047#discussion_r1303402561
##########
superset-frontend/src/SqlLab/reducers/getInitialState.ts:
##########
@@ -140,11 +136,12 @@ export default function getInitialState({
* hasn't used SQL Lab after it has been turned on, the state will be stored
* in the browser's local storage.
*/
- if (
- localStorage.getItem('redux') &&
- JSON.parse(localStorage.getItem('redux')).sqlLab
- ) {
- const { sqlLab } = JSON.parse(localStorage.getItem('redux'));
+ const localStorageData = localStorage.getItem('redux');
Review Comment:
Once we consolidate the redux store with SPA store this (redux) will cover for all redux persist state but I agree with the naming. The problem is we should keep checking `redux` value because the legacy user still stores the previous session data in that key. so it's not hard but hard to maintain/manage new key value and old key value.
--
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