nhphong / nimble-surveys

0 stars 0 forks source link

Room integration #9

Open sleepylee opened 5 years ago

sleepylee commented 5 years ago

I'm not sure if I understand your intention right, but at your SurveyDAO and its final usage on binding the data back to ViewModel, it looks cumbersome to me when we have to convert it first with room-rxjava and then at the end, without any transformation, the data is set back to a LiveData source. Could you please elaborate if you find this is useful somehow in your experience? I'd love to know more about this, because imo, Room has exposed interfaces to interact with LiveData at its nature.

luongvo commented 5 years ago

My suggestion is we could implement LiveData directly on SurveyDAO class. How is your thinking? 🤔

nhphong commented 5 years ago

If I understand correctly, having Room DAO returns a LiveData will make the flow look like this: Room Database -> DAO -> Repository -> ViewModel -> View

So the View will subscribe directly to a LiveData object produced by the DAO. It means that if the field is changed in the database, it will notify the View immediately. (sounds like a real-time database)

However it is not what I wanted. What I wanted was having the ViewModel as a single source of truth that emits data on its purpose. For example, in this case, I want the ViewModel to emit data from the database only when the app is opened.

nhphong commented 5 years ago

@sleepylee @luongvo Feel free to correct me if my thinking was wrong.

sleepylee commented 5 years ago

@nhphong actually we're aligned on the idea of keeping the viewmodel as the single source of truth, what we meant is that you still can publish the changes to ViewModel's source not directly to the View, but instead of converting Room output source to Rx wrapper, we could convert it directly to LiveData type no? 🤔 https://link.medium.com/vXCbThQrNV

nhphong commented 5 years ago

I've read the link you shared. It is similar to the flow I mentioned before: Room Database -> DAO -> Repository -> ViewModel -> View

The View (MainActivity) subscribes to ViewModel.allWords image

The ViewModel gets that LiveData from Repository image

The Repository, in turn, gets the LiveData object from DAO image

In the end, the View will subscribe to a LiveData object produced by the DAO

sleepylee commented 5 years ago

well..if you want to have a separate observable source on ViewModel, you can subscribe to the DAO source and update to the VM's data source as well no? Just to confirm that your very first approach with rxjava is correct as well, but instead of subscribe via a Rx source, my point is that we can observe directly from the LiveData source or converting if needed, but I don't see a clear need of rxjava here? 🤔 how do you think?

nhphong commented 5 years ago

Right. We can create an LiveData instance within the ViewModel, and let it subscribe to the one created by the DAO.

However, if we follow that way, it will end up with a Repository like this image

A mix between RxObservables and LiveData in the interface of a Repository. 🤔 🤔 I think it would be harder to read. How do you think?

sleepylee commented 5 years ago

I agree it's not nice to mix both techniques in one, but indeed that's our concerns since the beginning 😄so, the question now is: can you propose a viable solution for only LiveData implementations to the rest 2 methods there for consistency? I don't think there is yet to be an official adapter for retrofit working with LiveData, but it doesn't mean impossible. The completable type is easy imo.

nhphong commented 5 years ago

Since LiveData is just an observable source, converting it to/from RxObservable is not a difficult task. What I'm concern here is whether we should do that. We already have a nice RxJava2CallAdapterFactory for Retrofit, but I never see people trying to implement a LiveData adapter for it (or at least, not yet 😅).

If the implementation is just for this technical challenge, we can give it a try though.

nhphong commented 5 years ago

On the second thought, converting RxJava to LiveData in this case takes time and effort, and it's not natural for the case of Retrofit. Since we already have a nice support for RxJava from Room, I think it's better that we keep this way for simplicity, well, unless you insist.