Skip to content
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

Merged
merged 19 commits into from Apr 26, 2021
Merged

stream_from #104

merged 19 commits into from Apr 26, 2021

Conversation

leastbad
Copy link
Contributor

@leastbad leastbad commented Jan 27, 2021

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 like User and AR model instances like User.first.

First, run the included rake task. It will make sure that CableReady is properly initialized with the ActionCable consumer:

rails g cable_ready:stream_from

With setup out of the way, here's what it looks like in practice for a string identifier:

<%= stream_from "i-am-a-teapot" %>
cable_ready["i-am-a-teapot"].console_log(message: "yo").broadcast

ActiveRecord models are transparently converted to sgids, if you View Source.

<%= stream_from User.first %>
cable_ready[User.first].console_log(message: "yo").broadcast

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.

<%= stream_from :i_am_a_teapot %>
<%= stream_from User.first %>
<%= stream_from User %>
<%= stream_from User.first, :i_am_a_teapot %>
<%= stream_from :i_am_a_teapot, 69 %>
<%= stream_from :i_am_a_teapot, :short, :stout %>

<button data-reflex="click->Runner#chain">Chain</button>
  def chain
    cable_ready[:i_am_a_teapot].console_log(message: "symbol").broadcast
    cable_ready[User.first].console_log(message: "resource").broadcast
    cable_ready[User].console_log(message: "class constant").broadcast
    cable_ready[User.first, :i_am_a_teapot].console_log(message: "resource, symbol").broadcast
    cable_ready[:i_am_a_teapot, 69].console_log(message: "symbol, integer").broadcast
    cable_ready[:i_am_a_teapot, :short, :stout].console_log(message: "symbol, symbol, symbol").broadcast
    morph :nothing
  end

This solution requires that CableReady be initialized with the memoized consumer in your application pack or controller index. It needs to include something like:

import consumer from '../channels/consumer'
import CableReady from 'cable_ready'
CableReady.initialize({ consumer })

Luckily, running rails g cable_ready:helpers will add those lines for you.

Resolves #91

@joshleblanc
Copy link
Contributor

joshleblanc commented Jan 27, 2021

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.

@leastbad
Copy link
Contributor Author

@paramagicdev and I couldn't get it to work. Webpacker was complaining about the static syntax and after a few hours, I doubled down on wanting to keep Stimulus stuff out of CableReady - the hoops you had to jump through to get it working are just not necessary given that they have to have Stimulus installed anyhow. Plus having it in their app means that they have the opportunity to customize it if they want to. This is a win.

SR v4 will be moving to the application.consumer = consumer convention, and I push it fairly aggressively in the controllers I put into the world. Once I have a chance to return more of my attention to StimulusConnect, I'll be amping up my efforts in this regard.

Do you have any feedback on the more significant aspects of the effort?

@joshleblanc
Copy link
Contributor

What hoops exactly?

What's wrong with just putting the registerController() call in the generator?

@leastbad
Copy link
Contributor Author

I'm just not wild about the implementation of registerController.js. It's not personal. The whole reliance on stimulus as a peer dependency and doing async imports... again, we couldn't get it working and we're not experts but we're not dumb dumbs, either. Konnor moreso than I in this regard. Copying it to the host app alongside other controllers works really well.

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.

@afomera
Copy link

afomera commented Jan 27, 2021

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 Current.account, : posts_needing_review and it'd string together those records and pull in with an identifier like: account:5:posts_needing_review for example. Does this implementation allow that kind of flexibility of mixing AR backed + other identifiers? (I'm in the middle of some stuff, or i'd check It out, so forgive me for being lazy here and asking)

See: https://github.com/hotwired/turbo-rails/blob/b3fab95cc3e73b1a2960f0ea4a3509e0d490a16a/app/helpers/turbo/streams_helper.rb#L30-L44

<%= stream_from(Current.account, :posts_needing_review)" %> for example is what I'd be looking at wanting to do most situations when I don't have a model I want to back it by.

And then in the CR call... it'd be ideal to be able to broadcast it to it.

cable_ready[CableReady::Stream].console_log(message: "yo, new post in review").broadcast_to(Current.account, :posts_needing_review)

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.

@leastbad
Copy link
Contributor Author

The stream-from controller is not something you have to ever interact with, which is very much in line with the Rails Doctrine approach of providing sharp knives.

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 index.js - wherever you keep it. Would that be more palatable?

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. 😅

@afomera
Copy link

afomera commented Jan 28, 2021

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 much any JS and stick to Ruby that's unlocking powerful abilities (and leaving the sharper tools in the shed).

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.

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:

<%= turbo_stream_from Current.account, "discussions" %>

Outputs:

<turbo-cable-stream-source channel="Turbo::StreamsChannel" signed-stream-name="ImRpc2N1c3Npb25zIg==--403537b4985501dd7c7f94d25da6858991b6f605ee570e5ddd932b5729679802"></turbo-cable-stream-source>

and... example usage in a callback...

after_create_commit  -> { broadcast_prepend_to(Current.account, "discussions") }

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:

  • A shared JS file and a view helper that when called would automatically subscribe to and listen for the CR received function (which the JS written satisfies)
  • An easy way to pass identifiers to CR that matches what we could pass to the stream_from view helper. Which is one of the things I really liked about the implementation in add "stream_from" functionality #91.

But maybe it's clear CR wants to go in a completely different direction for point 2, which is your right as maintainers.

@leastbad
Copy link
Contributor Author

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. combo, sausage, pizza, hat, snake, plop, smoosh, stuff are all strong, strong candidates.

@joshleblanc
Copy link
Contributor

Seems like we're jumping through hoops to get around MessageVerifier at that point 😄

@leastbad
Copy link
Contributor Author

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 stream_from helper as well as a way to build the string identifier without manual concatenation as @afomera wants, without making any changes to the existing CableReady API.

@afomera
Copy link

afomera commented Jan 28, 2021

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 stream_name(*args) or named_stream(*args)

Then when you're calling it, cable_ready[named_stream(User.first, :silly)].console_log(message: "yo").broadcast?

@leastbad
Copy link
Contributor Author

leastbad commented Jan 29, 2021

@afomera was right: the correct thing to do was to just handle it.

I would really appreciate it if someone could show me the correct way to put the compound function in one place in CableReady that can be included or required. Right now, the exact same function is in a private section inside both the helper and channels.rb. It's because I don't know how to do it any better. Help! Thanks @Paramagicdev!

I'm going to update the PR description with the latest syntax.

@leastbad
Copy link
Contributor Author

leastbad commented Feb 6, 2021

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 ?

@hopsoft
Copy link
Contributor

hopsoft commented Feb 8, 2021

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.

@leastbad
Copy link
Contributor Author

leastbad commented Feb 8, 2021

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!

@leastbad
Copy link
Contributor Author

I'm also open to consider renaming the web component to cable-ready or cable-ready-stream.

@KonnorRogers
Copy link
Contributor

KonnorRogers commented Apr 21, 2021

cable-stream (similar to turbo-stream)
stream-cable (almost like "streamable")

@leastbad
Copy link
Contributor Author

It's fair to say that we don't want to confuse anyone by being too close to turbo-stream.

Copy link
Contributor

@hopsoft hopsoft left a 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.

@leastbad leastbad merged commit daf7ddc into stimulusreflex:master Apr 26, 2021
@leastbad
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants