Bug : Liveview dynamics with nil values when using nested loops

[imartinat]

Environment

• Elixir version (elixir -v): Elixir 1.8.1 (compiled with Erlang/OTP 21)
• Phoenix version (mix deps): phoenix 1.4.11
• Phoenix LiveView version (mix deps): phoenix_live_view 0.4.1
• NodeJS version (node -v): v8.10.0
• NPM version (npm -v): 5.6.0
• Operating system: MacOS 10.13.16

Actual behavior

If we have

<%= for row <- @rows do %>
      <%= for col <- @cols do %>
           <%= Map.get(row,col) %>

If rows gets a new value and cols doesn't get a new value Liveview returns this diff
dynamics: [[nil], [nil]]

I first explained the problem on https://elixirforum.com/t/problem-with-liveview-sending-diff-with-null-values/27507/5

When I use this template https://gist.github.com/imartinat/da1707a3c7e4681e6e1d8c31179b6ce4

Liveview doesn't work and sends a diff with null values. The first render works fine but when I click on “Replace users”, the diff sent by Liveview contains:

4: {response: {diff: {0: {d: [[null], [null]]}}}, status: "ok"}
response: {diff: {0: {d: [[null], [null]]}}}
diff: {0: {d: [[null], [null]]}}
0: {d: [[null], [null]]}
d: [[null], [null]]
0: [null]
1: [null]
status: "ok"

Expected behavior

Liveview works fine and updates the list of users if I replace:

<%= for col <- @cols do %>
    <td><%= Map.get(row,col) %></td>
<% end %>

BY

 <td><%= Map.get(row,:id) %></td>
 <td><%= Map.get(row,:user_name) %></td>

It was working fine on a previous version of Liveview.

[imartinat]

If I add in the handle_event a new value for cols, Liveview works fine.

def handle_event("replace_users", _ , socket) do
  rows = [%{id: 2, user_name: "user2" },%{id: 3, user_name: "user3" }]
  cols = [:user_name,:user_name] #previous_value : cols = [:id,:user_name] 
  {:noreply, assign(socket, rows: rows, cols: cols)}
end

<%= for row <- @rows do %>
      <%= for col <- @cols do %>
           <%= Map.get(row,col) %>

It seems that when cols doesn't change, Liveview doesn't compute the diff with the correct value for cols.

If cols doesn't get a new value, here is what I get in the Phoenix.Liveview.Rendered struct :

%Phoenix.LiveView.Rendered{
dynamic: [
%Phoenix.LiveView.Comprehension{
dynamics: [[nil], [nil]],
fingerprint: 104996377389804317111680919555175865526,
static: ["\n \n\n ", "\n\n\n \n "]
}
],
fingerprint: 210190403758897323675912346290647143943,
static: ........
}

[imartinat]

Here is the value for assigns

%{
  cols: [:id, :user_name],
  rows: [%{id: 2, user_name: "user2"}, %{id: 3, user_name: "user3"}],
  socket: #Phoenix.LiveView.Socket<
    assigns: %{
      cols: [:id, :user_name],
      rows: [%{id: 2, user_name: "user2"}, %{id: 3, ...}]
    },
    changed: %{rows: true},

It seems that since in the inner loop, cols doesn't change, Liveview doesn't evaluate the inner loop.

<%= for row <- @rows do %>
      <%= for col <- @cols do %>
           <%= Map.get(row,col) %>

My temporary fix is to have the function changed_assign? in Liveview Engine to always return true. I know it's not optimal since Liveview won't track anymore the changes and will re-render everything.

[imartinat]

If I add this test in engine_test.exs

 template =
        "<%= for x <- @foo do %>X: <%= for y <- @bar do %>Y: <%= x %><%= y %><% end %><% end %>"

 assert [
              %{
                dynamics: [
                  [%{dynamics: [["1", nil]], static: ["Y: ", "", ""]}]
                ],
                static: ["X: ", ""]
              }
            ] = changed(template, %{foo: [1], bar: [1]}, %{foo: true})

If foo changes to [1], Liveview should evaluate Y: <%= x %> to Y: 1 (even if bar doesn't change) so dynamics should be [["1", nil]].

But this test fails :

test change tracking does not render dynamic for nested optimized comprehensions with variables (Phoenix.LiveView.EngineTest)
     test/phoenix_live_view/engine_test.exs:269
     match (=) failed
     code:  assert [%{dynamics: [[%{dynamics: [["1", nil]], static: ["Y: ", "", ""]}]], static: ["X: ", ""]}] = changed(template, %{foo: [1], bar: [1]}, %{foo: true})
     right: [
              %Phoenix.LiveView.Comprehension{
                dynamics: [[nil]],
                fingerprint: 293818862268262170973681223291782393056,
                static: ["X: ", ""]
              }
            ]
     stacktrace: 
         test/phoenix_live_view/engine_test.exs:293: (test)

[jvantuyl]

Similar problem. Have a list of "items". Each contains a "metadata" map. Outer loop always renders fine. Inner loop renders fine for first render. Mutations cause the inner loop to always return null.

[imartinat]

Similar problem. Have a list of "items". Each contains a "metadata" map. Outer loop always renders fine. Inner loop renders fine for first render. Mutations cause the inner loop to always return null.

If you look at https://elixirforum.com/t/problem-with-liveview-sending-diff-with-null-values/27507/5, I gave 2 ways to make it work.

[feliperenan]

I just faced this issue as well. It seems that this issue started from this commit: 57f2ef7

For the time being, we can set our mix to get Phoenix Live View from this specific commit:

{:phoenix_live_view, github: "phoenixframework/phoenix_live_view", ref: "ba6d17bbaaccf6981ed859697329d7cfe2484f71"},

[feliperenan]

I've been trying to find out the solution but I'm kinda lost in how to debug the AST generated by the old code vs the one generated by the new code, I can see they have differences but I cannot point out where and why 🙈

This is the test I believe reproduces this issue. At least it pass when I run it in ba6d17b.

    test "nested for comprehensions" do
      template = "<%= for foo <- @foo do %><%= for bar <- foo.bar do %><%= foo.x %><%= bar.y %><% end %><% end %>"

      assert [
        %{
          dynamics: [
            [%{dynamics: [["1", "1"]], static: ["", "", ""]}]
          ],
          static: ["", ""]
        }
      ] = changed(template, %{foo: [%{x: 1, bar: [%{y: 1}]}]}, %{foo: true})
    end

Hope it helps find the issue cc @josevalim

[josevalim]

Thanks @feliperenan for pinging me! The sample to reproduce the issue helped a lot! A fix was pushed!