Stop adding whitespace when parsing our html (#492)

* Uncomment selector_broadcaster_test to improve our coverage. The tests are passing and it will help us to continue adding more scenarios to check (like the whitespace issue seen on #487). * Also check for reflexID on selector broadcaster. This is just applying the changes suggested by @leastbad in the PR review. * Make selector test fail to show we were not outputting the same html parsed. This is just to prove the issue we were having of morphdom needing to switch several parts of the HTML not because it changed, but because we added newlines to the output when we parsed it. To make the spec pass and show what we were doing, just change the expected html output to be: `"html" => "<div>bar</div>\n<div>baz</div>"` * Make page test fail to show we were not outputting the same html parsed. This is just to prove the issue we were having of morphdom needing to switch several parts of the HTML not because it changed, but because we added newlines to the output when we parsed it. To make the spec pass and show what we were doing, just change the expected html output to be: `"html" => "<div>New Content</div>\n<div>Another Content</div>"` * Stop adding whitespace when parsing our html on page morph. As shown by @shepmaster on #487, we were adding extra whitespace to the DOM when parsing. That was causing morphdom to do several replaces on the first page load since it was comparing a page without the whitespace and the "new" page with all the whitespaces added. The solution was just saying for Nokogiri to use the DEFAULT_HTML but without doing the FORMAT part. Closes #487 * Stop adding whitespace when parsing our html on selector morph.
stimulusreflex · Apr 30, 2021 · 9a94d69 · 9a94d69
1 parent a03c5c4
commit 9a94d69
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 55 deletions.
diff --git a/lib/stimulus_reflex/broadcasters/broadcaster.rb b/lib/stimulus_reflex/broadcasters/broadcaster.rb
@@ -5,6 +5,9 @@ class Broadcaster
     attr_reader :reflex, :logger, :operations
     delegate :cable_ready, :permanent_attribute_name, :payload, to: :reflex
 
+    DEFAULT_HTML_WITHOUT_FORMAT = Nokogiri::XML::Node::SaveOptions::DEFAULT_HTML &
+      ~Nokogiri::XML::Node::SaveOptions::FORMAT
+
     def initialize(reflex)
       @reflex = reflex
       @logger = Rails.logger if defined?(Rails.logger)

diff --git a/lib/stimulus_reflex/broadcasters/page_broadcaster.rb b/lib/stimulus_reflex/broadcasters/page_broadcaster.rb
@@ -12,7 +12,7 @@ def broadcast(selectors, data)
       selectors = selectors.select { |s| document.css(s).present? }
       selectors.each do |selector|
         operations << [selector, :morph]
-        html = document.css(selector).inner_html
+        html = document.css(selector).inner_html(save_with: Broadcaster::DEFAULT_HTML_WITHOUT_FORMAT)
         cable_ready.morph(
           selector: selector,
           html: html,

diff --git a/lib/stimulus_reflex/broadcasters/selector_broadcaster.rb b/lib/stimulus_reflex/broadcasters/selector_broadcaster.rb
@@ -17,7 +17,7 @@ def broadcast(_, data = {})
             operations << [selector, :morph]
             cable_ready.morph(
               selector: selector,
-              html: match.inner_html,
+              html: match.inner_html(save_with: Broadcaster::DEFAULT_HTML_WITHOUT_FORMAT),
               payload: payload,
               children_only: true,
               permanent_attribute_name: permanent_attribute_name,

diff --git a/test/broadcasters/page_broadcaster_test.rb b/test/broadcasters/page_broadcaster_test.rb
@@ -12,7 +12,7 @@ class StimulusReflex::PageBroadcasterTest < StimulusReflex::BroadcasterTestCase
   test "performs a page morph on body" do
     class << @reflex.controller.response
       def body
-        "<html><head></head><body>New Content</body></html>"
+        "<html><head></head><body><div>New Content</div><div>Another Content</div></body></html>"
       end
     end
 
@@ -24,7 +24,7 @@ def body
         "morph" => [
           {
             "selector" => "body",
-            "html" => "New Content",
+            "html" => "<div>New Content</div><div>Another Content</div>",
             "payload" => {},
             "childrenOnly" => true,
             "permanentAttributeName" => nil,

diff --git a/test/broadcasters/selector_broadcaster_test.rb b/test/broadcasters/selector_broadcaster_test.rb
@@ -1,59 +1,63 @@
 # frozen_string_literal: true
 
-# require_relative "broadcaster_test_case"
+require_relative "broadcaster_test_case"
 
-# class StimulusReflex::SelectorBroadcasterTest < StimulusReflex::BroadcasterTestCase
-# test "morphs the contents of an element if the selector(s) are present in both original and morphed html fragments" do
-# broadcaster = StimulusReflex::SelectorBroadcaster.new(@reflex)
-# broadcaster.append_morph("#foo", "<div id=\"foo\"><span>bar</span></div>")
+module StimulusReflex
+  class SelectorBroadcasterTest < StimulusReflex::BroadcasterTestCase
+    test "morphs the contents of an element if the selector(s) are present in both original and morphed html fragments" do
+      broadcaster = StimulusReflex::SelectorBroadcaster.new(@reflex)
+      broadcaster.append_morph("#foo", '<div id="foo"><div>bar</div><div>baz</div></div>')
 
-# expected = {
-# "cableReady" => true,
-# "operations" => {
-# "morph" => [
-# {
-# "selector" => "#foo",
-# "html" => "<span>bar</span>",
-# "childrenOnly" => true,
-# "permanentAttributeName" => nil,
-# "stimulusReflex" => {
-# "payload" => {},
-# "some" => :data,
-# "morph" => :selector
-# }
-# }
-# ]
-# }
-# }
+      expected = {
+        "cableReady" => true,
+        "operations" => {
+          "morph" => [
+            {
+              "selector" => "#foo",
+              "html" => "<div>bar</div><div>baz</div>",
+              "payload" => {},
+              "childrenOnly" => true,
+              "permanentAttributeName" => nil,
+              "stimulusReflex" => {
+                "some" => "data",
+                "morph" => "selector"
+              },
+              "reflexId" => "666"
+            }
+          ]
+        }
+      }
 
-# assert_broadcast_on @reflex.stream_name, expected do
-# broadcaster.broadcast nil, some: :data
-# end
-# end
+      assert_broadcast_on @reflex.stream_name, expected do
+        broadcaster.broadcast nil, some: :data
+      end
+    end
 
-# test "replaces the contents of an element and ignores permanent-attributes if the selector(s) aren't present in the replacing html fragment" do
-# broadcaster = StimulusReflex::SelectorBroadcaster.new(@reflex)
-# broadcaster.append_morph("#foo", "<div id=\"baz\"><span>bar</span></div>")
+    test "replaces the contents of an element and ignores permanent-attributes if the selector(s) aren't present in the replacing html fragment" do
+      broadcaster = StimulusReflex::SelectorBroadcaster.new(@reflex)
+      broadcaster.append_morph("#foo", '<div id="baz"><span>bar</span></div>')
 
-# expected = {
-# "cableReady" => true,
-# "operations" => {
-# "innerHtml" => [
-# {
-# "selector" => "#foo",
-# "html" => "<div id=\"baz\"><span>bar</span></div>",
-# "stimulusReflex" => {
-# "payload" => {},
-# "some" => :data,
-# "morph" => :selector
-# }
-# }
-# ]
-# }
-# }
+      expected = {
+        "cableReady" => true,
+        "operations" => {
+          "innerHtml" => [
+            {
+              "selector" => "#foo",
+              "html" => '<div id="baz"><span>bar</span></div>',
+              "payload" => {},
+              "stimulusReflex" => {
+                "some" => "data",
+                "morph" => "selector"
+              },
+              "reflexId" => "666"
+            }
+          ]
+        }
+      }
 
-# assert_broadcast_on @reflex.stream_name, expected do
-# broadcaster.broadcast nil, some: :data
-# end
-# end
-# end
+      assert_broadcast_on @reflex.stream_name, expected do
+        broadcaster.broadcast nil, some: :data
+      end
+    end
+  end
+end