phoenixframework / phoenix_live_view

Rich, real-time user experiences with server-rendered HTML
https://hex.pm/packages/phoenix_live_view
MIT License
6.21k stars 930 forks source link

Live.Component diff results in client event even when there is no change #2855

Closed m0rt3nlund closed 1 year ago

m0rt3nlund commented 1 year ago

Environment

Actual behavior

Using a Live.Component and issuing send_update to it, then responding with no change in the update/2 callback, returning {:ok, socket} and Liveview will still send a diff to the client: %{c: %{}} even though this is noop

I think the problem is located here: https://github.com/phoenixframework/phoenix_live_view/blob/v0.20.1/lib/phoenix_live_view/diff.ex#L214C56-L214C56

This will return a map where the :c key is set. Since there is no actual change this should not have been set for functionality further down the diff track to be able to return noop

        {diff, cdiffs} = extract_events({diff, cdiffs})

        # Will return a updated map in any case, diff or not
        {Map.put(diff, @components, cdiffs), components, extra}

And this this is expecting an empty map, to say "noop", but due to the lines above it receives %{c: %{}} https://github.com/phoenixframework/phoenix_live_view/blob/v0.20.1/lib/phoenix_live_view/channel.ex#L901C41-L901C41

Expected behavior

Expects no diff to be sent to the client when there is no changes in the assigns of the Live.Component

Possible solution could be something like:

      extract_events({diff, cdiffs})
      |> case do
        {diff, cdiffs} when cdiffs == %{} ->
          {diff, components, extra}

        # Only set `:c` in the diff map if there is actual diff
        {diff, cdiffs} ->
          {Map.put(diff, @components, cdiffs), components, extra}
      end
josevalim commented 1 year ago

Feel free to send a PR but there are several caveats to applying this optimization:

  1. we must not apply it for client events, as they need a response as an ack
  2. we must not apply it if there are events in the response

Given it only applies in every limited cases and the payload is small, we won't chase it, but I will be glad to review if you want to work on it. :) Thanks!