[GitHub] [superset] codyml commented on a diff in pull request #21920: fix(explore): Chart save modal displays error instead of failing silently

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

codyml commented on code in PR #21920:
URL: https://github.com/apache/superset/pull/21920#discussion_r1003780253


##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -150,9 +151,6 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
   async saveOrOverwrite(gotodash: boolean) {
     this.setState({ alert: null, isLoading: true });
     this.props.actions.removeSaveModalAlert();
-    // eslint-disable-next-line @typescript-eslint/no-unused-vars
-
-    let promise = Promise.resolve();

Review Comment:
   Changing promises to `await`s looks great, thanks!  Two related notes:
   1. Since all the `catch` blocks are the same, should we just wrap the entire function body in one `try` block?
   2. Previously, since these promises had no `catch` block, errors would go unhandled and therefore show up in the browser console.  Should we preserve that behavior by re-throwing or not catching errors?
   
   If you think yes for both of those, we could just do something like:
   ```javascript
   async saveOrOverwrite(goodish:boolean) {
     try {
       // If anything throws here, modal will stay open, finally block will execute,
       // and error will remain unhandled because there's no catch block
       this.props.onHide();
     } finally {
       this.setState({ isLoading: false });
     }
   }



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