From eea9da6639c3fe81f328e7da8904d89df6b2467b Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Fri, 13 Dec 2024 10:34:58 -0800 Subject: [PATCH] Improve template path detection for forms (#3236) --- .changeset/khaki-seals-battle.md | 5 +++ app/lib/primer/forms/acts_as_component.rb | 11 +++++- app/lib/primer/forms/base.rb | 5 --- app/lib/primer/forms/base_component.rb | 8 ++-- app/lib/primer/forms/utils.rb | 17 +++++++- demo/config/application.rb | 1 - test/lib/primer/forms/const_src.rb | 9 +++++ test/lib/primer/forms/utils_test.rb | 48 +++++++++++++++++++++++ 8 files changed, 92 insertions(+), 12 deletions(-) create mode 100644 .changeset/khaki-seals-battle.md create mode 100644 test/lib/primer/forms/const_src.rb create mode 100644 test/lib/primer/forms/utils_test.rb diff --git a/.changeset/khaki-seals-battle.md b/.changeset/khaki-seals-battle.md new file mode 100644 index 0000000000..2ce3bdc870 --- /dev/null +++ b/.changeset/khaki-seals-battle.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': patch +--- + +Improve template path detection for forms diff --git a/app/lib/primer/forms/acts_as_component.rb b/app/lib/primer/forms/acts_as_component.rb index 57fd6bcd96..cd044e1253 100644 --- a/app/lib/primer/forms/acts_as_component.rb +++ b/app/lib/primer/forms/acts_as_component.rb @@ -50,8 +50,6 @@ def self.extended(base) TemplateGlob = Struct.new(:glob_pattern, :method_name, :on_compile_callback) TemplateParams = Struct.new(:source, :identifier, :type, :format, keyword_init: true) - attr_accessor :template_root_path - def renders_templates(glob_pattern, method_name = nil, &block) template_globs << TemplateGlob.new(glob_pattern, method_name, block) end @@ -70,6 +68,15 @@ def compile! private + def template_root_path + return @template_root_path if defined?(@template_root_path) + + form_path = Utils.const_source_location(self.name) + @template_root_path = if form_path + File.join(File.dirname(form_path), self.name.demodulize.underscore) + end + end + def template_globs @template_globs ||= [] end diff --git a/app/lib/primer/forms/base.rb b/app/lib/primer/forms/base.rb index d010c45f20..cbc3fa087b 100644 --- a/app/lib/primer/forms/base.rb +++ b/app/lib/primer/forms/base.rb @@ -29,11 +29,6 @@ def new(builder, **options) end def inherited(base) - form_path = Utils.const_source_location(base.name) - return unless form_path - - base.template_root_path = File.join(File.dirname(form_path), base.name.demodulize.underscore) - base.renders_template "after_content.html.erb" do base.instance_variable_set(:@has_after_content, true) end diff --git a/app/lib/primer/forms/base_component.rb b/app/lib/primer/forms/base_component.rb index 0199afc8b1..4cb2bed8ef 100644 --- a/app/lib/primer/forms/base_component.rb +++ b/app/lib/primer/forms/base_component.rb @@ -7,8 +7,8 @@ class BaseComponent include Primer::ClassNameHelper extend ActsAsComponent - def self.inherited(base) - base_path = Utils.const_source_location(base.name) + def self.compile! + base_path = Utils.const_source_location(self.name) unless base_path warn "Could not identify the template for #{base}" @@ -16,7 +16,9 @@ def self.inherited(base) end dir = File.dirname(base_path) - base.renders_template File.join(dir, "#{base.name.demodulize.underscore}.html.erb"), :render_template + renders_template File.join(dir, "#{self.name.demodulize.underscore}.html.erb"), :render_template + + super end delegate :required?, :disabled?, :hidden?, to: :@input diff --git a/app/lib/primer/forms/utils.rb b/app/lib/primer/forms/utils.rb index 595aab72ff..65ed122af7 100644 --- a/app/lib/primer/forms/utils.rb +++ b/app/lib/primer/forms/utils.rb @@ -14,13 +14,28 @@ module Utils # for the file in the configured autoload paths. Doing so relies on Rails' autoloading # conventions, so it should work ok. Zeitwerk also has this information but lacks a # public API to map constants to source files. + # + # Now that the Ruby bug above has been fixed and released, this method should be used only + # as a fallback for older Rubies. def const_source_location(class_name) return nil unless class_name + if (location = Object.const_source_location(class_name)&.[](0)) + return location + end + # NOTE: underscore respects namespacing, i.e. will convert Foo::Bar to foo/bar. class_path = "#{class_name.underscore}.rb" - ActiveSupport::Dependencies.autoload_paths.each do |autoload_path| + # Prefer Zeitwerk-managed paths, falling back to ActiveSupport::Dependencies if Zeitwerk + # is disabled or not in use (i.e. the case for older Rails versions). + autoload_paths = if Rails.respond_to?(:autoloaders) && Rails.autoloaders.zeitwerk_enabled? + Rails.autoloaders.main.dirs + else + ActiveSupport::Dependencies.autoload_paths + end + + autoload_paths.each do |autoload_path| absolute_path = File.join(autoload_path, class_path) return absolute_path if File.exist?(absolute_path) end diff --git a/demo/config/application.rb b/demo/config/application.rb index 18aa0a92e8..0723867338 100644 --- a/demo/config/application.rb +++ b/demo/config/application.rb @@ -33,7 +33,6 @@ class Application < Rails::Application config.view_component.preview_controller = "PreviewController" config.view_component.preview_paths << Rails.root.join("..", "previews") - config.autoload_paths << Rails.root.join("..", "test", "forms") config.autoload_paths << Rails.root.join("..", "test", "test_helpers", "components") config.action_dispatch.default_headers.clear diff --git a/test/lib/primer/forms/const_src.rb b/test/lib/primer/forms/const_src.rb new file mode 100644 index 0000000000..5b5a201f2a --- /dev/null +++ b/test/lib/primer/forms/const_src.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +# Used in utils_test.rb +module Primer + module Forms + class ConstSrc + end + end +end diff --git a/test/lib/primer/forms/utils_test.rb b/test/lib/primer/forms/utils_test.rb new file mode 100644 index 0000000000..ac05ab9fab --- /dev/null +++ b/test/lib/primer/forms/utils_test.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require "lib/test_helper" + +class Primer::Forms::UtilsTest < Minitest::Test + # This should pass under both Rubies that have the bug fix described in + # https://github.com/ruby/ruby/pull/5646 and Rubies that do not. Ruby versions 2.7 to 3.1 + # (inclusive) are affected. See also https://bugs.ruby-lang.org/issues/18624. + def test_const_source_location + ensure_autoload! + + actual_loc = Primer::Forms::Utils.const_source_location("Primer::Forms::ConstSrc") + expected_loc = File.join(File.dirname(__FILE__), "const_src.rb") + + assert actual_loc == expected_loc + end + + def test_const_source_location_falls_back_to_enumerating_load_path + # Normally this would be dangerous since test/lib isn't Zeitwerk-friendly, but because + # we're doing it after Zeitwerk setup, it will essentially have no effect. + Rails.autoloaders.main.push_dir(Rails.root.join("..", "test", "lib")) + ensure_autoload! + + called = false + + Object.stub(:const_source_location, [false, 0]) do + actual_loc = Primer::Forms::Utils.const_source_location("Primer::Forms::ConstSrc") + expected_loc = File.join(File.dirname(__FILE__), "const_src.rb") + assert actual_loc == expected_loc + called = true + end + + assert called, "Object.const_source_location was not called" + end + + private + + def ensure_autoload! + # Load the constant so const_source_location will return the correct path instead of + # the path to the file that defines the autoload (which is this file, see below). + Primer::Forms.const_get(:ConstSrc) + rescue NameError + # We unfortunately can't add test/lib to the load path because it's not Zeitwerk-friendly, + # so we fake it by setting up the autoload to our test file manually. + Primer::Forms.autoload :ConstSrc, File.join(File.dirname(__FILE__), "const_src") + Primer::Forms.const_get(:ConstSrc) + end +end