Proposal: Make frame render functions return the frame.

[christhekeele]

This accomplishes three things in the name of pipe-ability:

• Makes Kino.Frame.render consistent with Kino.render
  This also lets you do things like this:

  import Kino.Shorts
  - frame = frame() |> Kino.render
  - frame |> Kino.Frame.render(markdown("> ***Sumbit an event***"))
  + frame = frame() |> Kino.render() |> Kino.Frame.render(markdown("> ***Sumbit an event***"))

• Makes Kino.Frame.append(frame, term) more chainable
  For example:

  import Kino.Shorts
  frame() |> Kino.render
  |> Kino.Frame.render(markdown("> ***Sumbit an event***"))
  |> Kino.Frame.append(markdown("- Or any term really"))
  |> Kino.Frame.append(markdown("- I'm a form, not a cop"))

• Makes Kino.Frame.clear(frame) chainable
  For example:

  # for some existing frame and form...
  Kino.listen(form, fn event ->
    frame
    |> Kino.Frame.clear()
    |> Kino.Frame.render(markdown("`\#{inspect event}`"))
  end)

This implementation wraps the GenServer.calls with with, which technically may return something other than :ok, but effectively makes the same assumptions that the previous typespec did that it will always be :ok. We could just match on :ok (like in clear's cast) or even ignore the result of the call, but this is the most backwards-compatible with any existing code today that tries to handle non-:ok GenServer.call results (I would be suprised if any such code was out there, though).

[josevalim]

We can't do these changes, because it means any code today that does Kino.Frame.render(...) (or any of the functions above) at the end, will now embed the result into the frame and as an output. I am almost sure we chose :ok on purpose, exactly to avoid this.

[christhekeele]

We can't do these changes, because it means any code today that does Kino.Frame.render(...) (or any of the functions above) at the end, will now embed the result into the frame and as an output.

I see. The problem would be if you create a frame in one cell, then update/render it in another. Without a Kino.nothing ending the first cell it'll be implicitly rendered.

I tend to co-locate my frames with the code necessary to render into them in the same cell, which (without a fluent API) necessarily means the frame can't be the implicit output. (I have to assign it, render with :ok, then reference it in ex. a listener with its own return value.) So the frame never made it to the end of my cells, and this edge case did not occur to me. (At least, until I started this PR, there's a reason why I proposed the pipeable nothing at the same time: to frame() |> render() |> output_nothing()).

But yeah, dividing the frame creation and rendering between cells could lead to double-embedding issues, makes sense.

I am almost sure we chose :ok on purpose, exactly to avoid this.

Would it make sense to have them return a more intention-revealing Kino.nothing/0 to safe-guard against double rendering instead? When reading the code, relying on the GenServer APIs to return :ok did not signal this intent to me, neither did the docs/typespecs. Whereas something like this with a note in the @doc might be more instructive:

[christhekeele]

We can't do these changes, because it means any code today that does Kino.Frame.render(...) (or any of the functions above) at the end, will now embed the result into the frame and as an output.

I'm not convinced that this DX is a bad thing: if you are creating a frame in one cell and using it in another, you are probably experimenting with frames (and might want to see the intermediary output). As opposed to building a re-renderable UI in a single cell (and necessarily co-locating listeners after frame initialization, where you need a handle to it). A simple Kino.nothing() would fix if this is undesired. But it would be a possibly breaking change and need to be released accordingly.

[christhekeele]

Side note: if we do decide on this, releasing as a potentially breaking change, we can probably drop the with on the GenServer.call returns and simply match on :ok. Since we are more loudly modifying the return value of this function and asking people to check their call sites for assumptions, we can also narrow the expected return of the call from term() to :ok knowing folk won't be trying to handle term() themselves.

[jonatanklosko]

Hey @christhekeele :)

• Makes Kino.Frame.append(frame, term) more chainable
  For example:

  import Kino.Shorts
  frame() |> Kino.render
  |> Kino.Frame.render(markdown("> ***Sumbit an event***"))
  |> Kino.Frame.append(markdown("- Or any term really"))
  |> Kino.Frame.append(markdown("- I'm a form, not a cop"))

To render a group of outputs, a preferred way would be using grid, so that it's a single render. append is more useful if you want to add more outputs asynchronously.

• Makes Kino.Frame.clear(frame) chainable
  For example:

  # for some existing frame and form...
  Kino.listen(form, fn event ->
    frame
    |> Kino.Frame.clear()
    |> Kino.Frame.render(markdown("`\#{inspect event}`"))
  end)

render replaces the frame content, so clear is not necessary. I would say that chaining frame operations actually points towards an anti-pattern :)

When reading the code, relying on the GenServer APIs to return :ok did not signal this intent to me

GenServer.call/2 either returns the value from :reply (and we always reply with :ok), or it exits. So it wouldn't return anything other than :ok.

---

We changed Kino.render/1 from returning :ok to term, but it was very early in Kino v0.4.1.

Also, there is a bit of a difference in that, when you call Kino.render/1, by definition it's not the last operation in the cell (otherwise it's no point in calling it). Contrarily, Kino.Frame.render/2 could be the last operation.

Either way, I don't think it's worth the breaking change at this point.

[jonatanklosko]

On a sidenote, instead of

frame = frame() |> Kino.render()

...

Kino.nothing()

I would consider this more idiomatic:

frame = frame()

...

frame

It's less code and arguably makes it easier to change if you want to render a composition of elements, such as grid([form, frame]).

But that's definitely subjective. I can see people preferring the first snippet, if they are less familiar with the notion of "cell outputs effectively being Kino.render-ed". I think @josevalim prefers explicit renders in many cases :)

[christhekeele]

Hey @jonatanklosko! Thanks for your explanations, they are instructive.

To render a group of outputs, a preferred way would be using grid, so that it's a single render. append is more useful if you want to add more outputs asynchronously.

In my case, I've been finding myself adding more outputs conditionally, so they don't reliably fit into a specific size grid, and frame chainability with append and tap/then would be nice.

It's less code and arguably makes it easier to change if you want to render a composition of elements, such as grid([form, frame]).

But that's definitely subjective. I can see people preferring the first snippet, if they are less familiar with the notion of "cell outputs effectively being Kino.render-ed". I think @josevalim prefers explicit renders in many cases :)

Some of this is definitely me learning new idioms for UI, evolving from simple inputs to packing forms and frames. In my case, what I'm experimenting with is keeping cohesive bits of related complex UI concerns grouped in single code cells, with my final output being Kino.Markdown usage instructions for apps. So in each code cell I'm doing a lot of manual rendering of UI before presenting instructions, and evolving a personal style for this project of preferring explicit renders for consistency.

[christhekeele]

PR Summary

• Frame.render -> frame

  I don't think it's worth the breaking change at this point.

• Frame.clear -> frame

  render replaces the frame content, so clear is not necessary

• Frame.append -> frame

  I still maintain that this would be nice

  In my case, I've been finding myself adding more outputs conditionally, so they don't reliably fit into a specific size grid, and frame chainability with append and tap/then would be nice.

  But there's still the hard counter of

  I don't think it's worth the breaking change at this point.

There is one discussion point in my mind yet unresolved, which is

Would it make sense to have them return a more intention-revealing Kino.nothing/0 to safe-guard against double rendering instead? When reading the code, relying on the GenServer APIs to return :ok did not signal this intent to me, neither did the docs/typespecs.

GenServer.call/2 either returns the value from :reply (and we always reply with :ok), or it exits. So it wouldn't return anything other than :ok.

I maintain that return :ok is fine, but still kind of a leaking implementation detail from the frame server. Especially if the main argument against this frame-chaining proposal is that we don't want things to accidentally render, and push folk towards other approaches to using frames, I think returning Kino.nothing/0 doubles down on those no-rendering guarantees and is a more self-explanatory of the intended usage pattern, providing a starting point for documentation to latch onto and explain.

However, that is separate from what this PR was exploring—so I will close, and allow someone to take the point above into an issue/discussion if there is interest beyond my own. Thanks for the discussion everyone!

[jonatanklosko]

In my case, I've been finding myself adding more outputs conditionally, so they don't reliably fit into a specific size grid

I think size shouldn't be a problem, by default it's a single column, so my point is that this:

Kino.Frame.append(frame, Kino.Markdown.new("foo"))
Kino.Frame.append(frame, Kino.Markdown.new("bar"))
Kino.Frame.append(frame, Kino.Markdown.new("baz"))

looks the same as this:

Kino.Frame.render(
  frame,
  Kino.Layout.grid([
    Kino.Markdown.new("foo"),
    Kino.Markdown.new("bar"),
    Kino.Markdown.new("baz")
  ])
)

You can build the list conditionally, but if you prefer append, I think that's fine.

I maintain that return :ok is fine, but still kind of a leaking implementation detail from the frame server.

It's not leaking implementation, we return :ok on purpose, because that's very common for stateful actions. And from the user perspective, it doesn't matter if the return comes from GenServer.call, or if the function explicitly has :ok at the end. The spec says it returns :ok, and the implementation guarantees that this is the case :)