Closed VitalyPeryatin closed 1 year ago
Hello! Thanks for your suggestions.
I don't think it's a good idea to add a global error handler for the entire application. If one of the developers wishes to use the global handler, then this will affect all existing ViewModels. Now errors will not fall into the default Thread.setDefaultUncaughtExceptionHandler()
inside which error events are already being sent to Firebase.
Therefore, I think that it is better not to add such a behavior change feature to existing code.
I think it would be best to give the ability to customize CoroutineScope in subclasses.
To do this, it would be worth doing private val coroutineTags = hashMapOf<String, CoroutineScope>()
to public.
Further, all the necessary behavior (CoroutineExceptionHandlers
) can be implemented in the subclass of the base ViewModel for your project.
@Skeptick @AlexGladkov what do you think?
Hello, @Starmel ! Thank you for your response. I partly agree with you, but I think it's safe to add this code to the library and would be useful.
I saved an opportunity not to use a single CouroutineExceptionHandler for all ViewModels. Behavior of exception handling will not be changed by default relative to the current version of the library. But there is will be additionally option for applications which use 'BaseViewModels' with custom exception handlers. It is usually more safe to have base coroutine exception handler, because it will not to crash whole application if some exception will occur. I think it would be useful to promote your library because this is a fairly common case. But if developers don't want to use this feature, nothing will change in their code :)
I thought about your suggestion to make coroutineTags
to public. But it's not easy to do it in process of initialization of class. And if we will do it in runtime some coroutine tasks/scopes can be lost, unfortunately. I guess this way may lead to new bugs for other developers
I agree with @VitalyPeryatin.
It is probably worth clarifying in the documentation that when using a global handler you need to take care about crash reporting.
Also, I would not like to expose coroutineTags
outside because this is a kind of "internal" of this library.
Okay
@Skeptick I added a note about 'when using a global handler you need to take care about crash reporting' in the documentation. Thank you for your remark
Fix this issue: https://github.com/adeo-opensource/kviewmodel--mpp/issues/13
Add opportunities for: 1) Add common (shared) CoroutineExceptionHandlers for all ViewModels. It is convenient, because often application need only one single common ExceptionHandler for logging exceptions to Firebase Crashlytics 2) Add custom CoroutineExceptionHandler for current ViewModel. It adds more flexibility and may be useful in some cases. For example, on some screens we want to show snackbars with exception descriptions