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

Turbo 8: broadcasts_refreshes method doesn't propagate destroy or update by default #545

Closed
gobijan opened this issue Dec 15, 2023 · 6 comments

Comments

@gobijan
Copy link
Contributor

gobijan commented Dec 15, 2023

The broadcasts_refreshes method should be of least surprise for developers in my opinion.

Let's say we subscribe to a parentless top level model.
Currently it is implemented like this:

def broadcasts_refreshes(stream = model_name.plural)
    after_create_commit  -> { broadcast_refresh_later_to(stream) }
    after_update_commit  -> { broadcast_refresh_later }
    after_destroy_commit -> { broadcast_refresh }
end

Which basically means that it's not propagating the update and destroys.
Why is this? Is it because we assume that it always belongs to a parent?

To achieve full propagation of a parentless model I'd currently have to do something like this in the model:

broadcasts_refreshes_to ->(stream) { stream.class.broadcast_target_default }

Feels a little counter intuitive to me.
An alternative implementation that works for parentless collections would be this:

def broadcasts_refreshes(stream = broadcast_target_default)
    after_commit -> { broadcast_refresh_later_to(stream) }
end

Is the current behaviour explicitly wanted?
If so would it make sense to create a method for parentless collections that propagates all commits implemented like the proposal?

def broadcasts_refreshes_all(stream = broadcast_target_default)
    after_commit -> { broadcast_refresh_later_to(stream) }
end

I'd be more than happy to create a PR but let's first discuss.
Cheers Bijan

Note: broadcast_target_default is defined as a call to model_name.plural
Therefore it would be more consistent to use it as default parameter value (like in other methods)

@brunoprietog
Copy link
Contributor

The updates are propagating correctly. Note that broadcast_refresh_to is not the same as broadcast_refresh.

For create, the stream is the plural form of the model. For the rest, the stream is the instance of the record.

def broadcast_refresh_to(*streamables)
Turbo::StreamsChannel.broadcast_refresh_to *streamables unless suppressed_turbo_broadcasts?
end
def broadcast_refresh
broadcast_refresh_to self
end

@gobijan
Copy link
Contributor Author

gobijan commented Dec 19, 2023

Please not that I am referring to parentless models.
Here is a demo repo where I implemented a simple Todo App:

https://github.com/gobijan/todo-rails-realtime-morphing

@brunoprietog
Copy link
Contributor

Yes, it's the expected behavior. You have to subscribe for each model. In that case, you can add under this line the following:

https://github.com/gobijan/todo-rails-realtime-morphing/blob/0bda84fa84d3fef15df69b818b181c7e4a631519/app/views/todos/index.html.erb#L8

<% @todos.each do |todo| %>
  <%= turbo_stream_from todo %>
<% end %>

And in the model simply broadcasts_refreshes.

This is because you may also want to subscribe to the show action of each record, for example.

I recommend you to check basecamp/turbo-8-morphing-demo

@gobijan
Copy link
Contributor Author

gobijan commented Dec 19, 2023

I did check out that demo repo and looked at the implementation before I created my demo and this issue.

Beta 1 had a different implementation and behaviour than current main branch.
As I stated you don't need to subscribe in a loop. Instead this works for me in the todos example:

In the model:

broadcasts_refreshes_to` ->(stream) { stream.class.broadcast_target_default }

In the view:

<%= turbo_refreshes_with method: :morph, scroll: :preserve  %>
<%= turbo_stream_from "todos" %>

My point is that the behaviour (especially for quick demos) is a little unexpected from developers perspective.
Why is a create broadcasted globally by default but update and delete not?

@brunoprietog
Copy link
Contributor

Why is a create broadcasted globally by default but update and delete not?

That's because you don't have a record before creation. If you want to react to the creation, say, be subscribed to the channel before it happens, it has to be a global stream. For the rest, there is already a record to subscribe to. See #521.

@gobijan
Copy link
Contributor Author

gobijan commented Dec 19, 2023

Ok, that answer was helpful. So coming from the mindset that the usual case is to subscribe to a model with a parent anyways I can see why it's implemented like this. Guess we call it a close with comment for now. Thx for elaboration.

@gobijan gobijan closed this as completed Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants