adventures-in / chat_app

A group effort to build a chat app and learn as we go.
18 stars 0 forks source link

Determine stream management strategy for database service #209

Closed nickmeinhold closed 4 years ago

nickmeinhold commented 4 years ago

issue #205 with PR #204 brought up a discussion about how to manage the stream(s) created in the database service

Working on "Don't repeat date in each message" (#172/#200), we came up with a solution where we modify each data event in the stream from firestore, adding a SectionDate item into the messages list whenever a message is on a new date.

This is an excellent opportunity to discuss our options for using and managing streams.

Relevant References:

Some questions for our particular case:

nickmeinhold commented 4 years ago

@tourdownunder I was having a look at the changes in #204 just now - the reason it doesn't throw the error when you open the same conversation twice is that the StreamController created in getMessagesStream is not added to _controllers. So it creates a new StreamController every time. I think that's one viable approach that's worth discussing (new controller each time). Anyway there's no need for the map - nothing is added to it, and if it's implemented as I believe you intended (where each new controller gets added to _controllers), you do indeed get the error when opening the same conversation twice.

nickmeinhold commented 4 years ago

Capturing previous discussion:

using a broadcast stream

would it be simpler to to use a broadcast stream (with .asBroadcastStream()) which doesn't throw the error when the stream is listened to more than once? (this is just off the top of my head and may be a bad approach)

Broadcast Stream is not a fix for this scenario. My mental model is television stations and that every unique conversation is a TV station. I don't think it is correct to reuse the same controller as its jumbling the signal.

@tourdownunder thanks for your answer, that makes sense to me when I think about it more and I think you're right - I imagine the idea seemed reasonable to me because when I use redux I have a controller (and stream) that gets used in multiple services but is only consumed in one place so it works nicely.

EDIT: haha I'm now less convinced - I need to think this through more... I might write some tests for learning sake and report back :-)

EDIT2: thinking about it more - in the original case where we had a single stream for all conversations, we were (as you say) jumbling the signal - and I would say that's true regardless of whether you use broadcast streams or not, and I think you were right that in that case broadcast streams was not a solution, but as it turns out your map solution throws the same error, you just have to wait till you open the same conversation again. But!... now that the jumbling signals problem is fixed by your map, using broadcast streams would (I think) solve the problem. I believe there were actually two problems disguised as one - the jumbling (solved by your map) and the stream re-use (solved by broadcast streams)... that's pretty cool :-)

EDIT3: In the end I disagree that a broadcast stream is not a fix in this scenario and that television stations is a helpful model here - we have a stream from the firestore that we listen to in the database service and a stream controller that the service uses to provide a separate stream that the widgets can listen to. I believe we could definitely use broadcast streams and a single stream controller - in this scenario there is only one widget listening to a stream at any given time and when the widget stops listening it cancels the database service stream (automatically done by the StreamBuilder's dispose) and presumably the function return will cancel the firestore stream (although we should probably keep a subscription and close manually to be sure). In the end (as far as I can tell) we don't have the potential for "jumbling the signal".