Skip to content

Add test showing that form helpers aren't used by View Components #1260

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

Closed
wants to merge 1 commit into from

Conversation

sampart
Copy link
Contributor

@sampart sampart commented Feb 1, 2022

Summary

This PR adds a test showing that form helpers aren't used when a View Component calls form_tag. CI will fail because of the failing test, but I'm envisaging that having the test will provide a helpful starting point for addressing this issue.

Other Information

I'd expect form helpers to be picked up automatically. form_tag_html is an override of this method which is called by form_tag directly here and indirectly here.

@sampart sampart requested a review from a team as a code owner February 1, 2022 12:01
@camertron
Copy link
Contributor

Thanks for the failing test @sampart. Is it related to another issue, and if so, would you mind linking to it in the description?

@mhw
Copy link
Contributor

mhw commented Feb 4, 2022

I think part of the issue here is that helpers defined in app/helpers are handled differently from those defined in action_view/helpers. The ActionView helpers are included in to ActionView::Base and so are available directly within View Components (and their templates) as they are inherited from ActionView::Base. The ones from app/helpers are included in to an anonymous module that is then included in to an anonymous subclass of ActionView::Base, and on which the templates from app/views are compiled. This is why View Component provides the helpers method - it gives access to the original view context (the anonymous subclass of ActionView::Base) that the helpers from app/helpers are defined on.

Given this, I don't think redefining form_tag_html from app/helpers in a View Component context will work.

Problems with form_tag may be due to it using capture and issues that currently exist with not having a global output buffer across the original view context and the View Components. There's some work to address this here

@camertron
Copy link
Contributor

@mhw makes sense, thanks for that additional context.

We chatted about this internally and decided to include all helpers in components starting with view_component v3.0.

@BlakeWilliams
Copy link
Contributor

We chatted about this internally and decided to include all helpers in components starting with view_component v3.0.

I'm generally a fan of that idea, but I think we have to be careful about how we implement it. If we delegate method_missing to helpers I think that would be fine behavior wise, but if we include helpers in the base object we'll run into issues where instance variables no longer share state across components renders or partial renders.

e.g. Given a contrived helper like:

module MyHelper
  def expensive_op
    @_expensive_op ||= ExpensiveCall.new.perform
  end
end

Being called from several different partial/locations:

<%= render :my_partial # calls expensive_op, total calls: 1 %> 
<%= render :different_partial # calls expensive_op, total calls: 1 %> 

<%= render NotIncludedComponent.new # calls helpers.expensive_op, total calls: 1 %>

<%= render IncludedComponent.new # calls expensive_op via `include MyHelper`, total calls: 2 %>
<%= render IncludedComponent.new # calls expensive_op via `include MyHelper`, total calls: 3 %>
<%= render IncludedComponent.new # calls expensive_op via `include MyHelper`, total calls: 4 %>

This and other interop problems are pretty tricky to get right. I've been thinking a lot lately about somehow compiling components down into partials so we get interop and other benefits for free, but that's another problem that's hard to solve and comes with its own tradeoffs. 🙂

@joelhawksley
Copy link
Member

Closing as stale ❤️

@joelhawksley joelhawksley deleted the form-helper-test branch March 11, 2025 17:38
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.

5 participants