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

DO NOT MERGE - live data & coroutines experiments #770

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yigit
Copy link

@yigit yigit commented Oct 8, 2019

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

This PR has two parts, just sending to share some ideas.

First part is about using the liveData builder instead of launching in scopes and calling setValue on a LiveData instance.
This allows tests to just use LiveData.asFlow() instead of LiveDataTestUtils. Also much nicer organization on how the value is computed (since there is only 1 place to look)
For the test case we want 2 events, unfortunately events get conflated with LiveData.asFlow and test coroutine dispatcher executes all runnables immediately (instead of first come first serve) but adding a yield between two emits in the ViewModel solves it (not great).

The second part is replacing Event class with a regular kotlin channel.
Channels have a fan-out functionality which is explicit (unlike in SingleLiveEvent which had implicit behavior that does not match LiveData). I find it much cleaner and has room to be generalized.

I don't think it makes sense to merge this PR (hence DO NOT MERGE), but i think there is room to move more code into coroutines / flows also cleanup this PRs code.
I think even the LiveData computations can be reduced into Flows all the way until UI (viewModel's public fields that is).

💡 Motivation and Context

Avoiding LiveDataTestUtil.

💚 How did you test it?

ShotViewModelTest

📝 Checklist

  • I ran ./gradlew spotlessApply before submitting the PR
  • I reviewed submitted code
  • [] I added tests to verify changes
  • All tests passing

🔮 Next steps

just check it

📸 Screenshots / GIFs

@@ -1,4 +1,4 @@
/*
/*

Choose a reason for hiding this comment

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

super nit: It looks like a space got added here accidentally. :)

@codingjeremy codingjeremy changed the base branch from master to main September 29, 2020 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants