Extract Select rendering methods into SelectRenderer module and simplify Action View base helper logic #48574

pinzonjulian · 2023-06-25T12:36:13Z

Motivation / Background

This Pull Request has been made to help make the Action View Helper Tags a little bit better. This is purely a refactoring PR and does not introduce any changes to the behaviour of tags.

The reason I'm doing this is because I'm diving deep into these tags (1, 2) in order to find some sort of abstraction (this is one possible version) that might help make it easier to style form elements (input, select, label and text-area)

In doing so, I've discovered that the Tags::Base class is in need of some maintenance to make it easier to understand where the boundaries are drawn between different elements.

Detail

This Pull Request:

Simplifies some of the logic in the Tags::Base class
Creates a new SelectRenderer module that contains three methods only used by <select> tags.
Includes the FormOptionsHelper module only in <select> classes.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

3 methods that originally lived in the Tags::Base class are only used by tags that output a <select> tag. These were extracted into a SelectRenderer module which is now only added to the tags that need it.

The FormOptionsHelper is only needed in classes where a <select> tag is rendered. It was previously included in the Tags::Base class which unnecessarily included it every child Tag class.

rafaelfranca · 2023-06-26T04:12:33Z

actionview/lib/action_view/helpers/tags/collection_select.rb

@@ -1,9 +1,14 @@
 # frozen_string_literal: true

+require "action_view/helpers/tags/select_renderer"


If it is autoloaded we should not require

rafaelfranca · 2023-06-26T04:14:27Z

actionview/lib/action_view/helpers/tags.rb

@@ -30,6 +30,7 @@ module Tags # :nodoc:
        autoload :RangeField
        autoload :SearchField
        autoload :Select
+        autoload :SelectRenderer


We can move this out of the eager_autoload block. It doesn't need to be eager loaded since it is included in classes that are eager loaded

nikz · 2023-06-26T23:31:45Z

