Disable change tracking when passing dynamic assigns (#3923)

* Disable change tracking when passing dynamic assigns

When passing dynamic assigns to a component, we cannot make
any assumptions about what keys are considered changed, therefore
we need to disable change tracking.

Consider this example:

When a component is called and it receives dynamic attributes

```heex
<.my_component {@action[:attrs] || %{}}>{@action.text}</.my_component>
```

defined as

```elixir
attr :special, :boolean, default: false

defp my_component(assigns) do
  ~H"""
  <div style={if(@special, do: "background-color: red;")}>
    {render_slot(@inner_block)}
  </div>
  """
end
```

Now assuming it is rendered first with `@action` as `%{text: "No red"}`
and then toggling between that and `%{text: "Red", attrs: %{special: true}`:

Previously, once toggled, the background color would be stuck on red,
since `special` would not be considered changed, therefore the diff
for the style would not be sent.

* add e2e test

* adjust test that assumed change tracking for dynamic assigns

---------

Co-authored-by: José Valim <[email protected]>

Author: SteffenDE

lib/phoenix_live_view/engine.ex

@@ -773,8 +773,14 @@ defmodule Phoenix.LiveView.Engine do
 
     static_extra = extra ++ static
 
-    static_changed =
-      if static_extra != [] do
+    # Rewrite slots in static parts
+    static = slots_to_rendered(static, vars, caller, Keyword.get(meta, :slots, []))
+
+    cond do
+      # We can optimize when there are static parts and no dynamic parts.
+      # We skip live components because they have their own tracking.
+      static_extra != [] and dynamic == %{} and
+          not match?({:&, _, [{:/, _, [{:live_component, _, _}, 1]}]}, fun) ->
         keys =
           for {key, value} <- static_extra,
               # We pass empty assigns because if this code is rendered,
@@ -792,58 +798,24 @@ defmodule Phoenix.LiveView.Engine do
             {_assign, keys} -> keys
           end)
 
-        quote do
-          unquote(__MODULE__).to_component_static(
-            unquote(keys),
-            unquote(@assigns_var),
-            changed,
-            %{unquote_splicing(vars_changed_vars(vars_changed_keys))},
-            vars_changed
-          )
-        end
-      else
-        Macro.escape(%{})
-      end
-
-    static = slots_to_rendered(static, vars, caller, Keyword.get(meta, :slots, []))
+        static_changed =
+          quote do
+            unquote(__MODULE__).to_component_static(
+              unquote(keys),
+              unquote(@assigns_var),
+              changed,
+              %{unquote_splicing(vars_changed_vars(vars_changed_keys))},
+              vars_changed
+            )
+          end
 
-    cond do
-      # We can't infer anything, so return the expression as is.
-      static_extra == [] and dynamic == %{} ->
-        expr
-
-      # Live components do not need to compute the changed because they track their own changed.
-      match?({:&, _, [{:/, _, [{:live_component, _, _}, 1]}]}, fun) ->
-        if dynamic == %{} do
-          quote do: %{unquote_splicing(static)}
-        else
-          quote do: Map.merge(unquote(dynamic), %{unquote_splicing(static)})
-        end
+        quote do: %{unquote_splicing([__changed__: static_changed] ++ static)}
 
-      # We were actually able to find some static bits, but no dynamic.
-      # Embed the static parts alongside the computed changed.
       dynamic == %{} ->
-        quote do
-          %{unquote_splicing([__changed__: static_changed] ++ static)}
-        end
+        quote do: %{unquote_splicing(static)}
 
-      # Merge both static and dynamic.
       true ->
-        {_, keys, _} = analyze_and_return_tainted_keys(dynamic, vars, %{}, caller)
-        keys = to_component_keys(keys)
-
-        quote do
-          unquote(__MODULE__).to_component_dynamic(
-            %{unquote_splicing(static)},
-            unquote(dynamic),
-            unquote(static_changed),
-            unquote(keys),
-            unquote(@assigns_var),
-            changed,
-            %{unquote_splicing(vars_changed_vars(keys))},
-            vars_changed
-          )
-        end
+        quote do: Map.merge(unquote(dynamic), %{unquote_splicing(static)})
     end
   end
 
@@ -880,44 +852,6 @@ defmodule Phoenix.LiveView.Engine do
         do: {assign, changed}
   end
 
-  @doc false
-  def to_component_dynamic(
-        static,
-        dynamic,
-        _static_changed,
-        _keys,
-        _assigns,
-        nil,
-        _vars_changed_vars,
-        nil
-      ) do
-    merge_dynamic_static_changed(dynamic, static, nil)
-  end
-
-  def to_component_dynamic(
-        static,
-        dynamic,
-        static_changed,
-        keys,
-        assigns,
-        changed,
-        vars_changed_vars,
-        vars_changed
-      ) do
-    component_changed =
-      if component_changed(keys, assigns, changed, vars_changed_vars, vars_changed) do
-        Enum.reduce(dynamic, static_changed, fn {k, _}, acc -> Map.put(acc, k, true) end)
-      else
-        static_changed
-      end
-
-    merge_dynamic_static_changed(dynamic, static, component_changed)
-  end
-
-  defp merge_dynamic_static_changed(dynamic, static, changed) do
-    dynamic |> Map.merge(static) |> Map.put(:__changed__, changed)
-  end
-
   defp component_changed(:all, _assigns, _changed, _vars_changed_vars, _vars_changed), do: true
 
   defp component_changed([path], assigns, changed, vars_changed_vars, vars_changed) do

test/e2e/support/issues/issue_3919.ex

@@ -0,0 +1,37 @@
+defmodule Phoenix.LiveViewTest.E2E.Issue3919Live do
+  use Phoenix.LiveView
+
+  def mount(_params, _session, socket) do
+    {:ok, assign(socket, action: %{text: "No red"})}
+  end
+
+  def handle_event("toggle_special", %{}, socket) do
+    new_action =
+      if socket.assigns.action[:attrs] do
+        %{text: "No red"}
+      else
+        %{text: "Red", attrs: %{special: true}}
+      end
+
+    {:noreply, assign(socket, action: new_action)}
+  end
+
+  def render(assigns) do
+    ~H"""
+    <.my_component {@action[:attrs] || %{}}>{@action.text}</.my_component>
+
+    <button phx-click="toggle_special">toggle</button>
+    """
+  end
+
+  attr(:special, :boolean, default: false)
+  slot(:inner_block)
+
+  defp my_component(assigns) do
+    ~H"""
+    <div style={if(@special, do: "background-color: red;")}>
+      {render_slot(@inner_block)}
+    </div>
+    """
+  end
+end

test/e2e/test_helper.exs

@@ -196,6 +196,7 @@ defmodule Phoenix.LiveViewTest.E2E.Router do
       live "/3719", Issue3719Live
       live "/3814", Issue3814Live
       live "/3819", Issue3819Live
+      live "/3919", Issue3919Live
     end
   end
 

test/e2e/tests/issues/3919.spec.js

@@ -0,0 +1,32 @@
+import { test, expect } from "../../test-fixtures";
+import { syncLV } from "../../utils";
+
+// https://github.com/phoenixframework/phoenix_live_view/issues/3919
+test("attribute defaults are properly considered as changed", async ({
+  page,
+}) => {
+  await page.goto("/issues/3919");
+  await syncLV(page);
+
+  const styledDiv = page.locator("div[style]");
+
+  await expect(styledDiv).toContainText("No red");
+  await expect(styledDiv).not.toHaveAttribute(
+    "style",
+    "background-color: red;",
+  );
+  await page.getByRole("button", { name: "toggle" }).click();
+  await syncLV(page);
+
+  await expect(styledDiv).not.toContainText("No red");
+  await expect(styledDiv).toHaveAttribute("style", "background-color: red;");
+  await page.getByRole("button", { name: "toggle" }).click();
+  await syncLV(page);
+
+  // bug: previously, the red background remained
+  await expect(styledDiv).toContainText("No red");
+  await expect(styledDiv).not.toHaveAttribute(
+    "style",
+    "background-color: red;",
+  );
+});

test/phoenix_component/rendering_test.exs

@@ -175,38 +175,40 @@ defmodule Phoenix.ComponentRenderingTest do
       assigns = %{foo: 1, bar: %{foo: 2}, __changed__: %{}}
       assert eval(~H"<.changed foo={@foo} {@bar} />") == [nil]
 
+      # we cannot perform any change tracking when dynamic assigns are involved
       assigns = %{foo: 1, bar: %{foo: 2}, __changed__: %{bar: true}}
-      assert eval(~H"<.changed foo={@foo} {@bar} />") == [["%{foo: true}"]]
+      assert eval(~H"<.changed foo={@foo} {@bar} />") == [["nil"]]
 
       assigns = %{foo: 1, bar: %{foo: 2}, __changed__: %{foo: true}}
-      assert eval(~H"<.changed foo={@foo} {@bar} />") == [["%{foo: true}"]]
+      assert eval(~H"<.changed foo={@foo} {@bar} />") == [["nil"]]
 
       assigns = %{foo: 1, bar: %{foo: 2}, baz: 3, __changed__: %{baz: true}}
-      assert eval(~H"<.changed foo={@foo} {@bar} baz={@baz} />") == [["%{baz: true}"]]
+      assert eval(~H"<.changed foo={@foo} {@bar} baz={@baz} />") == [["nil"]]
     end
 
     test "with dynamic assigns" do
       assigns = %{foo: %{a: 1, b: 2}, __changed__: %{}}
       assert eval(~H"<.changed {@foo} />") == [nil]
 
+      # we cannot perform any change tracking when dynamic assigns are involved
       assigns = %{foo: %{a: 1, b: 2}, __changed__: %{foo: true}}
-      assert eval(~H"<.changed {@foo} />") == [["%{a: true, b: true}"]]
+      assert eval(~H"<.changed {@foo} />") == [["nil"]]
 
       assigns = %{foo: %{a: 1, b: 2}, bar: 3, __changed__: %{bar: true}}
-      assert eval(~H"<.changed {@foo} bar={@bar} />") == [["%{bar: true}"]]
+      assert eval(~H"<.changed {@foo} bar={@bar} />") == [["nil"]]
 
       assigns = %{foo: %{a: 1, b: 2}, bar: 3, __changed__: %{bar: true}}
-      assert eval(~H"<.changed {%{a: 1, b: 2}} bar={@bar} />") == [["%{bar: true}"]]
+      assert eval(~H"<.changed {%{a: 1, b: 2}} bar={@bar} />") == [["nil"]]
 
       assigns = %{bar: 3, __changed__: %{bar: true}}
 
       assert eval(~H"<.changed {%{a: assigns[:b], b: assigns[:a]}} bar={@bar} />") ==
-               [["%{bar: true}"]]
+               [["nil"]]
 
       assigns = %{a: 1, b: 2, bar: 3, __changed__: %{a: true, b: true, bar: true}}
 
       assert eval(~H"<.changed {%{a: assigns[:b], b: assigns[:a]}} bar={@bar} />") ==
-               [["%{a: true, b: true, bar: true}"]]
+               [["nil"]]
     end
 
     defp wrapper(assigns) do