Conversation
|
Demo: Mix.install([
{:phoenix_playground, "~> 0.1.7"},
{:phoenix_live_view, github: "phoenixframework/phoenix_live_view", branch: "main", override: true}
# {:phoenix_live_view, path: "~/oss/phoenix_live_view", override: true}
])
defmodule DemoLive do
use Phoenix.LiveView
def render(assigns) do
~H"""
<button phx-click="randomize">randomize</button>
<button phx-click="change_0">change first</button>
<ul>
<li :for={%{id: id, name: name} <- @items} :key={id}>
Count: <span>{@count}</span>,
item: {name}
</li>
</ul>
"""
end
def mount(_params, _session, socket) do
socket = socket |> assign(:count, 0) |> assign(:items, random_items())
{:ok, assign(socket, :count, 0)}
end
def handle_event("randomize", _params, socket) do
{:noreply, socket |> assign(:items, random_items()) |> update(:count, &(&1 + 1))}
end
def handle_event("change_0", _params, socket) do
{:noreply, socket |> assign(:items, [%{id: 20, name: "#{System.unique_integer()}"} | Enum.slice(socket.assigns.items, 1..11)])}
end
def random_items() do
1..11
|> Enum.take_random(10)
|> Enum.map(&%{id: &1, name: "New#{&1 + 1}"})
end
end
PhoenixPlayground.start(live: DemoLive) |
|
See also #3837. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces keyed for comprehensions that transform a comprehension’s inner content into a LiveComponent for improved change tracking and diff optimization. Key changes include the addition of new tests for keyed comprehensions, updates to the tag engine and engine modules to support the new :key attribute and change-tracking logic, and documentation updates in both the component API and guides.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/phoenix_live_view/diff_test.exs | Adds tests verifying keyed comprehensions’ diff updates. |
| lib/phoenix_live_view/tag_engine.ex | Updates special attributes handling to include :key support. |
| lib/phoenix_live_view/keyed_comprehension.ex | Introduces the new module that powers keyed comprehensions. |
| lib/phoenix_live_view/engine.ex | Adjusts rendering and change tracking logic to support :key. |
| lib/phoenix_component.ex | Updates documentation to describe keyed for comprehensions. |
| guides/server/assigns-eex.md | Revises guide text to showcase usage of keyed comprehensions. |
Comments suppressed due to low confidence (2)
lib/phoenix_live_view/engine.ex:1063
- [nitpick] Add an inline comment clarifying the handling of variables marked as :change_track in the analyze function to improve readability.
%{^name => :change_track} ->
lib/phoenix_live_view/tag_engine.ex:989
- [nitpick] Confirm that adding the :key attribute to the list of special attributes does not introduce inconsistencies with the validation logic elsewhere; consider adding tests if not already covered.
Enum.reduce([for: ":for", if: ":if", key: ":key"], {false, tag_meta, attrs}, fn
e262436 to
f6ccf4c
Compare
|
TODO: more tests for special cases |
lib/phoenix_live_view/tag_engine.ex
Outdated
| {pin_expr, {false, vars}} | ||
|
|
||
| # skip the right hand side in something like <<foo::binary>> | ||
| {:"::", meta, [left, right]}, {true, vars} -> |
There was a problem hiding this comment.
I believe we no longer need this now as analyze skips this already. It also means we solve the issue with setting the accumlator to false below.
I guess another option to solve above is to deal with ^ in the engine, basically we pattern match on ^var's AST and we handle the variable directly. Unsure which one is better.
There was a problem hiding this comment.
we still need it, as we build the assigns for the live component; I think I fixed the processing though and also added a test.
A keyed for comprehension works by transforming the comprehension's
content into a LiveComponent, which will perform change-tracking and
optimizes the payload over the wire:
```heex
<ul>
<li :for={%{id: @id, name: @name} <- @Items} :key={@id}>
Count: <span>{@count}</span>,
item: {@name}
</li>
</ul>
```
To support change tracking, keyed comprehensions require you to define
the left-hand side of the `:for` comprehension, as well as the `:key`,
using assign-syntax: `{@item <- @Items}` instead of `{item <- @Items}`.
Because they use live components under the hood, keyed comprehensions
come with the limitation of only being supported on HTML tags, so while
you can use a regular `:for` comprehension on `<.function_components>`,
and `<:slots>`, this is not supported keyed comprehensions and will raise
an exception at compile time.
* with inner_block * without inner_block * update docs * format * include module and line in key * store change tracked vars in vars
Co-authored-by: José Valim <jose.valim@dashbit.co>
600132d to
fda93ce
Compare
| vars_changed = Map.take(assigns.__changed__, assigns.keys) | ||
| rendered = assigns.render.(vars_changed) | ||
| # the engine ensures that this is valid | ||
| %{rendered | root: true} |
There was a problem hiding this comment.
It would be nice if we could pass this an option when we build the rendered construct. For example, we could perhaps pass root: true to handle_end and have it stored in the meta or similar?
| EEx.compile_string(source, options) | ||
| end | ||
|
|
||
| defmacrop keyed_comprehension(id, vars_changed) do |
There was a problem hiding this comment.
I am honestly not a big fan of this test, it is a unit test and it can fail if we change implementation details and everything continues working. Let's discard this and add some proper tests instead (I assume in html_engine_test.exs) that asserts it returns a component tree and then we render them.
There was a problem hiding this comment.
Actually, me too. I wanted to just test the variable extraction, but to do that it should probably just be a public function (not documented) and I call it directly. For the bigger picture we already have a test checking the diff.
| ) | ||
|
|
||
| {ast, vars} | ||
| end |
There was a problem hiding this comment.
This is definitely way more complex than it needs to be. Doing AST recursion would do the job but I think we should solve it instead in engine.ex. The engine already skips the right-hand side of ::, so no variable there would be tracked, and we can add a clause that checks explicitly for ^, like this:
def analyze({:^, _, [{var, _, context}]}, ...) do
endthat does not include them on change tracking. This way this code block can be "dumb" and annotate all vars, trusting the engine will handle it accordingly (which it already mostly does).
There was a problem hiding this comment.
that would work for the pin operator, but we still need to handle binaries, since we pass them as state to the live component, otherwise we get
error: undefined variable "binary"
│
17 │ <li :for={%{id: id, name: name, bin: <<thebin::binary>>} <- @items} :key={id}>
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
c4d7b89 to
2dd8e2c
Compare
josevalim
left a comment
There was a problem hiding this comment.
LGTM! I want to improve the Macro.traverse bits but it can be done after this is merged :)
A keyed for comprehension works by transforming the comprehension's content into a LiveComponent, which will perform change-tracking and optimizes the payload over the wire:
Because they use live components under the hood, keyed comprehensions come with the limitation of only being supported on HTML tags, so while you can use a regular
:forcomprehension on<.function_components>, and<:slots>, this is not supported keyed comprehensions and will raise an exception at compile time.