[GitHub] [superset] justinpark commented on a diff in pull request #23637: fix: Removes Redux state mutations - iteration 3

["justinpark (via GitHub)" <gi...@apache.org>]

justinpark commented on code in PR #23637:
URL: https://github.com/apache/superset/pull/23637#discussion_r1162060785


##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -250,6 +250,11 @@ class ChartRenderer extends React.Component {
       postTransformProps,
     } = this.props;
 
+    // TODO: queriesResponse comes from Redux store but it's being edited by
+    // the plugins, hence we need to clone it to avoid state mutation
+    // until we change the reducers to use Redux Toolkit with Immer
+    const mutableQueriesResponse = cloneDeep(queriesResponse);

Review Comment:
   This might cause the performance regression because it will deeply clone the same json response each rendering time (even though no queriesResponse updates).
   We should build the clone value only queriesResponse has been updated.
   
   (Since it's legacy component so need to revisit to fix later but the easiest way) My suggestion is using a clone reference in the construction and update during `shouldComponentUpdate` checking.
   
   ```
       constructor(props) {
          ...
          this.mutableQueriesResponse = cloneDeep(this.props.queriesResponse);
       }
   
        shouldComponentUpdate(nextProps, nextState) {
              ....
              this.hasQueryResponseChange =
                  nextProps.queriesResponse !== this.props.queriesResponse;
               if (this.hasQueryResponseChange) { this.mutableQueriesResponse = cloneDeep(nextProps.queriesResponse); }
           ...
   
           render() {
                 ...
                 queriesData={this.mutableQueriesResponse}
   ```
   
   



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