[GitHub] [superset] eric-briscoe commented on a diff in pull request #20179: chore: Convert QueryAutoRefresh to TypeScript functional React component

[GitBox <gi...@apache.org>]

eric-briscoe commented on code in PR #20179:
URL: https://github.com/apache/superset/pull/20179#discussion_r881166798


##########
superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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 { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import * as Actions from 'src/SqlLab/actions/sqlLab';
+import { Query } from 'src/SqlLab/types';
+import useInterval from '../../utils/useInterval';
+
+const QUERY_UPDATE_FREQ = 2000;
+const QUERY_UPDATE_BUFFER_MS = 5000;
+const MAX_QUERY_AGE_TO_POLL = 21600000;
+const QUERY_TIMEOUT_LIMIT = 10000;
+
+interface UserOfflineFunc {
+  (offline: boolean): boolean;
+}
+
+interface RefreshQueriesFunc {
+  (alteredQueries: any): any;
+}
+
+interface Actions {
+  setUserOffline: UserOfflineFunc;
+  refreshQueries: RefreshQueriesFunc;
+}
+
+export interface QueryAutoRefreshProps {
+  offline: boolean;
+  queries: Query[];
+  actions: Actions;
+  queriesLastUpdate: number;
+}
+
+// returns true if the Query.state matches one of the specifc values indicating the query is still processing on server
+export const isQueryRunning = (q: Query): boolean =>
+  ['running', 'started', 'pending', 'fetching'].indexOf(q?.state) >= 0;
+
+// returns true if at least one query is running and within the max age to poll timeframe
+export const shouldCheckForQueries = (queryList: Query[]): boolean => {
+  let shouldCheck = false;
+  // if there are started or running queries, this method should return true
+  const now = Date.now();
+  if (queryList && typeof queryList === 'object') {
+    shouldCheck = Object.values(queryList).some(

Review Comment:
   @eschutho This is definitely a great area for discussion!  I usually try to make code runtime fault tolerant and verify that behavior in the unit tests.  Because TypeScript is only doing static code analysis at compile time it really provides not actual safety for the runtime execution.  TypeScript will catch a TON of places in code where the wrong type of value is passed, but it cannot guarantee null, undefined, invalid data ends up in redux or passed down on props at runtime.  The associated unit tests failed when I passed a null or undefined value so I added the explicit check if the value is both truthy and object because `typeof null === 'object'`.  
   
   Also verifying the typeof is object helps avoid odd Object.values behavior if someone happened to pass a string instead of object this will make each character of a string a value in the response array.  This portion of code is not performance sensitive where it is executing rapidly in a loop, or expensive to do these checks so I felt it was worth the check to make the code bomb proof.  Pretty flexible on this if anyone has strong opinions otherwise but my go to is to try to ensure there is no way a function can produce an uncaught exception. 
   
   React rendering loops can be pretty unforgiving where one call like object.values(null) in render loop can lead you to white screen of doom 😮 Trust but verify I always say :) 
   
   Another approach I have used in the past is to add utility functions that can be imported like isArray, isObject, isString, etc so things like `if (queryList && typeof queryList === 'object') {` can be reduced to `if (isObject(queryList)) {`.  Having those utilities can lead to consistent code practice for validating inputs and help eliminate potential runtime errors.



-- 
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