render "foo" tries to render the Rails "foo" view instead of a string

[bradgessler]

In an effort for Phlex to be more compatible with Rails, a call to render "foo" will try to find the view foo instead of rendering the string "foo".

This causes a few problems:

1. When a third party component is brought into Rails, it may fail if there's render String calls.
2. When writing Phlex code in Rails, the content that's about to be rendered has to be checked if its a String and then rendered via plain.

Here's the call site of the code in question: https://github.com//phlex-ruby/phlex-rails/blob/76efa50ebfaa70e828e4c396722b7c1da47cf0d8/lib/phlex/rails/sgml/overrides.rb#L26

A few proposals:

Require the partial: parameter for render

Behavior for rendering templates in Rails 2.2 and before looked like:

render partial: "products/form"

Source: https://guides.rubyonrails.org/layouts_and_rendering.html#rendering-an-action-s-template-from-another-controller

This would break compatibility with the current version of Phlex and likely require a deprecation message in the 1.x series and necessitate a 2.0 release.

The render("foo") call would then behave with the base Phlex behavior.

Require an explicit call to Rails view_context

There's a few ways to do this, but either way its requiring the developer to make an explicit call to the renderer like this:

view_context.render "foo" # Renders a Rails "foo" view"

If the helpers proxy is going to be kept around, Rails renderer could be moved behind there:

helpers.render "foo" # Renders a Rails "foo" view"

Or rails:

rails.render "foo" # Renders a Rails "foo" view"

This would free up the base render method to behavior with Phlex's renderer:

render "foo" # Renders via Phlex

This would also break compatibility with 1.x and require a 2.x release.

---

I'd like to keep this issue open to collect a few more ideas for navigating this bug fix while minimizing impact to 1.x. Once something is decided I can open a PR.

[joeldrapper]

Thanks for opening this @bradgessler. I do think the ability to render strings is important enough for this to be worth figuring out. Of course you can always render strings with plain, but being able to render an object without checking if it's a string or a block or a renderable is really powerful.

[bradgessler]

being able to render an object without checking if it's a string or a block or a renderable is really powerful

I'd say it's a necessity based on the work arounds I had to do to get this working in Rails. In my case I had a layout with def title in it that would return a string or Phlex markup.

def title = "My Page"

Running the title string through plain works, but then that blows up when I need to do something to my title like make it a link:

def title
  a(href: "#title") { "My Page" }
end

[bradgessler]

Some (sloppy) notes from Discord relevant to the implementation of the PR for this issue:

Add String to this line:

phlex-rails/lib/phlex/rails/sgml/overrides.rb

Lines 15 to 31 in e886ad8

	def render(*args, **kwargs, &block)
	renderable = args[0]
	case renderable
	when Phlex::SGML, Proc
	return super
	when Class
	return super if renderable < Phlex::SGML
	when Enumerable
	return super unless renderable.is_a?(ActiveRecord::Relation)
	else
	captured_block = -> { capture(&block) } if block
	@_context.target << @_view_context.render(*args, **kwargs, &captured_block)
	end
	nil
	end

phlex-rails/lib/phlex/rails/sgml/overrides.rb

Line 19 in e886ad8

when Phlex::SGML, Proc

:partial, :template, etc at

phlex-rails/lib/phlex/rails/sgml/overrides.rb

Line 27 in e886ad8

@_context.target << @_view_context.render(*args, **kwargs, &captured_block)

We should also remove this check here IMO

phlex-rails/lib/phlex/rails/sgml/overrides.rb

Line 24 in e886ad8

return super unless renderable.is_a?(ActiveRecord::Relation)

Remove this check

https://github.com/phlex-ruby/phlex-rails/blob/e886ad82b0003576abcf3e0b17c90371b43fc5d3/lib/phlex/rails/sgml/overrides.rb

I’d try something like this

def render(*args, **kwargs, &block)
  renderable = args[0]

  case renderable
  when Phlex::SGML, Proc, String, Enumerable, Class
    return super
  else
    if block
         @_context.target << @_view_context.render(*args, **kwargs) { capture(&block) }
    else
      @_context.target << @_view_context.render(*args, **kwargs)
    end
  end

  nil
end

I think you can do something like this for 1.x

def render(*args, **kwargs, &block)
  renderable = args[0]


  case renderable
  when Phlex::SGML, Proc
    return super
  when Class
    return super if renderable < Phlex::SGML
  when Enumerable
    if renderable.is_a?)(ActiveRecord::Relation)
      warn "Some warning here"
    else
      return super
    end
  else
    if renderable.is_a?(String)
      warn "Some other warning"
    end
    
    captured_block = -> { capture(&block) } if block
    @_context.target << @_view_context.render(*args, **kwargs, &captured_block)
  end

  nil
end

[bradgessler]

I decided I'm going to have a 1.x and 2.x module in the same branch, with a setting to switch these out. This means in 1.x people can make the upgrade and verify it's working.

When we're happy with that a 2.x will go out, which removes the 1.x code and would break everything in a project that hasn't upgraded.