Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change behavior of render "foo" to render a string instead of a Rails template #149

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bradgessler
Copy link
Contributor

From Issue #137, this PR makes it such that:

render "foo"

renders a String instead of the rails partial foo. Rendering a partial will be changed to:

render partial: "foo"

This will be a breaking change for Phlex 1.x and ultimately require a path to Phlex 2.x. As such, I'm going to have an Overrides::Version1 and Overrides::Verrsion2 module that get included depending on the setting specified. This will let people upgraded in 1.x to 2.x. When 2.x ships I'll remove 1.x and any updates will break.

@bradgessler
Copy link
Contributor Author

Turns out render "foo" and render partial: "foo" behavior differently in Rails 🤦.

Given this Erb template:

Hi

<%= render "foo" do %>
The middle
<% end %>

<%= render partial: "foo" do %>
The middle
<% end %>

The first one will work, but the latter will throw this error:

'nil' is not an ActiveModel-compatible object. It must implement :to_partial_path.

@bradgessler bradgessler reopened this Feb 8, 2024
@bradgessler
Copy link
Contributor Author

bradgessler commented Feb 8, 2024

New approach, which I think could eliminate a lot of the overrides.

render(rails("fizz") { "buzz" })

The rails method returns this object:

# frozen_string_literal: true

# @api private
module Phlex::Rails
  class Rendition
    attr_reader :args, :kwargs, :block

    def initialize(*args, **kwargs, &block)
      @args = args
      @kwargs = kwargs
      @block = block
    end
  end
end

Which is intercepted by the render override:

when Phlex::Rails::Rendition
  captured_block = -> { capture(&renderable.block) } if renderable.block
  @_context.target << @_view_context.render(*renderable.args, **renderable.kwargs, &captured_block)

I would propose moving this into an adapter, which would be included via the kitchen sink, but at least for all-Phlex apps the Rails renderer would never be invoked.

@bradgessler
Copy link
Contributor Author

I opened a bug at rails/rails#51015. If Rails decides this is a bug and fixes it, we should be set. If they don't then we'll have to figure out how to render from Phlex in a manner that doesn't obstruct the "fidelity" of rendering in Rails.

@joeldrapper
Copy link
Collaborator

Had a conversation about the Rails issue here. https://discord.com/channels/849034466856665118/974005005768069211/1218911072522600481

@joeldrapper joeldrapper added this to the 2.0 milestone Mar 18, 2024
@joeldrapper
Copy link
Collaborator

@bradgessler any thoughts on what we can do about this for 2.0? 🤔

@bradgessler
Copy link
Contributor Author

Yeah we could either take the approach above where we wrap Rails rendering calls in a class via render(rails("fizz") { "buzz" }). This would provide the most "faithful" rendition of Rails content since we'd let people continue calling into this undefined/broken behavior.

Another option is Phlex assumes render "string" does NOT call out to Rails and we instead require a keyword like partial: to call out into Rails renditions.

Another option is we could have a rails.render or rails_render method that makes it clear the call is into Rails and not into Phlex.

I think we should rule out patching Rails since (1) they don't seem interested and (2) getting patches through for stuff like this has historically been a 🥙 for me.

@joeldrapper
Copy link
Collaborator

Agh, this is such a tricky issue. None of those options feel quite right, but we have to come up with something. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants