From cd28f6c358239e6abb81598786f65100b05b6929 Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Mon, 4 Nov 2024 22:36:42 +1030 Subject: [PATCH] Refactor compiler and template to use requested details --- docs/CHANGELOG.md | 11 +++ lib/view_component/base.rb | 25 +++--- lib/view_component/compiler.rb | 86 +++++++++---------- lib/view_component/errors.rb | 16 ++++ lib/view_component/request_details.rb | 30 +++++++ lib/view_component/template.rb | 71 +++++++-------- lib/view_component/test_helpers.rb | 20 +++-- ...bo_stream_format_component.html+custom.erb | 1 + .../turbo_stream_format_component.html.erb | 1 + .../turbo_stream_format_component.rb | 4 + test/sandbox/test/rendering_test.rb | 34 +++++++- 11 files changed, 194 insertions(+), 105 deletions(-) create mode 100644 lib/view_component/request_details.rb create mode 100644 test/sandbox/app/components/turbo_stream_format_component.html+custom.erb create mode 100644 test/sandbox/app/components/turbo_stream_format_component.html.erb create mode 100644 test/sandbox/app/components/turbo_stream_format_component.rb diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 9287c2522..df46266a8 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -28,6 +28,17 @@ nav_order: 5 *Stephen Nelson* +* BREAKING: Use ActionView's `lookup_context` for picking templates instead of the request format. + + 3.15 added support for using templates that match the request format, i.e. if `/resource.csv` is requested then + ViewComponents would pick `_component.csv.erb` over `_component.html.erb`. + + With this release, the request format is no longer considered and instead ViewComponent will use the Rails logic + for picking the most appropriate template type, i.e. the csv template will be used if it matches the `Accept` header + or because the controller uses a `respond_to` block to pick the response format. + + *Stephen Nelson* + * Ensure HTML output safety wrapper is used for all inline templates. *Joel Hawksley* diff --git a/lib/view_component/base.rb b/lib/view_component/base.rb index 69d8815d6..5099901a9 100644 --- a/lib/view_component/base.rb +++ b/lib/view_component/base.rb @@ -9,6 +9,7 @@ require "view_component/errors" require "view_component/inline_template" require "view_component/preview" +require "view_component/request_details" require "view_component/slotable" require "view_component/slotable_default" require "view_component/template" @@ -63,6 +64,8 @@ def set_original_view_context(view_context) self.__vc_original_view_context = view_context end + using RequestDetails + # Entrypoint for rendering components. # # - `view_context`: ActionView context from calling view @@ -90,13 +93,12 @@ def render_in(view_context, &block) # For i18n @virtual_path ||= virtual_path - # For template variants (+phone, +desktop, etc.) - @__vc_variant ||= @lookup_context.variants.first + # Describes the inferred request constraints (locales, formats, variants) + @__vc_requested_details ||= @lookup_context.vc_requested_details # For caching, such as #cache_if @current_template = nil unless defined?(@current_template) old_current_template = @current_template - @current_template = self if block && defined?(@__vc_content_set_by_with_content) raise DuplicateContentError.new(self.class.name) @@ -108,7 +110,7 @@ def render_in(view_context, &block) before_render if render? - rendered_template = render_template_for(@__vc_variant, __vc_request&.format&.to_sym).to_s + rendered_template = render_template_for(@__vc_requested_details).to_s # Avoid allocating new string when output_preamble and output_postamble are blank if output_preamble.blank? && output_postamble.blank? @@ -156,7 +158,7 @@ def render_parent_to_string target_render = self.class.instance_variable_get(:@__vc_ancestor_calls)[@__vc_parent_render_level] @__vc_parent_render_level += 1 - target_render.bind_call(self, @__vc_variant) + target_render.bind_call(self, @__vc_requested_details) ensure @__vc_parent_render_level -= 1 end @@ -267,11 +269,10 @@ def view_cache_dependencies [] end - # For caching, such as #cache_if - # - # @private + # Rails expects us to define `format` on all renderables, + # but we do not know the `format` of a ViewComponent until runtime. def format - @__vc_variant if defined?(@__vc_variant) + nil end # The current request. Use sparingly as doing so introduces coupling that @@ -328,7 +329,7 @@ def content_evaluated? end def maybe_escape_html(text) - return text if __vc_request && !__vc_request.format.html? + return text if @current_template && !@current_template.html? return text if text.blank? if text.html_safe? @@ -517,12 +518,12 @@ def inherited(child) # meaning it will not be called for any children and thus not compile their templates. if !child.instance_methods(false).include?(:render_template_for) && !child.compiled? child.class_eval <<~RUBY, __FILE__, __LINE__ + 1 - def render_template_for(variant = nil, format = nil) + def render_template_for(requested_details) # Force compilation here so the compiler always redefines render_template_for. # This is mostly a safeguard to prevent infinite recursion. self.class.compile(raise_errors: true, force: true) # .compile replaces this method; call the new one - render_template_for(variant, format) + render_template_for(requested_details) end RUBY end diff --git a/lib/view_component/compiler.rb b/lib/view_component/compiler.rb index 8e80ae026..9bad61349 100644 --- a/lib/view_component/compiler.rb +++ b/lib/view_component/compiler.rb @@ -55,6 +55,23 @@ def compile(raise_errors: false, force: false) end end + # @return all matching compiled templates, in priority order based on the requested details from LookupContext + # + # @param [ActionView::TemplateDetails::Requested] requested_details i.e. locales, formats, variants + def find_templates_for(requested_details) + filtered_templates = @templates.select do |template| + template.details.matches?(requested_details) + end + + if filtered_templates.count > 1 + filtered_templates.sort_by! do |template| + template.details.sort_key_for(requested_details) + end + end + + filtered_templates + end + private attr_reader :templates @@ -64,40 +81,25 @@ def define_render_template_for template.compile_to_component end - method_body = - if @templates.one? - @templates.first.safe_method_name_call - elsif (template = @templates.find(&:inline?)) - template.safe_method_name_call - else - branches = [] - - @templates.each do |template| - conditional = - if template.inline_call? - "variant&.to_sym == #{template.variant.inspect}" - else - [ - template.default_format? ? "(format == #{ViewComponent::Base::VC_INTERNAL_DEFAULT_FORMAT.inspect} || format.nil?)" : "format == #{template.format.inspect}", - template.variant.nil? ? "variant.nil?" : "variant&.to_sym == #{template.variant.inspect}" - ].join(" && ") - end - - branches << [conditional, template.safe_method_name_call] - end + @component.silence_redefinition_of_method(:render_template_for) - out = branches.each_with_object(+"") do |(conditional, branch_body), memo| - memo << "#{(!memo.present?) ? "if" : "elsif"} #{conditional}\n #{branch_body}\n" + if @templates.one? + template = @templates.first + safe_call = template.safe_method_name_call + @component.define_method(:render_template_for) do |_| + @current_template = template + instance_exec(&safe_call) + end + else + compiler = self + @component.define_method(:render_template_for) do |details| + if (@current_template = compiler.find_templates_for(details).first) + instance_exec(&@current_template.safe_method_name_call) + else + raise MissingTemplateError.new(self.class.name, details) end - out << "else\n #{templates.find { _1.variant.nil? && _1.default_format? }.safe_method_name_call}\nend" end - - @component.silence_redefinition_of_method(:render_template_for) - @component.class_eval <<-RUBY, __FILE__, __LINE__ + 1 - def render_template_for(variant = nil, format = nil) - #{method_body} end - RUBY end def template_errors @@ -168,15 +170,18 @@ def template_errors def gather_templates @templates ||= - begin + if @component.inline_template.present? + [Template::Inline.new( + component: @component, + inline_template: @component.inline_template + )] + else path_parser = ActionView::Resolver::PathParser.new templates = @component.sidecar_files( ActionView::Template.template_handler_extensions ).map do |path| details = path_parser.parse(path).details - out = Template::File.new(component: @component, path: path, details: details) - - out + Template::File.new(component: @component, path: path, details: details) end component_instance_methods_on_self = @component.instance_methods(false) @@ -186,17 +191,10 @@ def gather_templates ).flat_map { |ancestor| ancestor.instance_methods(false).grep(/^call(_|$)/) } .uniq .each do |method_name| - templates << Template::InlineCall.new( - component: @component, - method_name: method_name, - defined_on_self: component_instance_methods_on_self.include?(method_name) - ) - end - - if @component.inline_template.present? - templates << Template::Inline.new( + templates << Template::InlineCall.new( component: @component, - inline_template: @component.inline_template + method_name: method_name, + defined_on_self: component_instance_methods_on_self.include?(method_name) ) end diff --git a/lib/view_component/errors.rb b/lib/view_component/errors.rb index e8cb8b10e..22f445569 100644 --- a/lib/view_component/errors.rb +++ b/lib/view_component/errors.rb @@ -38,6 +38,22 @@ def initialize(example) end end + class MissingTemplateError < StandardError + MESSAGE = + "No templates for COMPONENT match the request DETAIL.\n\n" \ + "To fix this issue, provide a suitable template." + + def initialize(component, request_detail) + detail = { + locale: request_detail.locale, + formats: request_detail.formats, + variants: request_detail.variants, + handlers: request_detail.handlers + } + super(MESSAGE.gsub("COMPONENT", component).gsub("DETAIL", detail.inspect)) + end + end + class DuplicateContentError < StandardError MESSAGE = "It looks like a block was provided after calling `with_content` on COMPONENT, " \ diff --git a/lib/view_component/request_details.rb b/lib/view_component/request_details.rb new file mode 100644 index 000000000..c502ce1eb --- /dev/null +++ b/lib/view_component/request_details.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module ViewComponent + # LookupContext computes and encapsulates @details for each request + # so that it doesn't need to be recomputed on each partial render. + # This data is wrapped in ActionView::TemplateDetails::Requested and + # used by instances of ActionView::Resolver to choose which template + # best matches the request. + # + # ActionView considers this logic internal to template/partial resolution. + # We're exposing it to the compiler via `refine` so that ViewComponent + # can match Rails' template picking logic. + module RequestDetails + refine ActionView::LookupContext do + # Return an abstraction for matching and sorting available templates + # based on the current lookup context details. + # + # @return ActionView::TemplateDetails::Requested + # @see ActionView::LookupContext#detail_args_for + # @see ActionView::FileSystemResolver#_find_all + def vc_requested_details(user_details = {}) + # The hash `user_details` would normally be the standard arguments that + # `render` accepts, but there's currently no mechanism for users to + # provide these when calling render on a ViewComponent. + details, cached = detail_args_for(user_details) + cached || ActionView::TemplateDetails::Requested.new(**details) + end + end + end +end diff --git a/lib/view_component/template.rb b/lib/view_component/template.rb index 09d7bdc5f..969a2e8ee 100644 --- a/lib/view_component/template.rb +++ b/lib/view_component/template.rb @@ -7,30 +7,14 @@ class Template attr_reader :details - delegate :format, :variant, to: :details - - def initialize( - component:, - details:, - lineno: nil, - path: nil, - method_name: nil - ) + delegate :virtual_path, to: :@component + delegate :format, :variant, to: :@details + + def initialize(component:, details:, lineno: nil, path: nil) @component = component @details = details @lineno = lineno @path = path - @method_name = method_name - - @call_method_name = - if @method_name - @method_name - else - out = +"call" - out << "_#{normalized_variant_name}" if variant.present? - out << "_#{format}" if format.present? && format != ViewComponent::Base::VC_INTERNAL_DEFAULT_FORMAT - out - end end class File < Template @@ -79,12 +63,9 @@ def initialize(component:, method_name:, defined_on_self:) variant = method_name.to_s.include?("call_") ? method_name.to_s.sub("call_", "").to_sym : nil details = ActionView::TemplateDetails.new(nil, nil, nil, variant) - super( - component: component, - details: details, - method_name: method_name - ) + super(component: component, details: details) + @call_method_name = method_name @defined_on_self = defined_on_self end @@ -96,19 +77,29 @@ def compile_to_component @component.define_method(safe_method_name, @component.instance_method(@call_method_name)) end + def safe_method_name_call + m = safe_method_name + proc do + maybe_escape_html(send(m)) do + Kernel.warn("WARNING: The #{self.class} component rendered HTML-unsafe output. " \ + "The output will be automatically escaped, but you may want to investigate.") + end + end + end + def defined_on_self? @defined_on_self end end def compile_to_component - @component.silence_redefinition_of_method(@call_method_name) + @component.silence_redefinition_of_method(call_method_name) # rubocop:disable Style/EvalWithLocation - @component.class_eval <<-RUBY, @path, @lineno - def #{@call_method_name} - #{compiled_source} - end + @component.class_eval <<~RUBY, @path, @lineno + def #{call_method_name} + #{compiled_source} + end RUBY # rubocop:enable Style/EvalWithLocation @@ -116,11 +107,8 @@ def #{@call_method_name} end def safe_method_name_call - return safe_method_name unless inline_call? - - "maybe_escape_html(#{safe_method_name}) " \ - "{ Kernel.warn('WARNING: The #{@component} component rendered HTML-unsafe output. " \ - "The output will be automatically escaped, but you may want to investigate.') } " + m = safe_method_name + proc { send(m) } end def requires_compiled_superclass? @@ -131,16 +119,19 @@ def inline_call? type == :inline_call end - def inline? - type == :inline - end - def default_format? format.nil? || format == ViewComponent::Base::VC_INTERNAL_DEFAULT_FORMAT end + alias_method :html?, :default_format? + + def call_method_name + @call_method_name ||= + ["call", (normalized_variant_name if variant.present?), (format unless default_format?)] + .compact.join("_").to_sym + end def safe_method_name - "_#{@call_method_name}_#{@component.name.underscore.gsub("/", "__")}" + "_#{call_method_name}_#{@component.name.underscore.gsub("/", "__")}" end def normalized_variant_name diff --git a/lib/view_component/test_helpers.rb b/lib/view_component/test_helpers.rb index 58e4e2a4c..6b6ace7b2 100644 --- a/lib/view_component/test_helpers.rb +++ b/lib/view_component/test_helpers.rb @@ -127,11 +127,11 @@ def render_in_view_context(*args, &block) # end # ``` # - # @param variant [Symbol] The variant to be set for the provided block. - def with_variant(variant) + # @param variants [Symbol[]] The variants to be set for the provided block. + def with_variant(*variants) old_variants = vc_test_controller.view_context.lookup_context.variants - vc_test_controller.view_context.lookup_context.variants = variant + vc_test_controller.view_context.lookup_context.variants += variants yield ensure vc_test_controller.view_context.lookup_context.variants = old_variants @@ -164,9 +164,14 @@ def with_controller_class(klass) # end # ``` # - # @param format [Symbol] The format to be set for the provided block. - def with_format(format) - with_request_url("/", format: format) { yield } + # @param formats [Symbol[]] The format(s) to be set for the provided block. + def with_format(*formats) + old_formats = vc_test_controller.view_context.lookup_context.formats + + vc_test_controller.view_context.lookup_context.formats = formats + yield + ensure + vc_test_controller.view_context.lookup_context.formats = old_formats end # Set the URL of the current request (such as when using request-dependent path helpers): @@ -196,7 +201,7 @@ def with_format(format) # @param full_path [String] The path to set for the current request. # @param host [String] The host to set for the current request. # @param method [String] The request method to set for the current request. - def with_request_url(full_path, host: nil, method: nil, format: ViewComponent::Base::VC_INTERNAL_DEFAULT_FORMAT) + def with_request_url(full_path, host: nil, method: nil) old_request_host = vc_test_request.host old_request_method = vc_test_request.request_method old_request_path_info = vc_test_request.path_info @@ -216,7 +221,6 @@ def with_request_url(full_path, host: nil, method: nil, format: ViewComponent::B vc_test_request.set_header("action_dispatch.request.query_parameters", Rack::Utils.parse_nested_query(query).with_indifferent_access) vc_test_request.set_header(Rack::QUERY_STRING, query) - vc_test_request.format = format yield ensure vc_test_request.host = old_request_host diff --git a/test/sandbox/app/components/turbo_stream_format_component.html+custom.erb b/test/sandbox/app/components/turbo_stream_format_component.html+custom.erb new file mode 100644 index 000000000..2a019d410 --- /dev/null +++ b/test/sandbox/app/components/turbo_stream_format_component.html+custom.erb @@ -0,0 +1 @@ +Hi turbo stream custom! diff --git a/test/sandbox/app/components/turbo_stream_format_component.html.erb b/test/sandbox/app/components/turbo_stream_format_component.html.erb new file mode 100644 index 000000000..f6e0d0b4f --- /dev/null +++ b/test/sandbox/app/components/turbo_stream_format_component.html.erb @@ -0,0 +1 @@ +Hi turbo stream! diff --git a/test/sandbox/app/components/turbo_stream_format_component.rb b/test/sandbox/app/components/turbo_stream_format_component.rb new file mode 100644 index 000000000..ea4a3d0fc --- /dev/null +++ b/test/sandbox/app/components/turbo_stream_format_component.rb @@ -0,0 +1,4 @@ +# frozen_string_literal: true + +class TurboStreamFormatComponent < ViewComponent::Base +end diff --git a/test/sandbox/test/rendering_test.rb b/test/sandbox/test/rendering_test.rb index 0434c3eef..bdd120f0c 100644 --- a/test/sandbox/test/rendering_test.rb +++ b/test/sandbox/test/rendering_test.rb @@ -15,7 +15,7 @@ def test_render_inline_allocations ViewComponent::CompileCache.cache.delete(MyComponent) MyComponent.ensure_compiled - assert_allocations("3.4.0" => 109, "3.3.6" => 115, "3.3.0" => 124, "3.2.6" => 114) do + assert_allocations("3.5.0" => 104, "3.4.1" => 107, "3.3.6" => 107, "3.2.6" => 105) do render_inline(MyComponent.new) end @@ -190,6 +190,14 @@ def test_renders_component_with_variant end end + def test_renders_component_with_multiple_variants + with_variant :app, :phone do + render_inline(VariantsComponent.new) + + assert_text("Phone") + end + end + def test_renders_component_with_variant_containing_a_dash with_variant :"mini-watch" do render_inline(VariantsComponent.new) @@ -1198,6 +1206,20 @@ def test_with_format end end + def test_with_format_missing + with_format(:xml) do + exception = + assert_raises ViewComponent::MissingTemplateError do + render_inline(MultipleFormatsComponent.new) + end + + assert_includes( + exception.message, + "No templates for MultipleFormatsComponent match the request" + ) + end + end + def test_localised_component render_inline(LocalisedComponent.new) @@ -1209,4 +1231,14 @@ def test_request_param assert_text("foo") end + + def test_turbo_stream_format_custom_variant + with_format(:turbo_stream, :html) do + with_variant(:custom) do + render_inline(TurboStreamFormatComponent.new) + + assert_text("Hi turbo stream custom!") + end + end + end end