willcosgrove commented 1 year ago

Describe the bug

Phlex views that use a builder-style API are rendering in unbuffered mode, as though they were called from an ERB template.

To Reproduce

Steps to reproduce the behavior:

Given a pair of Phlex components defined like this:

class List < Phlex::HTML
  def template
    ul { yield }
  end

  def item(...)
    render Item.new(...)
  end
end

class Item < Phlex::HTML
  def template
    li { yield }
  end
end

And a preview defined like this:

class ListPreview < Lookbook::Preview
  def default
    render List.new do |list|
      list.item { "Hello" }
      list.item { "World" }
    end
  end
end

You will get an output that looks like this:

<ul>
  <li>World</li>
</ul>

This is because when Rails' render method is used (which gets turned into render_in), Phlex will turn the component into an "unbuffered" representation. This means that now each method that normally would have added output to the internal buffer and returned nil, will now not append anything to the internal buffer, and instead return the string it would have appended.

So in our above example the line by line evaluation looks like this:

render List.new do |list|
  list.item { "Hello" } # => "<li>Hello</li>
  list.item { "World" } # => "<li>World</li>
end

When the List component finishes it's block, it sees that it's own internal buffer has not been modified and assumes that the return value of the block is supposed to be appended to the buffer, which is why we get the final list item only.

This is how Phlex must work when it's being rendered from inside another template

<%= render List.new do |list| %>
  <li>First</li>
  <%= list.item { "Hello" } %>
  <%= list.item { "World" } %>
  <li>Last</li>
<% end %>

In this context, the buffer is being managed by ERB.

Expected behavior

My expectation was that, because I am rendering my Phlex component in a (for lack of a better term) "ruby context"— not inside of a template, that the component would behave the same way it does when I render it within other Phlex views.

It's the view of the Phlex author that render_in should remain adapted to the Rails view rendering context, and that to render Phlex components in a "ruby context" the call method should be used.

Version numbers

Please complete the following information:

Lookbook: 2.0.0.beta.9
Phlex: 1.6.1
Rails: 7.0.4.3
Ruby: 3.2.1

allmarkedup commented 1 year ago

Hey @willcosgrove, thanks for this. I have a few questions if you don't mind - I'm not very familiar with the Phlex codebase so I'm just trying to get my head around the issue. I'm also far from an expert when it comes to things like output buffers so apologies if I'm a little slow on the pick up!

So, as a bit of background info, the render method in the Lookbook::Preview class is actually a bit of misdirection. The components do not actually get rendered there - the render method generates a hash of data, which includes the instantiated component that was passed to the render method initially.

The actual component rendering is done via a controller action, which renders this template: https://github.com/ViewComponent/lookbook/blob/5ad0219a882dc42f5ff86a2963daccb1d440c41e/app/views/lookbook/previews/preview.html.erb#L1-L5

The @render_args variable here contains the hash of data generated by the Lookbook::Preview#render method.

So, the instantiated component here is actually being rendered in an ERB template... but I guess the issue is that the block itself is not rendered as ERB so the list.item calls are not outputting to it's internal buffer. Have I got that correct?

I assume therefore (and I realise that it is unlikely that anyone would do this, but just for my own understanding) that if one instantiates a component in a controller action, passes that to the view and renders it there, that will also not work? For example:

class FooController < ApplicationController
  def show
    @list = List.new do |list|
      list.item { "Hello" }
      list.item { "World" }
    end
  end
end

<!-- foo/show.html.erb -->
<%= render @list %>

and that to render Phlex components in a "ruby context" the call method should be used

Do you think you could provide me with a few pointers as to what this might look like? I'm going to try to dig into the Phlex codebase now to try and figure out what to best do about this but any suggestions would be much appreciated.

allmarkedup commented 1 year ago

@willcosgrove okay so after a bit of trial and error this change to the preview template seems to do the trick:

<% if @render_args[:component] %>
  <% if @render_args[:component].is_a?(Phlex::HTML) %>
    <%== @render_args[:component].call(view_context: self, &@render_args[:block]) %>
  <% else %>
    <%= render(@render_args[:component], @render_args[:args], &@render_args[:block]) %>
  <% end %>
<% else %>
  <%= render(@render_args[:template], **@render_args[:locals], &@render_args[:block]) %>
<% end %>

Does calling the instantiated Phlex component like that look correct to you? Anything I'm missing there that you can spot?

