Skip to content

Update: add catch blocks to handle exceptions #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tranjennie wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Update: add catch blocks to handle exceptions #39

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
04d4909
Update: add catch blocks to handle exceptions
tranjennie Jul 26, 2022
0c9611f
update package
tranjennie Aug 1, 2022
eea14aa
remove unnecessary catch block
tranjennie Aug 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9,332 changes: 5,141 additions & 4,191 deletions dist/api.min.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/api.min.js.map
View file
Open in desktop

Large diffs are not rendered by default.

11 changes: 10 additions & 1 deletion lib/ApiHelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ class ApiHelper {
}
credentials = data.credentials;
this.cred.sessionId = credentials.sessionId;
}).catch(err => {
throw err;
Copy link
Contributor

@beefo beefo Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tranjennie What issue does catching and re-throwing the err fix? We should add a unit test to cover this senario if possible.

Copy link
Author

@tranjennie tranjennie Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beefo I've removed this code. After some testing, I see that this was not catching any errors

Copy link
Contributor

@beefo beefo Aug 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tranjennie there are a bunch more of the same catch and re-thow paterns added.

 .catch(err => {
                    throw err;
                });

If the one above didn't do anything, I assume the other don't do anything?
If we remove all of these, there's no change here as far as I can tell. What precisely are we fixing here?

Copy link
Contributor

@beefo beefo Aug 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a legitimate fix here, please add a unit test for it.

});
return credentials;
}
Expand All @@ -132,6 +134,9 @@ class ApiHelper {
if(response.data.result){
return response.data.result;
}
})
.catch(err => {
throw err;
});
return call;
}
Expand Down Expand Up @@ -196,7 +201,9 @@ class ApiHelper {
} else {
return response;
}
})
}).catch(err => {
throw err;
});
} else {
call.then( response => {
if(response.data.result){
Expand All @@ -207,6 +214,8 @@ class ApiHelper {
}
}
return response;
}).catch(err => {
throw err;
});
}
return call;
Expand Down
13 changes: 11 additions & 2 deletions lib/AxiosCall.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ class AxiosCall {
data: this.encode(this.body),
headers: this.config.headers,
timeout: timeout * 1000
})
.catch(err => {
throw err;
});
// Normal callback behaviour if we have one
if (this.callbackSuccess) {
Expand All @@ -56,8 +59,9 @@ class AxiosCall {
} else {
this.callbackSuccess(data.result);
}
}
);
}).catch(err => {
throw err;
});
}

// If we got a callbackError, we can catch the error and use it
Expand All @@ -68,6 +72,11 @@ class AxiosCall {
}
);
}
else{
this.request.catch(err => {
throw err;
});
}
// Returning promise
return this.request;
}
Expand Down
Loading