Skip to content
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.

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

Add custom exception handlers for ViewModels #15

Merged
merged 3 commits into from
Nov 29, 2022
Merged

Add custom exception handlers for ViewModels #15

merged 3 commits into from
Nov 29, 2022

Conversation

VitalyPeryatin
Copy link
Contributor

Fix this issue: #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

Signed-off-by: Vitaly Peryatin <peryatin.vitalik37@gmail.com>
Signed-off-by: Vitaly Peryatin <peryatin.vitalik37@gmail.com>
Copy link
Contributor

@Skeptick Skeptick left a comment

Choose a reason for hiding this comment

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

I created a develop branch. Please target it for this PR. When I have time, I will create a release.
Thank you!

@VitalyPeryatin VitalyPeryatin changed the base branch from main to develop November 21, 2022 09:21
@Starmel
Copy link
Contributor

Starmel commented Nov 21, 2022

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?

@VitalyPeryatin
Copy link
Contributor Author

VitalyPeryatin commented Nov 21, 2022

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

@Skeptick
Copy link
Contributor

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.

@Starmel
Copy link
Contributor

Starmel commented Nov 22, 2022

Okay

Signed-off-by: Vitaly Peryatin <peryatin.vitalik37@gmail.com>
@VitalyPeryatin
Copy link
Contributor Author

@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

@Skeptick Skeptick merged commit 24dea35 into adeo-opensource:develop Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants