When API requests fail, we used to print the error message returned by the API, eg. The destination file already exists.. After switching to Axios, we now print the generic web request error, eg. Request failed with status code 400. As you can see, the generic error is meaningless and doesn't tell you what went wrong and how to fix it. We should therefore bring back the old behavior and show the original error as returned by the API.
This fix consists of two parts:
1. Update error processing:
Change:
|
if (response.error && |
|
response.error['odata.error'] && |
|
response.error['odata.error'].message) { |
|
return callback(new CommandError(response.error['odata.error'].message.value)); |
|
} |
to:
if (response.isAxiosError &&
response.response &&
response.response.data &&
response.response.data['odata.error'] &&
response.response.data['odata.error'].message) {
return callback(new CommandError(response.response.data['odata.error'].message.value));
}
I assume that because we're using Axios all error response objects we get on failed promises have this shape, but we'd need to double check that.
2. Update all tests
We have a number of tests where we mock the error object using the old structure (response object with an error property). We'd need to find all occurrences and update them to match the new structure (replace error with response.data).
Not sure if we should keep the isAxiosError property to ensure that we're dealing with an Axios error. It gives us an extra level of confidence, but I wonder if we can get non-Axios errors in this code path rendering this check unnecessary.
When API requests fail, we used to print the error message returned by the API, eg.
The destination file already exists.. After switching to Axios, we now print the generic web request error, eg.Request failed with status code 400. As you can see, the generic error is meaningless and doesn't tell you what went wrong and how to fix it. We should therefore bring back the old behavior and show the original error as returned by the API.This fix consists of two parts:
1. Update error processing:
Change:
cli-microsoft365/src/Command.ts
Lines 219 to 223 in 7d5d60e
to:
I assume that because we're using Axios all error response objects we get on failed promises have this shape, but we'd need to double check that.
2. Update all tests
We have a number of tests where we mock the error object using the old structure (response object with an
errorproperty). We'd need to find all occurrences and update them to match the new structure (replaceerrorwithresponse.data).Not sure if we should keep the
isAxiosErrorproperty to ensure that we're dealing with an Axios error. It gives us an extra level of confidence, but I wonder if we can get non-Axios errors in this code path rendering this check unnecessary.