From 8f9b9ba92320a1c88d9cb5bbe513754a25d6c35a Mon Sep 17 00:00:00 2001 From: Joel Hawksley Date: Wed, 16 Oct 2024 09:12:50 -0600 Subject: [PATCH 1/8] add .DS_Store to gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 340f093f5..dfe6728fd 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ *.gem *.rbc .ruby-version +.DS_Store /.config /coverage/assets /coverage/index.html From 595aeb2ab36084b319cddb0d9497988d01960581 Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Mon, 4 Nov 2024 15:53:44 +1030 Subject: [PATCH 2/8] Add Template subclasses to improve compiler polymorphism --- lib/view_component/compiler.rb | 11 ++++------- lib/view_component/template.rb | 28 ++++++++++++++++++++++------ 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/lib/view_component/compiler.rb b/lib/view_component/compiler.rb index c4ab88df4..285da8a46 100644 --- a/lib/view_component/compiler.rb +++ b/lib/view_component/compiler.rb @@ -174,16 +174,15 @@ def gather_templates ).map do |path| # Extract format and variant from template filename this_format, variant = - File + ::File .basename(path) # "variants_component.html+mini.watch.erb" .split(".")[1..-2] # ["html+mini", "watch"] .join(".") # "html+mini.watch" .split("+") # ["html", "mini.watch"] .map(&:to_sym) # [:html, :"mini.watch"] - out = Template.new( + out = Template::File.new( component: @component, - type: :file, path: path, lineno: 0, extension: path.split(".").last, @@ -201,9 +200,8 @@ def gather_templates ).flat_map { |ancestor| ancestor.instance_methods(false).grep(/^call(_|$)/) } .uniq .each do |method_name| - templates << Template.new( + templates << Template::InlineCall.new( component: @component, - type: :inline_call, this_format: ViewComponent::Base::VC_INTERNAL_DEFAULT_FORMAT, variant: method_name.to_s.include?("call_") ? method_name.to_s.sub("call_", "").to_sym : nil, method_name: method_name, @@ -212,9 +210,8 @@ def gather_templates end if @component.inline_template.present? - templates << Template.new( + templates << Template::Inline.new( component: @component, - type: :inline, path: @component.inline_template.path, lineno: @component.inline_template.lineno, source: @component.inline_template.source.dup, diff --git a/lib/view_component/template.rb b/lib/view_component/template.rb index f01fd9368..ad490432d 100644 --- a/lib/view_component/template.rb +++ b/lib/view_component/template.rb @@ -5,11 +5,10 @@ class Template DataWithSource = Struct.new(:format, :identifier, :short_identifier, :type, keyword_init: true) DataNoSource = Struct.new(:source, :identifier, :type, keyword_init: true) - attr_reader :variant, :this_format, :type + attr_reader :variant, :this_format def initialize( component:, - type:, this_format: nil, variant: nil, lineno: nil, @@ -20,7 +19,6 @@ def initialize( defined_on_self: true ) @component = component - @type = type @this_format = this_format @variant = variant&.to_sym @lineno = lineno @@ -43,6 +41,24 @@ def initialize( end end + class File < Template + def type + :file + end + end + + class Inline < Template + def type + :inline + end + end + + class InlineCall < Template + def type + :inline_call + end + end + def compile_to_component if !inline_call? @component.silence_redefinition_of_method(@call_method_name) @@ -72,11 +88,11 @@ def requires_compiled_superclass? end def inline_call? - @type == :inline_call + type == :inline_call end def inline? - @type == :inline + type == :inline end def default_format? @@ -104,7 +120,7 @@ def defined_on_self? def source if @source_originally_nil # Load file each time we look up #source in case the file has been modified - File.read(@path) + ::File.read(@path) else @source end From e6bf01c32111fe39eaf72df40c839cde4e07245c Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Mon, 4 Nov 2024 16:04:03 +1030 Subject: [PATCH 3/8] Move template type-specific logic to constructors --- lib/view_component/compiler.rb | 25 ++------------------- lib/view_component/template.rb | 40 ++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 23 deletions(-) diff --git a/lib/view_component/compiler.rb b/lib/view_component/compiler.rb index 285da8a46..4c5939080 100644 --- a/lib/view_component/compiler.rb +++ b/lib/view_component/compiler.rb @@ -172,23 +172,7 @@ def gather_templates templates = @component.sidecar_files( ActionView::Template.template_handler_extensions ).map do |path| - # Extract format and variant from template filename - this_format, variant = - ::File - .basename(path) # "variants_component.html+mini.watch.erb" - .split(".")[1..-2] # ["html+mini", "watch"] - .join(".") # "html+mini.watch" - .split("+") # ["html", "mini.watch"] - .map(&:to_sym) # [:html, :"mini.watch"] - - out = Template::File.new( - component: @component, - path: path, - lineno: 0, - extension: path.split(".").last, - this_format: this_format.to_s.split(".").last&.to_sym, # strip locale from this_format, see #2113 - variant: variant - ) + out = Template::File.new(component: @component, path: path) out end @@ -202,8 +186,6 @@ def gather_templates .each do |method_name| templates << Template::InlineCall.new( component: @component, - this_format: ViewComponent::Base::VC_INTERNAL_DEFAULT_FORMAT, - variant: method_name.to_s.include?("call_") ? method_name.to_s.sub("call_", "").to_sym : nil, method_name: method_name, defined_on_self: component_instance_methods_on_self.include?(method_name) ) @@ -212,10 +194,7 @@ def gather_templates if @component.inline_template.present? templates << Template::Inline.new( component: @component, - path: @component.inline_template.path, - lineno: @component.inline_template.lineno, - source: @component.inline_template.source.dup, - extension: @component.inline_template.language + inline_template: @component.inline_template ) end diff --git a/lib/view_component/template.rb b/lib/view_component/template.rb index ad490432d..c2292e0ce 100644 --- a/lib/view_component/template.rb +++ b/lib/view_component/template.rb @@ -42,18 +42,58 @@ def initialize( end class File < Template + def initialize(component:, path:) + # Extract format and variant from template filename + this_format, variant = + ::File + .basename(path) # "variants_component.html+mini.watch.erb" + .split(".")[1..-2] # ["html+mini", "watch"] + .join(".") # "html+mini.watch" + .split("+") # ["html", "mini.watch"] + .map(&:to_sym) # [:html, :"mini.watch"] + + super( + component: component, + path: path, + lineno: 0, + extension: path.split(".").last, + this_format: this_format.to_s.split(".").last&.to_sym, # strip locale from this_format, see #2113 + variant: variant + ) + end + def type :file end end class Inline < Template + def initialize(component:, inline_template:) + super( + component: component, + path: inline_template.path, + lineno: inline_template.lineno, + source: inline_template.source.dup, + extension: inline_template.language + ) + end + def type :inline end end class InlineCall < Template + def initialize(component:, method_name:, defined_on_self:) + super( + component: component, + this_format: ViewComponent::Base::VC_INTERNAL_DEFAULT_FORMAT, + variant: method_name.to_s.include?("call_") ? method_name.to_s.sub("call_", "").to_sym : nil, + method_name: method_name, + defined_on_self: defined_on_self + ) + end + def type :inline_call end From 1db086b2607840a7c7bf84cf974e70a3f194fb06 Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Mon, 4 Nov 2024 16:18:47 +1030 Subject: [PATCH 4/8] Inline source into templates that require it --- lib/view_component/template.rb | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/lib/view_component/template.rb b/lib/view_component/template.rb index c2292e0ce..eeb8c2038 100644 --- a/lib/view_component/template.rb +++ b/lib/view_component/template.rb @@ -14,7 +14,6 @@ def initialize( lineno: nil, path: nil, extension: nil, - source: nil, method_name: nil, defined_on_self: true ) @@ -24,12 +23,9 @@ def initialize( @lineno = lineno @path = path @extension = extension - @source = source @method_name = method_name @defined_on_self = defined_on_self - @source_originally_nil = @source.nil? - @call_method_name = if @method_name @method_name @@ -65,17 +61,25 @@ def initialize(component:, path:) def type :file end + + # Load file each time we look up #source in case the file has been modified + def source + ::File.read(@path) + end end class Inline < Template + attr_reader :source + def initialize(component:, inline_template:) super( component: component, path: inline_template.path, lineno: inline_template.lineno, - source: inline_template.source.dup, extension: inline_template.language ) + + @source = inline_template.source.dup end def type @@ -157,15 +161,6 @@ def defined_on_self? private - def source - if @source_originally_nil - # Load file each time we look up #source in case the file has been modified - ::File.read(@path) - else - @source - end - end - def compiled_source handler = ActionView::Template.handler_for_extension(@extension) this_source = source From e239c74bbb2823f32c3c6f30a65b34cb4df6b1d8 Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Mon, 4 Nov 2024 16:20:58 +1030 Subject: [PATCH 5/8] Flatten inline_call conditional in compile_to_component --- lib/view_component/template.rb | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/lib/view_component/template.rb b/lib/view_component/template.rb index eeb8c2038..7fe1b0c6b 100644 --- a/lib/view_component/template.rb +++ b/lib/view_component/template.rb @@ -101,20 +101,22 @@ def initialize(component:, method_name:, defined_on_self:) def type :inline_call end + + def compile_to_component + @component.define_method(safe_method_name, @component.instance_method(@call_method_name)) + end end def compile_to_component - if !inline_call? - @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 - RUBY - # rubocop:enable Style/EvalWithLocation + # rubocop:disable Style/EvalWithLocation + @component.class_eval <<-RUBY, @path, @lineno + def #{@call_method_name} + #{compiled_source} end + RUBY + # rubocop:enable Style/EvalWithLocation @component.define_method(safe_method_name, @component.instance_method(@call_method_name)) end From 38582bcc68642bd0469c0fdad6ab4284faa740fa Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Mon, 4 Nov 2024 16:23:09 +1030 Subject: [PATCH 6/8] Remove defined_on_self param from non-inline-call templates --- lib/view_component/template.rb | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/view_component/template.rb b/lib/view_component/template.rb index 7fe1b0c6b..9f25d868f 100644 --- a/lib/view_component/template.rb +++ b/lib/view_component/template.rb @@ -14,8 +14,7 @@ def initialize( lineno: nil, path: nil, extension: nil, - method_name: nil, - defined_on_self: true + method_name: nil ) @component = component @this_format = this_format @@ -24,7 +23,6 @@ def initialize( @path = path @extension = extension @method_name = method_name - @defined_on_self = defined_on_self @call_method_name = if @method_name @@ -94,8 +92,9 @@ def initialize(component:, method_name:, defined_on_self:) this_format: ViewComponent::Base::VC_INTERNAL_DEFAULT_FORMAT, variant: method_name.to_s.include?("call_") ? method_name.to_s.sub("call_", "").to_sym : nil, method_name: method_name, - defined_on_self: defined_on_self ) + + @defined_on_self = defined_on_self end def type @@ -105,6 +104,10 @@ def type def compile_to_component @component.define_method(safe_method_name, @component.instance_method(@call_method_name)) end + + def defined_on_self? + @defined_on_self + end end def compile_to_component @@ -157,10 +160,6 @@ def normalized_variant_name @variant.to_s.gsub("-", "__").gsub(".", "___") end - def defined_on_self? - @defined_on_self - end - private def compiled_source From dbb445f5fb71a626c1b82fd56d6bb66e4edfd929 Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Mon, 4 Nov 2024 17:09:52 +1030 Subject: [PATCH 7/8] Use ActionView logic for parsing template names Removes support for variant names containing `.`. --- docs/CHANGELOG.md | 4 ++++ docs/index.md | 1 + lib/view_component/compiler.rb | 4 +++- lib/view_component/template.rb | 19 +++++-------------- .../variants_component.html+mini.watch.erb | 1 - test/sandbox/test/rendering_test.rb | 8 -------- 6 files changed, 13 insertions(+), 24 deletions(-) delete mode 100644 test/sandbox/app/components/variants_component.html+mini.watch.erb diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 868adf283..9287c2522 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -24,6 +24,10 @@ nav_order: 5 *Joel Hawksley* +* BREAKING: Remove support for variant names containing `.` to be consistent with Rails. + + *Stephen Nelson* + * Ensure HTML output safety wrapper is used for all inline templates. *Joel Hawksley* diff --git a/docs/index.md b/docs/index.md index c810d6471..659efa588 100644 --- a/docs/index.md +++ b/docs/index.md @@ -193,6 +193,7 @@ ViewComponent is built by over a hundred members of the community, including: sammyhenningsson sampart seanpdoyle +sfnelson simonrand skryukov smashwilson diff --git a/lib/view_component/compiler.rb b/lib/view_component/compiler.rb index 4c5939080..8e80ae026 100644 --- a/lib/view_component/compiler.rb +++ b/lib/view_component/compiler.rb @@ -169,10 +169,12 @@ def template_errors def gather_templates @templates ||= begin + path_parser = ActionView::Resolver::PathParser.new templates = @component.sidecar_files( ActionView::Template.template_handler_extensions ).map do |path| - out = Template::File.new(component: @component, path: path) + details = path_parser.parse(path).details + out = Template::File.new(component: @component, path: path, details: details) out end diff --git a/lib/view_component/template.rb b/lib/view_component/template.rb index 9f25d868f..f4f344097 100644 --- a/lib/view_component/template.rb +++ b/lib/view_component/template.rb @@ -36,23 +36,14 @@ def initialize( end class File < Template - def initialize(component:, path:) - # Extract format and variant from template filename - this_format, variant = - ::File - .basename(path) # "variants_component.html+mini.watch.erb" - .split(".")[1..-2] # ["html+mini", "watch"] - .join(".") # "html+mini.watch" - .split("+") # ["html", "mini.watch"] - .map(&:to_sym) # [:html, :"mini.watch"] - + def initialize(component:, path:, details:) super( component: component, path: path, lineno: 0, - extension: path.split(".").last, - this_format: this_format.to_s.split(".").last&.to_sym, # strip locale from this_format, see #2113 - variant: variant + extension: details.handler.to_s, + this_format: details.format, + variant: details.variant ) end @@ -157,7 +148,7 @@ def safe_method_name end def normalized_variant_name - @variant.to_s.gsub("-", "__").gsub(".", "___") + @variant.to_s.gsub("-", "__") end private diff --git a/test/sandbox/app/components/variants_component.html+mini.watch.erb b/test/sandbox/app/components/variants_component.html+mini.watch.erb deleted file mode 100644 index b44d8505e..000000000 --- a/test/sandbox/app/components/variants_component.html+mini.watch.erb +++ /dev/null @@ -1 +0,0 @@ -Mini Watch with dot diff --git a/test/sandbox/test/rendering_test.rb b/test/sandbox/test/rendering_test.rb index d52840f85..10dd2e758 100644 --- a/test/sandbox/test/rendering_test.rb +++ b/test/sandbox/test/rendering_test.rb @@ -198,14 +198,6 @@ def test_renders_component_with_variant_containing_a_dash end end - def test_renders_component_with_variant_containing_a_dot - with_variant :"mini.watch" do - render_inline(VariantsComponent.new) - - assert_text("Mini Watch with dot") - end - end - def test_renders_default_template_when_variant_template_is_not_present with_variant :variant_without_template do render_inline(VariantsComponent.new) From 9e30635948ba930c6f0e17a38e7df6a5ceae9e67 Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Mon, 4 Nov 2024 17:29:43 +1030 Subject: [PATCH 8/8] Delegate template format and variant to TemplateDetails --- lib/view_component/template.rb | 51 ++++++++++++++++------------------ 1 file changed, 24 insertions(+), 27 deletions(-) diff --git a/lib/view_component/template.rb b/lib/view_component/template.rb index f4f344097..09d7bdc5f 100644 --- a/lib/view_component/template.rb +++ b/lib/view_component/template.rb @@ -5,23 +5,21 @@ class Template DataWithSource = Struct.new(:format, :identifier, :short_identifier, :type, keyword_init: true) DataNoSource = Struct.new(:source, :identifier, :type, keyword_init: true) - attr_reader :variant, :this_format + attr_reader :details + + delegate :format, :variant, to: :details def initialize( component:, - this_format: nil, - variant: nil, + details:, lineno: nil, path: nil, - extension: nil, method_name: nil ) @component = component - @this_format = this_format - @variant = variant&.to_sym + @details = details @lineno = lineno @path = path - @extension = extension @method_name = method_name @call_method_name = @@ -29,21 +27,19 @@ def initialize( @method_name else out = +"call" - out << "_#{normalized_variant_name}" if @variant.present? - out << "_#{@this_format}" if @this_format.present? && @this_format != ViewComponent::Base::VC_INTERNAL_DEFAULT_FORMAT + 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 - def initialize(component:, path:, details:) + def initialize(component:, details:, path:) super( component: component, + details: details, path: path, - lineno: 0, - extension: details.handler.to_s, - this_format: details.format, - variant: details.variant + lineno: 0 ) end @@ -61,11 +57,13 @@ class Inline < Template attr_reader :source def initialize(component:, inline_template:) + details = ActionView::TemplateDetails.new(nil, inline_template.language.to_sym, nil, nil) + super( component: component, + details: details, path: inline_template.path, lineno: inline_template.lineno, - extension: inline_template.language ) @source = inline_template.source.dup @@ -78,11 +76,13 @@ def type class InlineCall < Template 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, - this_format: ViewComponent::Base::VC_INTERNAL_DEFAULT_FORMAT, - variant: method_name.to_s.include?("call_") ? method_name.to_s.sub("call_", "").to_sym : nil, - method_name: method_name, + details: details, + method_name: method_name ) @defined_on_self = defined_on_self @@ -136,11 +136,7 @@ def inline? end def default_format? - @this_format == ViewComponent::Base::VC_INTERNAL_DEFAULT_FORMAT - end - - def format - @this_format + format.nil? || format == ViewComponent::Base::VC_INTERNAL_DEFAULT_FORMAT end def safe_method_name @@ -148,22 +144,23 @@ def safe_method_name end def normalized_variant_name - @variant.to_s.gsub("-", "__") + variant.to_s.gsub("-", "__") end private def compiled_source - handler = ActionView::Template.handler_for_extension(@extension) + handler = details.handler_class this_source = source this_source.rstrip! if @component.strip_trailing_whitespace? short_identifier = defined?(Rails.root) ? @path.sub("#{Rails.root}/", "") : @path - type = ActionView::Template::Types[@this_format] + format = self.format || ViewComponent::Base::VC_INTERNAL_DEFAULT_FORMAT + type = ActionView::Template::Types[format] if handler.method(:call).parameters.length > 1 handler.call( - DataWithSource.new(format: @this_format, identifier: @path, short_identifier: short_identifier, type: type), + DataWithSource.new(format: format, identifier: @path, short_identifier: short_identifier, type: type), this_source ) # :nocov: