-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
add "stream_from" functionality #91
Conversation
I love this! I've got numerous cases where I'd use that, stat. Just think we should find ways not to duplicate the existing broadcast_to functionality, i.e. wrap around it more cohesively |
This is cool stuff! It's significantly simpler than what I thought you were describing originally. I like that it takes advantage of Stimulus if it's present. I think that they'd have to call registerController after they'd defined their Stimulus application, which is just a documentation concern. To that end, I would like to bring you around to something that I will be pushing for when SR 4 comes along as well: I am strongly in favor of trying to get the Stimulus community to embrace setting application.consumer = consumer in their In the case of this PR, it would drastically simplify (and remove ambiguity from) the controller registration process because you could just tell people to register your controller in the same way that they likely register a few others, and the consumer just gets set once. This would mean we could also remove a substantial amount of complexity from SR for the same reason - there's no need to identify whether there's a consumer or not, you just access In terms of whether this "belongs" in CR proper or a separate library, I could make good arguments for both and I'm curious what @hopsoft will think. I have to admit that I personally like having full control of the process described in https://cableready.stimulusreflex.com/broadcasting-to-resources#we-dont-need-react-to-do-this-anymore since very few "simple scenarios" stay truly simple for very long. I feel genuinely weird that people are so excited to use view templates to set up ActionCable channels, because these are often the same folks who complain loudly and pedantically to "keep your business logic and presentation concerns separate" to the degree that they fuss about whether it's okay for a Job to know the DOM id of a component... but suddenly view helpers launching jobs is A-OK. 😀 Regardless of what gets decided, I think that this is super cool and I'll help document it as best I can. There's a small part of me that worries that implementing features from Turbo might broadcast that we're playing catch-up, but I will work hard to make sure we're always demonstrating that our tooling was flexible enough to accomodate this all along. |
Not sure we can wrap it more cohesively. The current channel just uses <%= cable_ready_stream_from "the battle of troy" %> cable_ready.stream_to("the battle of troy").console_log(message: "Hello!").broadcast
I'm actually kind of surprised action cable doesn't just expose a singleton. You would think that since you want to re-use the same consumer everywhere, that would be built in.
The hotwire view helper does the same thing this PR does, except in a custom element rather than a stimulus controller. No launching of jobs involved. Or was this a hyperbole? 😄 |
I could have sworn that the Turbo Streams broadcasting stuff was wrapped in jobs, but that could well have been a fever dream. I realize now that I assumed you were using stream_for but yes, stream_from makes good sense. At the end of the day, all stream identifiers are just strings. The stream_for/broadcast_to is just sugar. I agree that the |
Does this mean this change is safe? def [](identifier)
stream_name = stream_name_from(identifier)
@channels[stream_name] ||= CableReady::Channel.new(stream_name, operations)
end This would do away with the |
I don't fully understand what I'm looking at, TBH. I just meant that under the covers, ActionCable converts a stream_for resource to a string. I will attempt to look more carefully at the PR and figure out what you're really asking me. |
This is great. I wonder if we should go ahead and recommend this so we can simplify things a little. application.consumer = consumer @joshleblanc I think you'd be safe to make that last change and remove the |
I made those changes. I also moved |
Hey Josh, I finally had a chance to dig into this properly. Could we adapt your helper to make it shorter (is <%= stream_from my_model do |model| %>
<%= model.name %>
<% end %> You could always set up the method like Next up, I suspect you're over-thinking the MessageVerifier stuff. If you look at https://cableready.stimulusreflex.com/cableready-everywhere#activerecord you'll see how I recommend folks add an I saw that your approach had support for named, non-ActiveRecord model resource identifiers. I'm pretty sure that @julianrubisch figured out how to support AR and non-AR Global ID stuff for Futurism. Following that, the big news is that I'm pretty sure the existing cable_ready[CableReady::CableReadyChannel].console_log(message: "foo").broadcast_to(my_model) To me, the main thrust of this PR is that you give the users a helper that creates an element with a Stimulus controller that automatically subscribes to a common CR channel that can be used to stream to arbitrary resource blocks without having to write channel subscriber code on the client or create custom channels on the server. Well, boom - almost all of the magic is on the client. Your idea is intact even if we don't change anything on the server. Given the above, I would rename cable_ready[Stream].console_log(message: "foo").broadcast_to(my_model) ^^^ sexy ^^^ @afomera hopefully you like what you see! |
I believe there's a number of misconceptions here, but I'll need to take some time to really dig into it and confirm. Just off the top of my head
Again, need to dig into this more, but I have a strong feeling I'm miscommunicating something |
|
Started bringin myself back up to speed. The OP is out of date. The current equivelant of your last suggestion is actually just I'll update the original post. I don't really follow what you want me to change exactly. Maybe someone else can chime in, as I'm confused. |
@leastbad Some thoughts; I personally prefer the implementation as close as Turbo/Hotwire tbh, aka the MessageVerifier stuff as @joshleblanc lifted from the source. It gives the flexibility I'm hoping for to bring to our work app, and it doesn't involve me adding a sgid method to models. Additionally, it can be used without active record as you mentioned Maybe Futurism has it figured out to support it like you're saying, but this addition here in CR feels natural for anyone who's using Hotwire already. It also makes it easier for me to start with Turbo, and go, oh yeah this isn't great I want the clarity that CR's methods for operations provides, bam update the stream tag (or add the new one) an then start using CR in the places I'm broadcasting from. I actually agree re: changing the channel name the JS subscribes to... Perhaps The other thing I'll point and it's probably a minor thing, but I like the ability to use a different signed id for resources for different uses, so subscribing to updates, but if we take that signed ID you couldn't say look it up on an endpoint that may look up via signed id for another purpose. A HUGE motivation to me for this PR is the ability to drop in the helper tag, have it subscribe for me, and then to basically do virtually the same call I'm used to anyway, I think I'm starting to ramble so I'll cut it off here 😅 |
First off, this is just a proposal. I really liked the way hotwire handled isolated subscriptions to specific pages. I found it really helped developer confidence when using cable ready inside jobs.
This is a smaller PR than it looks. It just adds 2 features
The following is a light example of how you'd go about doing this:
Notes
stimulus dependency
I want to explicitly note that stimulus is not added as a dependency in this PR. It's added as a peer dependency, which means it's not included in the package itself.
If a user tries to register the stimulus controller without having stimulus installed, cable_ready will display a message and go on its merry way.
broadcast_to
This PR is almost a 1 to 1 copy of hotwire's
stream_from
implementation. They don't usebroadcast_to
, so I followed suit. I'm not sure how much this implementation conflicts with the existingbroadcast_to
methods in cable_ready.registerController
My first run at this, I just implicitly loaded the controller on an isolated consumer. However, the previously connected cable connection seemed to disconnect. I don't know enough about actioncable to say whether that's just intended behavior, of if there's a properly way to create an isolated connection.
I'd want it to be as hands-off as possible, so if it's possible to create the subscription without passing the consumer, I'd opt for that. If anyone knows how to do it.