[GitHub] [superset] eric-briscoe commented on a diff in pull request #20281: feat: Visualize SqlLab.Query model data in Explore 📈

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

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


##########
superset-frontend/src/explore/components/DatasourcePanel/index.tsx:
##########
@@ -282,22 +293,22 @@ export default function DataSourcePanel({
   }, [columns, datasource, metrics]);
 
   const sortCertifiedFirst = (slice: ColumnMeta[]) =>
-    slice.sort((a, b) => b.is_certified - a.is_certified);
+    slice.sort((a, b) => b?.is_certified - a?.is_certified);
 
   const metricSlice = useMemo(
     () =>
       showAllMetrics
-        ? lists.metrics
-        : lists.metrics.slice(0, DEFAULT_MAX_METRICS_LENGTH),
-    [lists.metrics, showAllMetrics],
+        ? lists?.metrics
+        : lists?.metrics?.slice?.(0, DEFAULT_MAX_METRICS_LENGTH),

Review Comment:
   @eschutho I always try to write code that assumes every prop passed to a component will be the wrong data type, null, undefined so that it is impossible for the component to throw a runtime error.  
   
   TypeScript will only help us when we don't pass around object refs (without explicit new object mapping each value to attribute on new object) component to component which essentially makes us think we are doing type checking, but in reality the object can be mutated at any point along the chain of props between components drilling, or redux and end up in a bad state.  We also have cases where components that are not TypeScript migrated yet are in the mix.  
   
   While it is unlikely metrics would get set to something other than undefined, null, or array, if it does get an incorrect value assigned this would error out.  Whenever there is not a performance concern - something called repeatedly in loops, I prefer to make the code runtime bomb proof.  
   
   I am completely flexible on this approach and can dial back the `?.(` pre function invocation checks if that is a preferred approach.  Probably worth deciding and discussing with team either way to be consistent :)



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