-
Notifications
You must be signed in to change notification settings - Fork 328
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
Convert DOM IDs before submitting ActionBroadcastJob #485
Conversation
3c916e6
to
7b35e22
Compare
test/streams/streams_channel_test.rb
Outdated
options = { partial: "messages/message", locals: { message: "hello!" } } | ||
|
||
|
||
class ::MyModel |
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.
Have you tried re-using the existing Message
model declared elsewhere in the test suite? It's ActiveModel::Conversion-compliant:
diff --git a/test/streams/streams_channel_test.rb b/test/streams/streams_channel_test.rb
index f212266..5d47345 100644
--- a/test/streams/streams_channel_test.rb
+++ b/test/streams/streams_channel_test.rb
@@ -1,6 +1,5 @@
require "test_helper"
require "action_cable"
-require "active_model"
class Turbo::StreamsChannelTest < ActionCable::Channel::TestCase
include ActiveJob::TestHelper, Turbo::Streams::ActionHelper
@@ -194,15 +193,9 @@ class Turbo::StreamsChannelTest < ActionCable::Channel::TestCase
test "broadcasting action later with ActiveModel array target" do
options = { partial: "messages/message", locals: { message: "hello!" } }
-
- class ::MyModel
- include ActiveModel::Model
- attr_accessor :id
- end
-
- my_model = MyModel.new(id: 42)
- target = [my_model, "opt"]
- expected_target = "opt_my_model_42"
+ message = Message.new(id: 42)
+ target = [message, "opt"]
+ expected_target = "opt_message_42"
assert_broadcast_on "stream", turbo_stream_action_tag("prepend", target: expected_target, template: render(options)) do
perform_enqueued_jobs do
@@ -215,15 +208,9 @@ class Turbo::StreamsChannelTest < ActionCable::Channel::TestCase
test "broadcasting action later with multiple ActiveModel targets" do
options = { partial: "messages/message", locals: { message: "hello!" } }
-
- class ::MyModel
- include ActiveModel::Model
- attr_accessor :id
- end
-
- one = MyModel.new(id: 1)
+ one = Message.new(id: 1)
targets = [one, "messages"]
- expected_targets = "#messages_my_model_1"
+ expected_targets = "#messages_message_1"
assert_broadcast_on "stream", turbo_stream_action_tag("prepend", targets: expected_targets, template: render(options)) do
perform_enqueued_jobs do
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.
Ohhh thanks, that's nice, I wasn't looking hard enough :)
Hello @dirkjonker, sorry for the lateness. Would you mind resolving the conflicts? |
When broadcasting to a target with a suffix, i.e. ```ruby target: [active_record_instance, :some_attribute] ``` The array will get passed to `ActionView::RecordIdentifier.dom_id` which turn it into something like: `"active_record_instance_1234_some_attribute"`. This works fine when broadcasting synchronously, but because array gets serialized into a parameter for the `ActionBroadcastJob`, it ends up forming an illegal target, something like: `"#<ActiveRecordInstance:0x00000> some_attribute"` This seems to include a memory address so it should be pretty much guaranteed that it does not result in any predictable changes to the DOM. This behavior difference is a bit confusing and hard to debug. This change generates the `target` names before serializing the job parameters, to keep the behavior consistent between the regulater and `later` methods. Behavior for existing applications may or may not change, depending on whether people expect this not to work.
@brunoprietog thanks for bringing this up again, I resolved the conflicts |
Thanks @dirkjonker! |
When broadcasting to a target with a suffix, i.e.
The array will get passed to
ActionView::RecordIdentifier.dom_id
which turn it into something like:"active_record_instance_1234_some_attribute"
.This works fine when broadcasting synchronously, but because array gets serialized into a parameter for the
ActionBroadcastJob
, it ends up forming an illegal target, something like:"#<ActiveRecordInstance:0x00000> some_attribute"
This seems to include a memory address so it should be pretty much guaranteed that it does not result in any predictable changes to the DOM.
This behavior difference is a bit confusing and hard to debug.
This change generates the
target
names before serializing the job parameters, to keep the behavior consistent between the regulater andlater
methods.Behavior for existing applications may or may not change, depending on whether people expect this not to work.