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

Remove morph broadcast helpers #647

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

omarluq
Copy link
Contributor

@omarluq omarluq commented Jul 16, 2024

Follow up to morph action API restructure by @seanpdoyle in hotwired/turbo#1240
This PR Removes morph stream action broadcast helpers and updates unit test and documentation for morph usage

@@ -279,6 +283,10 @@ def broadcast_replace(**rendering)
# # Sends <turbo-stream action="update" target="clearance_5"><template><div id="clearance_5">Other partial</div></template></turbo-stream>
# # to the stream named "identity:2:clearances"
# clearance.broadcast_update_to examiner.identity, :clearances, partial: "clearances/other_partial", locals: { a: 1 }
#
# # sends <turbo-stream action="update" method="morph" target="clearance_5"><template><div id="clearance_5">Other partial</div></template></turbo-stream>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the [method="morph"] come from in these code samples?

Would the default value of the :method attribute be "replace"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an optional attribute, updated the docs to reflect it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the default value would be replace but we don't need to explicitly pass it as you know! in case the method attribute is not present on the element the action will be processed as replace right?

assert_broadcast_on "stream", turbo_stream_action_tag("morph", target: "message_1", template: render(options)) do
Turbo::StreamsChannel.broadcast_morph_to "stream", target: "message_1", **options
test "broadcasting actions with method morph now" do
options = { method: :morph, partial: "messages/message", locals: { message: "hello!" } }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does :method need to be nested within an :attributes Hash option, and when can it be passed at the top level?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a miss on my part it should always be an optional key under attributes.

Update unit test
Update documentation
Copy link
Member

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much @omarluq 🙏

@jorgemanrubia jorgemanrubia merged commit 1428954 into hotwired:main Jul 19, 2024
15 checks passed
@searls
Copy link

searls commented Jul 24, 2024

I just tried implementing this after the release of 2.0.6 and wanted to share a challenge I had.

The first thing I tried was adding morph: to a turbo_stream.update method:

<%= turbo_stream.update @dom_id, method: :morph do %>

This didn't change the rendering of the tag, so I tried:

<%  turbo_stream_action_tag :update, target: @dom_id, method: :morph do %>

Which truncated the content in the block (whether or not I wrapped it in <template>).

This was the only formulation that worked:

<%= tag.turbo_stream(action: :update, target: @dom_id, method: :morph) do %>
  <template></template>
<% end %>

Finally, after reading this PR thread, I learned about the attributes keyword option:
Which seems to be the best working way to use the helper with the new morph update feature.

Path of least surprise: does it seem like the convenience methods of turbo_stream.update and turbo_stream.replace should support a top-level method param?

@seanpdoyle
Copy link
Contributor

seanpdoyle commented Jul 24, 2024

@searls from an ergonomics perspective, I think that special treatment of a :method keyword argument would be valuable. Having said that, for every method-level keyword option that we support, that's one fewer local variables the view partials will have available to them through their local_assigns.

If a view template relied on local assignments like :attributes, :target, and :method, could they be surprisingly nil or fail strict local assertions?

@searls
Copy link

searls commented Jul 24, 2024

Update: Whoops, my bad. That attributes above actually seems to have never worked. This is where I landed and it does work for multiple targets:

<%= tag.turbo_stream(action: :update, targets: @selector, method: :morph) do %>
  <template></template>
<% end %>

@seanpdoyle I'd leave up whether to pull up method as a special case up to you all, but I guess what I'm running into is just a sense of inconsistency. For example, I just had to update the above from:

<%= turbo_stream.update @dom_id, attributes: {method: :morph} do %>

To update_all, as the replaced item could appear multiple times on the page:

<%= turbo_stream.update_all @selector, attributes: {method: :morph} do %>

And it immediately stopped using morphing.

Because it's (apparently?) hard to tell if morphing is happening (I had to drop a debugger inside Turbo's source), asymmetries like this in the API could lead to people accidentally quietly disabling the feature without knowing it.

@brendon
Copy link

brendon commented Aug 21, 2024

It's not super clear, but was this ever resolved? I'm struggling to find any way to render an update with method: 'morph' from a controller. The turbo_stream.update method doesn't seem to allow any attributes passthrough to turbo_stream_action_tag.

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

Successfully merging this pull request may close these issues.

5 participants