willcosgrove commented 1 year ago

So, the instantiated component here is actually being rendered in an ERB template... but I guess the issue is that the block itself is not rendered as ERB so the list.item calls are not outputting to it's internal buffer. Have I got that correct?

Yes, you're exactly right. The block being passed to render is not from an ERB context, but from the method inside the user's ComponentPreview class.

I looked into having Phlex detect this context but detecting it is error prone (it varies by templating language used) and is slow (it requires inspecting the binding of the block which is not a fast process). Because it would be hard to get this right 100% of the time, and because it would slow down the render path, @joeldrapper decided that it should be the responsibility of the code doing the rendering to indicate what type of context you want the component rendered in.

render_in (which is what Rails is calling when you pass it to render in the view) is going to assume that you're inside a view file, and use an Unbuffered component.

call is what Phlex uses internally to render other Phlex views, and is what should be used to render within a "ruby context".

Here's an example of how you might be able to modify the view code you posted above to make it use call instead of render_in for Phlex views:

<% if @render_args[:component] %>
  <% case @render_args[:component] %>
  <% when Phlex::SGML %>
    <%= raw(@render_args[:component].call(view_context: self, &@render_args[:block])) %>
  <% else %>
    <%= render(@render_args[:component], @render_args[:args], &@render_args[:block]) %>
  <% end %>
<% else %> 
  <%= render(@render_args[:template], **@render_args[:locals], &@render_args[:block]) %> 
<% end %>

I don't think @render_args[:args] is needed in the Phlex case, but I'm not 100% sure what those would be, so I could be wrong.

willcosgrove commented 1 year ago

@allmarkedup your code looks perfect. The only thing I might suggest is checking for inheritance of Phlex::SGML instead of Phlex::HTML. SGML is higher up the chain, and also includes SVG views.

We may have an even higher abstract class later if Phlex branches out into other types of views, like JSON or XML, but for now Phlex::SGML is at the top of the hierarchy.

allmarkedup commented 1 year ago

@willcosgrove okay great, many thanks for the explanation. And makes sense about checking for Phlex::SGML, good shout.

Appreciate you taking the time to walk me through this and I'll get a fix up for this now 👍

joeldrapper commented 1 year ago

Thanks for writing this up @willcosgrove. @allmarkedup your fix looks good. Let me know if I can be of any help.

allmarkedup commented 1 year ago

Thanks again for the help, I've just merged the PR so will close this down now 👍

stephannv commented 9 months ago

I'm facing a similar issue with Lookbook + Phlex:

Phlex allows you to write something like this:

render Views::Phlex::ListExample.new do |list|
  list.item { "Hello" }  
  render(Views::Phlex::Item.new) { "Help!"}
  list.item { "World" }
end

But when we write this example on a Lookbook::Preview class, it doesn't work.

The render(Views::Phlex::Item.new) { "Help!" } returns a hash because it is using render from Lookbook::Preview class and the component isn't rendered. To solve that I could write something like this:

render Views::Phlex::ListExample.new do |list|
  list.item { "Hello" }  
  list.render(Views::Phlex::Item.new) { "Help!"}
  list.item { "World" }
end

But it has two problems:

On source tab, will display component usage list.render ..., but it isn't required, so other devs could understand that unnecessary code is required to build the component

I cannot use my component helpers. I added helpers to Lookbook::Preview, eg.:


module DesignSystemHelpers
def my_button(*args, **kwargs, &block)
render DesignSystem::ButtonComponent.new(*args, **kwargs, &block)
end

def my_card(...)
...
end

...
end

on initializers

config.to_prepare do Lookbook::Preview.include DesignSystemHelpers end

So something like this doesn't work on Lookbook:
```ruby
def default
  my_card do |card| # this will be rendered
    card.actions do # this will be rendered
      my_button(...) # this will NOT be rendered
      my_button(...) # this will NOT be rendered
    end
  end
end

This is important because I think Lookbook is a source of how developers should use our components.

And the other problem is that I cannot do that:

render Views::Phlex::ListExample.new do |list|
  h1 { "My list" }
  list.item { "Hello" }  
  list.item { "World" }
end

It will raise undefined method 'h1' for an instance of MyComponentPreview.

lookbook-hq / lookbook

Support for Phlex builder-style components #400

Describe the bug

To Reproduce

Expected behavior

Version numbers

on initializers