Refactoring to unlink Streams from Models

[JulianFeinauer]

Solves #14

This is not yet fully polished but I wanted to hear Feedback from others like @davish and @blopker about these changes.

Basically, I subclassed BroadcastableMixin with a BroadcastableModelMixin which contains the Model spezific logic and migrated all model-specific tasks from Consumers notify method there. So now we should be able to send general Messages with the consumer.

As Demo I added the wiretap view ("/wiretap") into the Chat Demo which receives all Messages send in all Channels.

What do you think?

[JulianFeinauer]

Thanks for taking a shot at this! After looking over the code and the wiretap example, I'm wondering if the new BroadcastableMixin is truly general -- wiretap seems to still make use of the Message class, even if it's not saving it to the database. It seems to be making the assumption that the payload for broadcasts will always be an object, which, in Python, I don't think is a requirement we need to pass along to developers.

I think a lot of this comes down to subclassing from BroadcastableModelMixin and trying to fit both model-oriented streams and non-model streams into the same call path / class hierarchy. The explicit isinstance checks in some helper functions seem a bit confusing to me, and also seem to be a symptom of trying to use the same call paths for both cases.

It would be great to hear from @danjac and others who are using streams that aren't tied to models to see what their use cases are, and how we could generalize that into a separate set of functions, or maybe a StreamsMixin for views, that would allow for non-model streams without trying to fit into the same framework as model-based streams.

Well... I wrote quite a long Answer and it... went away... thank you Github.

Generally speaking, I agree that we have not enough use cases to explore if the BroadcastableMixin makes sense on its own. We could also merge it back with the BroadcastableModelMixin but at least I migrated all the Model specific logic from the consumer in there.

My two examples "/wiretap" and "/broadcast" both use the Model because I re-use the view. I can do another version that does not re-use the view to show you another example.

But generally I agree that we have no suitable use case to reason about the "right" amount of abstraction that we have to put in here.

How should we proceed from here? I think it would be good to have a clear example of what exactly we want model as example with this feature.

[JulianFeinauer]

I had a Slack discussion with @davish which I quote here for convenience:

j.feinauer vor 18 Minuten @davish I improved the broadcast example now to NOT use a Model. Basically, the room_detail now registers at a second stream “broadcasts” as well where he then receives a broadvcast message that is rendered differently. To test it, just go in a room and then hit ‘/broadcast’ and you should see the broadcast in your room

j.feinauer vor 17 Minuten The TriggerBroadcast is IMHO a nice example how to trigger a Stream from a View (which is a pretty typical case, in many scenarios, I think). But it would surely make sense to extract the Base into a general class that can be used everywhere, likely weißes_häkchen augen erhobene_hände

davish vor 14 Minuten I’m just not sure if a class is the right abstraction we want to provide — a mixin makes sense for models since the ORM is very object oriented, but a lot of things in Python are just built around functions, so I feel like we should provide functions for broadcasting/subscribing that aren’t wrapped in a class

So I added a standalone function to perform the broadcast and added another demo view ("/broadcast2") which is views.second_broadcast which is a 2 liner.

Note I had to subscribe to another channel now as well on the client side, to receive broadcasts from this 'broadcast' channel as well!

[davish]

But generally I agree that we have no suitable use case to reason about the "right" amount of abstraction that we have to put in here.

How should we proceed from here? I think it would be good to have a clear example of what exactly we want model as example with this feature.

@JulianFeinauer I totally agree, it's tough to reason about the "right" abstraction out of the gate –– I think it's easy to fall into the trap of premature abstraction that can make the code more difficult to reason about. Personally, I think there will have to be some model-specific code within the consumer to handle retrieving the model instance on the receiving end of the channel – see my comments above for why sending the instance in the channel_send call might be problematic in production. I believe that separating out BroadcastableMixin from BroadcastableModelMixin would be premature at this stage, since I'm not too sure about what it means semantically for an arbitrary to be "Broadcastable" beyond it just implementing the methods.

At this stage, it seems to me that the additional checks in turbo_stream_from make a lot sense to add to allow subscribing to arbitrary channels. I also think the turbo.send_broadcast() function is a great addition. There will need to be some plumbing I think with the special case of DB models, which we can case on model=True or something like that in the send_broadcast function and the consumer notify. With those two things (extended turbo_stream_from tag and a send_broadcast() function), developers can start playing around with non-model streams at a low-level, and from there we can see what abstractions make sense on top of that.

[JulianFeinauer]

I refactored the PR again and re-merged the BroadcastableMixin and BroadcastableModelMixin as suggested by you. So basically, all thats left is the possibility to send non-Model messages.

Regarding the de-/serialization issue we should consider opening a new issue for that, or?

[davish]

@JulianFeinauer made the model refactor -- let me know what you think!

[JulianFeinauer]

LGTM (I cannot formally approve).

[JulianFeinauer]

Finally merged 🎉 Thanks for the awesome work @davish