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

Fix CoroutineScope in MainViewModel fetchUser() #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix CoroutineScope in MainViewModel fetchUser() #2

wants to merge 1 commit into from

Conversation

captswag
Copy link

@captswag captswag commented Dec 28, 2020

I was going through this blog article https://blog.mindorks.com/mvi-architecture-android-tutorial-for-beginners-step-by-step-guide and downloaded this repo to learn more about MVI.

Anyways, I found this error (sort of) in the MainViewModel fetchUser(). The problem here is fetchUser() is already called from a different CoroutineScope. So instead of launching a new viewModelScope, we can reuse the previous one. So the PR request I raised removes the viewModelScope.launch {} and instead makes the fetchUser() suspend.

@mirokolodii
Copy link

mirokolodii commented Feb 7, 2021

@captswag scope here is only one - viewModelScope, but within this scope several coroutines are launched. Each viewModelScope.launch launches a new coroutine within the same scope.
So in the code we have one coroutine, which is created to listen to new intents and it basically has the same lifecycle as ViewModel.
Then, inside this coroutine, a child coroutine is created to fetch users, which 'lives' only for a duration of this intent.
I would not say your proposal is not correct, but it's not a bug you're fixing, but rather introduction of different behavior.
In original code, new intent can be accepted while previous is still ongoing. This is because for 'users fetching' intent there is a new coroutine created, which can be suspended, while the parent coroutine continue operating and can accept new intents.
In your proposal, no new intent can be accepted until previous one is completed. This is because all work is done within single coroutine.

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.

2 participants