Improve DioErrors
#16
Conversation
dio/lib/src/dio_error.dart
Outdated
| if (error != null) { | ||
| msg += '\nError: $error'; | ||
| } | ||
| if (error is Error && error.stackTrace != stackTrace) { |
There was a problem hiding this comment.
I would prefer to hide stack traces from when describing a DioError, WDYT?
There was a problem hiding this comment.
Why do you prefer it?
I would prefer to get rid of the wrapping of exceptions, which is the cause of it. But as long as the wrapping of exceptions is done, the inner stacktrace might be helpful for debugging, so it should be included in the message. Otherwise, there's no way to get that stacktrace when using error monitoring tools.
There was a problem hiding this comment.
Because it holds the stack trace. The stack trace can be accessed as a field of DioError, rather than wrapped into a describing method. Consider we have an exception handler, so we can do if (error is DioError) stackTrace = error.stackTrace. You'll usually report exceptions so the report will contain duplicate stack traces.
dio/test/interceptor_test.dart
Outdated
| dio.get('/fakepath3').catchError((e) => throw (e as DioError).message), | ||
| throwsA('test error'), | ||
| dio.get('/fakepath3').catchError((e) => throw (e as DioError)), | ||
| throwsA(isA<DioError>()), |
There was a problem hiding this comment.
Might use .having to obtain the previous error message.
Co-authored-by: Alex Li <[email protected]> Signed-off-by: Jonas Uekötter <[email protected]>
Signed-off-by: Jonas Uekötter <[email protected]>
| } | ||
| rethrow; | ||
| } on TimeoutException { |
There was a problem hiding this comment.
Is it necessary to remove this?
There was a problem hiding this comment.
Since the exceptions are now thrown in the future.timeout(...) callback, no TimeoutException is thrown.
If
onTimeoutis omitted, a timeout will cause the returned future to complete with a TimeoutException.
See https://api.dart.dev/stable/2.18.4/dart-async/Future/timeout.html
| ), | ||
| StackTrace.current, | ||
| ); | ||
| xhr.abort(); |
There was a problem hiding this comment.
Checking around the code again, should we return the closure here?
There was a problem hiding this comment.
Yeah, I think that makes sense.
There was a problem hiding this comment.
We might figure out a proper test for this, but no need to rush.
|
This also fixes cfug/dio#1498. |
Originally proposed in cfug/dio#1405
Changes are already being covered by existing tests.