Baseflow
chore: adding errorListener
Allow user to send a custom function to track loading image errors.
If the function is sent we also prevent rethrow from being called.
We were receiving millions of errors monthly on simple "socket exception".
It is a non breaking change because if not sent the plugin will behave exactly the same.
To use just add to CachedNetworkImage a function like this:
errorListener: (e) {
if (e is SocketException) {
print('Error with ${e.address} and message ${e.message}');
} else {
print('Image Exception is: ${e.runtimeType}');
}
},
When is the merge planned? Looks critical for firebase crashalytics. @douglasiacovelli
To be honest I'm not the owner of this repo. I've just reviewed because I wasn't sure about the code itself :) @renefloor seems to be the right person to do that
@renefloor hey there! since it's been some months since this was opened, do you need any help releasing this fix?
Hi @renefloor - big fan of the lib. Would like to throw another plus 1 onto merging this. Currently, my handled image errors are blowing up my crash reporting (and stopping on breakpoints).
This seems like a very reasonable solution and isn't too dissimilar to how Flutter's own Image works.
- Example from the
precachefunction - onError docs
/// If this builder is not provided, any exceptions will be reported to /// [FlutterError.onError]. If it is provided, the caller should either handle /// the exception by providing a replacement widget, or rethrow the exception.
This will also solve numerous issues
- 99 comment issue - https://github.com/Baseflow/flutter_cached_network_image/issues/336
- This StackOverflow issue
- https://github.com/Baseflow/flutter_cached_network_image/issues/144
- https://github.com/Baseflow/flutter_cached_network_image/issues/504
- https://github.com/Baseflow/flutter_cached_network_image/issues/809
I don't expect this PR get merged soon, given that one of the maintainers doesn't consider this as an issue https://github.com/Baseflow/flutter_cached_network_image/issues/336#issuecomment-632131248.
@renefloor I kindly ask you to consider the POV of dozens (maybe hundreds?) of different developers and users from this library. If you don't consider this as an issue, at least consider as an enhancement. Give the possibility to the library users to handle the error if they want. This PR doesn't change the codebase to force everyone to handle, it just gives the option to those who want to handle it properly. I just can't understand why this change can't be added. It doesn't make any logical sense to don't allow it.
@ggirotto I'm sorry for not keeping up to date with all issues, but the errorListener is added back in version 3.3.0. Does that work for you?
Hey @renefloor thanks for the quick response.
Correct me if I'm wrong, but as far as I understood the current errorListener implementation only provides an way of listening for errors that will be rethrow. The whole point of this PR and all open issues discussion is to provide an way of intercepting these errors and don't throw them. This is represented by this part of the PR implementation: https://github.com/Baseflow/flutter_cached_network_image/pull/777/files#diff-6d42299f2962fd3431a55752a54debe1dead5c35de1c4f7c224a716694e834cbR134-R138
The current implementation doesn't address the open issues requests, since the error will be rethrown anyway. The main goal of all requests - as far as I understood - is to avoid crash tools pollution with undesired connection issues that may happen when downloading images (timeouts, socket, handshake, etc...) since these kind of errors are completely useless to track because there's nothing the developer can do if the user connection is poor/offline, and the current implementation doesn't allow this behavior of intercepting these connection errors and discard them before they're sent to crash/analytics services and tools.
The rethrow is needed for the errorWidget to be triggered.
I expected the error widget not to be shown when we add an errorListener as we don't throw an exception, but it's actually still shown because it is in an invalid state. However, this makes the error in the errorWidget useless and also shows the error widget for the wrong reasons.
Without errorListener:
With errorListener:
PR #891 actually solves your issue in the correct way, because indeed, the ImageStreamCompleter reports the error if it's not having an error listener, so passing it to the ImageStreamCompleter is the right way to go.
It's still unclear to me how #891 fixes the issue that this PR solves. We still have no way of using the widget without needing to handle with unnecessary network errors being thrown. #891 adds an way of listening to errors when using CachedNetworkImageProvider, but that's something we already have when using CachedNetworkImage widget directly. This PR proposal is completely different. It avoids the error rethrow outside the widget scope, which is the main complaint of the library users.
From PR description:
Allow user to send a custom function to track loading image errors.
If the function is sent we also prevent rethrow from being called.
We were receiving millions of errors monthly on simple "socket exception".
It is a non breaking change because if not sent the plugin will behave exactly the same.
If you disagree of this implementation/behavior please let us know because if so I'll create a mirrored fork with this change.
@ggirotto the rethrow is needed for the error widget to be properly shown. You can see that that is also done by the official Flutter image providers, such as NetworkImage
The main issue is/was that the ImageStreamCompleter reports this using FlutterError.reportError when there are no error listeners attached to the ImageStreamCompleter. That is also mentioned and linked in the other PR:
if (!handled) {
FlutterError.reportError(_currentError!);
}
https://github.com/flutter/flutter/blob/7f20e5d18ce4cb80c621533090a7c5113f5bdc52/packages/flutter/lib/src/painting/image_stream.dart#L797
By linking the errorListener to the ImageStreamCompleter the rethrow is no longer registered using FlutterError.reportError and should not get into any error reporting tools.
The rethrow cannot be removed, because that breaks the Image widget, but it also doesn't need to be removed.
@renefloor Thanks for the update. Just to better understand, now if we specify a errorListener the http errors will not be forward to FlutterError.onError callback? So all we want to do this
CachedNetworkImage(
errorListener: (error) {
// Do something with the error
},
imageUrl: '<URL>',
)
?