Hi @pinzonjulian @rafaelfranca - looks like this change might break the country_select gem (If I'm reading everything correctly, this module would need to include the SelectRenderer)

I think we might want to consider a way to release this change without breaking as it's a fairly popular/common gem.

pinzonjulian · 2023-06-27T01:38:38Z

Hey @nikz, thanks for letting us know!

I learnt a while ago that in Rails, all of the modules and classes tagged with the comment # :nodoc: are private to the Rails codebase. Similar to a private method, they're not expected to be used by anything else but the framework itself. This means that any libraries using these low-level, private APIs are taking on a bit of risk when using them.

Since all of the movements in this PR are within the realm of "private" modules and classes, we should probably take this conversation to the country_select gem and figure out a way for it to keep working with this change.

Feel free to open an issue in the country_select gem (or even better, a PR with a failing test) so we can figure out how to best help the gem adopt this change

pinzonjulian · 2023-06-27T01:42:21Z

By the way, I think the module should be included here @nikz
https://github.com/countries/country_select/blob/master/lib/country_select/country_select_helper.rb#L33

nikz · 2023-06-27T01:46:10Z

Hi @pinzonjulian,

Yep, aware of the # :nodoc: convention, thank you.

It's worth noting that this is country_select - originally extracted from Rails' codebase and with over 15 million downloads.

It seems like we should consider landing a change to country_select before landing the change in Rails. As far as I can tell the benefit of this change is just a refactoring, is that correct? So it would seem that we should perhaps prioritise not breaking a common gem over a style change?

pinzonjulian · 2023-06-27T02:58:37Z

It seems like we should consider landing a change to country_select before landing the change in Rails.

How do you envision this change looking like? Since this change is not on a public release of Rails it seems like maybe country_select could dynamically include the module for versions 7.1 an up?

As far as I can tell the benefit of this change is just a refactoring, is that correct?

Not really. The goal of cleaning up Action View Tags through small PRs like this one is to make Tags simpler to reason about. This will help uncover new abstractions to provide library authors with better tools to create custom Form elements without having to rely on private classes and modules –which is exemplified by the problem that country_select is experiencing with this change.

So it would seem that we should perhaps prioritise not breaking a common gem over a style change?

I'd delegate to @rafaelfranca here because I'm not part of the Rails core or issue team. I think that, at least in general, the refactoring of private APIs shouldn't be prevented by external libraries but it may be a case by case thing. Again, I'll let @rafaelfranca or someone from the Rails team to weigh in.

rafaelfranca · 2023-06-27T17:52:39Z

This change will indeed break gems that rely on the previous implementation. This was a risk those gems took on using private API of Rails. It is up to country_select maintainers to fix this issue.

Rails should not slow down the pace of changes in its private implementation, stylistics or not, because libraries are relying on APIs that aren't published.

The best way forward is to open an issue in that repository, and probably include the module conditionally when the version of Rails defines that module.

Currently , the FormBuilder class is defined in the FormHelper class and then reopened and complemented in the FormOptionsHelper class. This PR has been created because understanding all the moving pieces for FormBuilders, FormHelpers and FormOptionHelpers becomes increasingly hard when classes are created and reopened in multiple places. Consolidating it into a single file makes it easier to understand the breadth and purpose of the class. This PR is part of a larger effort to cleanup and modernise this section of the codebase to hopefully bring in newer abstractions to it. See rails#48574 for a previous PR with the same purpose.

mainly adjust to changes in AV::Helpers::Tags::Base see rails/rails#48574

rails-bot bot added the actionview label Jun 25, 2023

pinzonjulian force-pushed the simplify_action_view_bases_helper_logic branch from 957d8a5 to 39dd5eb Compare June 25, 2023 12:41

pinzonjulian changed the title ~~Simplify action view bases helper logic~~ Extract Select rendering methods into SelectRenderer module and simplify Action View base helper logic Jun 25, 2023

pinzonjulian added 2 commits June 25, 2023 22:44

Extract Select rendering methods into SelectRenderer module

9e85c2c

3 methods that originally lived in the Tags::Base class are only used by tags that output a <select> tag. These were extracted into a SelectRenderer module which is now only added to the tags that need it.

Simplify Tags::Base private method logic

f19f426

pinzonjulian force-pushed the simplify_action_view_bases_helper_logic branch from 39dd5eb to f19f426 Compare June 25, 2023 12:44

Move Form Options helper into select rendering classes

77f3fc1

The FormOptionsHelper is only needed in classes where a <select> tag is rendered. It was previously included in the Tags::Base class which unnecessarily included it every child Tag class.

pinzonjulian force-pushed the simplify_action_view_bases_helper_logic branch from a043aee to 77f3fc1 Compare June 25, 2023 23:39

rafaelfranca reviewed Jun 26, 2023

View reviewed changes

rafaelfranca merged commit 51ec30a into rails:main Jun 26, 2023
9 checks passed

pinzonjulian deleted the simplify_action_view_bases_helper_logic branch June 27, 2023 01:39

pinzonjulian mentioned this pull request Nov 1, 2023

Consolidate FormBuilder into a single file #49883

Open

2 tasks

glaszig added a commit to glaszig/recurring_select that referenced this pull request Nov 8, 2023

support rails 7.1

9434a9e

mainly adjust to changes in AV::Helpers::Tags::Base see rails/rails#48574

glaszig added a commit to glaszig/recurring_select that referenced this pull request Nov 8, 2023

support rails 7.1

7ed5cb2

mainly adjust to changes in AV::Helpers::Tags::Base see rails/rails#48574

jystewart pushed a commit to jystewart/recurring_select that referenced this pull request Nov 25, 2023

support rails 7.1

99e218a

mainly adjust to changes in AV::Helpers::Tags::Base see rails/rails#48574

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract Select rendering methods into SelectRenderer module and simplify Action View base helper logic #48574

Extract Select rendering methods into SelectRenderer module and simplify Action View base helper logic #48574

pinzonjulian commented Jun 25, 2023 •

edited

rafaelfranca Jun 26, 2023

rafaelfranca Jun 26, 2023

nikz commented Jun 26, 2023

pinzonjulian commented Jun 27, 2023

pinzonjulian commented Jun 27, 2023

nikz commented Jun 27, 2023

pinzonjulian commented Jun 27, 2023 •

edited

rafaelfranca commented Jun 27, 2023

		@@ -1,9 +1,14 @@
		# frozen_string_literal: true

		require "action_view/helpers/tags/select_renderer"

Extract Select rendering methods into SelectRenderer module and simplify Action View base helper logic #48574

Extract Select rendering methods into SelectRenderer module and simplify Action View base helper logic #48574

Conversation

pinzonjulian commented Jun 25, 2023 • edited

Motivation / Background

Detail

Checklist

rafaelfranca Jun 26, 2023

Choose a reason for hiding this comment

rafaelfranca Jun 26, 2023

Choose a reason for hiding this comment

nikz commented Jun 26, 2023

pinzonjulian commented Jun 27, 2023

pinzonjulian commented Jun 27, 2023

nikz commented Jun 27, 2023

pinzonjulian commented Jun 27, 2023 • edited

rafaelfranca commented Jun 27, 2023

pinzonjulian commented Jun 25, 2023 •

edited

pinzonjulian commented Jun 27, 2023 •

edited