-
-
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
stream_from #104
stream_from #104
Conversation
Why do we want to copy this controller to the user's project? This is our controller, and it would be our responsibility to maintain it. If we ever have to make changes to it, change it, enhance it, do absolutely anything to it, we can't update the user's version if we copy it in with a generator. |
@paramagicdev and I couldn't get it to work. Webpacker was complaining about the SR v4 will be moving to the Do you have any feedback on the more significant aspects of the effort? |
What hoops exactly? What's wrong with just putting the |
I'm just not wild about the implementation of If we're really genuinely concerned about being able to upgrade the controller in the future, and we're confident that just telling them to re-run the generator is not a viable solution, we could distribute the controller as an npm package that they could register. However, I think this is wild overkill. I see a lot of positive upside to the developers getting a copy of the controller in their project. They are developers, not morons. Anyhow, it's still early, Josh. I posted this to illustrate what I'd like to see in #91, and to show precisely what I meant about the MessageVerifier stuff not being required, that we can do everything you seem to want to do without complicating the server-side API. |
I'm personally not a fan of having to copy the controller into my app and then have it sit around forever and ever and look at it, when I don't need to touch it. Just a personal preference here though, I can probably learn to live with it, as long as this is the only controller I have to add to my app regardless of how many different places I use the Stream_from tag. If I want to customize the interface/make my own AC channel, I know what I'm getting into and can make my own channel/JS file to subscribe. I just won't love having to remember to ensure this file is up to date / moved again to the correct location in our work app (we don't use app/javascript/controllers, we're a little more a sectioned off because large app is large 😅) (But I guess I can Learn To Deal With It ™️) That's one the beauties of Turbo's offering. I literally don't see it, because I don't need to touch it. It Just Works ™️. I know it's there behind the scenes, being awesome, doing the Good Work with JS I never need to look at. One thing Turbo also allows with the MessageVerifier stuff they do is saying like
And then in the CR call... it'd be ideal to be able to broadcast it to it.
As a side note, I don't love this API for streaming to it. It's very verbose and I don't love that when I'm reading the operations that are gonna happen I have to remember and then be like 'okay now where are we sending them'. Again, maybe this is just me, I can deal with this too lol. My reasoning for this is because this multiple identifier flexibility allows me to say okay on this page I want to subscribe to this account's :posts_needing_review and then when one comes in on the backend, stream to it. Then I can do multiple stream_from tags in different partials and whatever and wherever those partials are rendered at, they'll always subscribe and update themselves as needed. |
The You cannot access the full power of CableReady if you're not comfortable working with JavaScript (see here and here). Making JavaScript disappear is not a goal of CableReady, even if using CableReady certainly means you'll write far less JS in terms of LOC. How would you feel about potentially distributing the controller as an npm package that would be automatically installed as a dependency of CableReady? This means you'd have to register the controller, once, in your CableReady has no ambition to emulate or provide syntax parity with Turbo. They are completely different libraries with completely different capabilities and ambitions. We designed the CableReady API that we want to use, and we like it very much. We'll continue to accept feature suggestions and borrow legitimately good ideas - OSS, huzzah! - but there really does come a point where if our design decisions don't work for someone, they really owe it to themselves to consider using other libraries that speak to them. We're really proud of what we've created and we'll continue making it better. As a custodian and co-curator of what defines "better", there's always going to be a responsibility to make hard decisions. That said, I just pushed a commit that adds support for multiple identifiers. You can now: <%= stream_from "i-am-a-teapot", :so_am_i, User.first %> and all three forms are now available to you: cable_ready["i-am-a-teapot"].console_log(message: "yo").broadcast
cable_ready[:so_am_i].console_log(message: "yo").broadcast
cable_ready[CableReady::Stream].console_log(message: "yo").broadcast_to(User.first) ... which is funny, because I just re-read your message and realized that you're talking about concatenation of a sort, not multiple identifiers. At the end of the day, all stream identifiers are just strings. Resource identifiers are just strings that have been generated with a convention. What you are describing (now that I read it) is just a string-based identifier. If you want to call <%= stream_from "user:#{user_id}:thirsty" %> Then you should just do so - we provide an excellent API for working with string identifiers. We're not slowly trying to turn CableReady into Turbo, though, so yes: the API for sending broadcasts is different. We are quite fond of it! It now seems as though I should remove the "multiple identifiers" commit because it actually is a completely different behavior from what you were asking for and could potentially confuse someone used to the Turbo API. Ahh, well. 😅 |
My point wasn't that I'm uncomfortable working with JS, it's that I shouldn't need to unless I want to, just because I want to take advantage of a small slice of CR's operations. I'm fine with either approach now (for the JS stuff) after sitting on it all day. Be it registering another package or generating the channel once so it's in my app's controllers. All of the apps that use CR also use Stimulus, so that's fine. I mean strictly speaking in regards to this PR, how cool is it that you can do a few commands from rails new (install stimulus, cable ready, run generator), drop a line in your views and then get to Cable Readying without writing any JS. Thats only going to help the wow factor for folks looking at CableReady compared to other libraries. Yes the libraries are different, but you have to be able to admit that for devs who don't want to write
Most of my views come from us heavily using CR and myself using other libraries and seeing where I feel like CR could be improved and reduce the need for me to instead use other libraries or workarounds/the same thing over and over (which we've had to do multiple times when a shared channel could do fine with a signed identifier). But... now I'm hesitant to provide more feedback down the road. I want to use CR... but with these improvements Turbo showed us. Also since we're so heavily using CR it doesn't make sense to introduce another library when it seems like something CR should expand to allow for In Turbo land, passing the array of things gets basically equated down into one identifier, which can be broadcasted to elsewhere (by passing the same array for example). Allowing me to be precise about which streams I want to get updates. So an example of the tag in the Dom:
Outputs:
and... example usage in a callback...
There's a lot of 'magic' happening there for their broadcasts, but from my POV when I see in the view the tag I know somewhere else in my codebase wherever I want to send things to that page, I use the same array of 'streamables'(their term in the codebase) and it all boils down into one signed name (that I don't care if it's pretty or not) under the hood for ActionCable to pick up on. As an aside, we do our own string concatenation inside the cable_ready[] already for work, and it works, but its... not my favorite to do it manually. I guess the two 'asks' I see here in the PR would be like this:
But maybe it's clear CR wants to go in a completely different direction for point 2, which is your right as maintainers. |
I really appreciate the detailed explanation in a normal voice with real code samples. I read everything carefully so your effort is well-spent. What would you say if there was a second helper provided that accepted arbitrary options and emitted a string that could be passed into a helper or a cable_ready call? <%= stream_from here(User.first, :groomed) %> cable_ready[here(User.first, :groomed)].console_log(message: "yo").broadcast This would seem like the best of all worlds, no? Suggestions for a method name accepted. |
Seems like we're jumping through hoops to get around MessageVerifier at that point 😄 |
This PR has included a full MessageVerifier/gid implementation since the beginning, but that's not really the critical point. The method I proposed last night can be used both as optional input to the |
I feel like it could be avoided by just re-considering and looking at how the library could just do it under the hood, but perhaps there's reasons too long to elaborate in a PR. Perhaps the helper could be named like Then when you're calling it, |
@afomera was right: the correct thing to do was to just handle it.
I'm going to update the PR description with the latest syntax. |
I just added a commit that has the Stimulus controller check to make sure Turbo Drive/TurboLinks isn't in cache preview mode before attempting to subscribe to a channel. It does seem silly to rapidly create+destroy+create a subscription for a few hundred ms of blink time. Plus, if you're truly disconnected, you won't be able to connect to the server anyhow. This came about because of an issue @jonsgreen was having with Stimulus+Turbo Drive where ActionCable didn't seem to be acking the subscription requests if they came too quickly. This scary-looking issue on the Rails repo suggests that there could be a thread-safety concern in the ActionCable server module, especially when there's a potential thundering herd problem. It could also "just" be a problem in beta versions of Turbo Drive, TBD. Anyhow, regardless, I think doing a basic check like this will conserve non-zero server resources. It might be appropriate to back-port this to @julianrubisch's channel generator from #95 ? |
I'm running some experiments to see if we can obviate the need for the generator. I may have some proposed minor changes behind that effort. |
So long as it's still possible for the developer to tweak their own local version as easily as running a generator, I'm excited! |
I'm also open to consider renaming the web component to |
|
It's fair to say that we don't want to confuse anyone by being too close to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this. I'm glad we let it sit for a while as I think it produced a much better result in the end.
Thanks to @joshleblanc and @afomera for providing the early ass-kicking required to get this PR where it needed to be. Thanks to @palkan and @julianrubisch for providing later ass-kicking to make sure we covered our bases, security-wise. |
Credit given to @joshleblanc, who had all of the smart ideas.
Proposal: we can implement a
stream_from
view helper that can handle multiple identifiers, and address it from the server using today's CR API. Identifiers can be Strings, Symbols, AR model classes likeUser
and AR model instances likeUser.first
.First, run the included rake task. It will make sure that CableReady is properly initialized with the ActionCable consumer:
With setup out of the way, here's what it looks like in practice for a string identifier:
ActiveRecord models are transparently converted to sgids, if you View Source.
The helper can accept multiple arguments, which will be compacted into a
:
separated string identifier. You'll be able to access this identifer again when you pass in the same arguments, in the same order.This solution requires that CableReady be initialized with the memoized consumer in your application pack or controller index. It needs to include something like:
Luckily, running
rails g cable_ready:helpers
will add those lines for you.Resolves #91