diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 9cee4c9ec..0e1a10624 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -10,6 +10,14 @@ nav_order: 6 ## main +* Fix line numbers and, under coverage, missing output for ERB templates that Rails doesn't annotate. + + On Rails 8.1+, ViewComponent compensated for the newline Rails adds to compiled ERB output ([rails/rails#53731](https://github.com/rails/rails/pull/53731)) for every ERB template, whether from a file or from `erb_template`. That newline lives inside the `` annotation, which Rails only emits when `annotate_rendered_view_with_filenames` is enabled *and* the template's format is HTML. Compensating unconditionally shifted backtraces for non-HTML templates (`.text.erb`, `.css.erb`) and for every ERB template when annotations are disabled, such as in production, by one line. When coverage was running, the same mismatch made the annotation-stripping workaround remove real template source, so non-HTML templates rendered empty. + + Inline templates now decide the compensation when they compile rather than when the component class is defined, matching file templates. + + *Ryutaro Mizokami* + * Reduce per-render allocations. Inline renders drop 2 to 3 allocations and collection renders drop 4 to 8 depending on Rails/Ruby version by caching the instrumentation-enabled flag at the module level, memoizing the empty-details `Requested` per `LookupContext`, hoisting per-item metadata lookups out of the collection render loop, and dropping a few gratuitous `**` splats on the `Collection` API boundary. *Joel Hawksley* diff --git a/lib/view_component/template.rb b/lib/view_component/template.rb index 062828295..36c4a1dff 100644 --- a/lib/view_component/template.rb +++ b/lib/view_component/template.rb @@ -60,13 +60,7 @@ def lineno # Strip the annotation line to maintain correct line numbers when coverage # is running (avoids segfault from negative lineno) def strip_annotation_line? - erb_newline_compensation_needed? && - coverage_running? && - ActionView::Base.annotate_rendered_view_with_filenames - end - - def erb_newline_compensation_needed? - Rails::VERSION::MAJOR >= 8 && Rails::VERSION::MINOR > 0 && details.handler == :erb + erb_newline_compensation_needed? && coverage_running? end def compiled_source @@ -82,22 +76,11 @@ class Inline < Template def initialize(component:, inline_template:) details = ActionView::TemplateDetails.new(nil, inline_template.language.to_sym, nil, nil) - # Rails 8.1 added a newline to compiled ERB output (rails/rails#53731). - # Subtract 1 to compensate for correct line numbers in stack traces. - # Inline templates start at line 2+ (defined inside a class), so this - # won't result in negative line numbers that cause segfaults with coverage. - lineno = - if Rails::VERSION::MAJOR >= 8 && Rails::VERSION::MINOR > 0 && details.handler == :erb - inline_template.lineno - 1 - else - inline_template.lineno - end - super( component: component, details: details, path: inline_template.path, - lineno: lineno, + lineno: inline_template.lineno, ) @source = inline_template.source.dup @@ -106,6 +89,19 @@ def initialize(component:, inline_template:) def type :inline end + + private + + # Rails 8.1 added a newline to compiled ERB output (rails/rails#53731). + # Subtract 1 to compensate for correct line numbers in stack traces. + # Inline templates start at line 2+ (defined inside a class), so this + # won't result in negative line numbers that cause segfaults with coverage. + # + # Computed at compile time rather than at initialization because the + # annotation setting can change between the two. + def lineno + erb_newline_compensation_needed? ? super - 1 : super + end end class InlineCall < Template @@ -196,6 +192,20 @@ def normalized_variant_name attr_reader :lineno + def erb_newline_compensation_needed? + Rails::VERSION::MAJOR >= 8 && Rails::VERSION::MINOR > 0 && + details.handler == :erb && + annotated? + end + + # The newline compensated for lives inside the `` + # annotation, which Rails only prepends to HTML templates, and only when + # annotations are enabled. Compensating outside those conditions shifts + # backtraces by a line and, under coverage, strips real template source. + def annotated? + ActionView::Base.annotate_rendered_view_with_filenames && html? + end + def compiled_source handler = details.handler_class this_source = source diff --git a/test/sandbox/app/components/raising_formats_component.html.erb b/test/sandbox/app/components/raising_formats_component.html.erb new file mode 100644 index 000000000..4cebe5744 --- /dev/null +++ b/test/sandbox/app/components/raising_formats_component.html.erb @@ -0,0 +1,3 @@ +
+ <%= raise ArgumentError, "oh no" %> +
diff --git a/test/sandbox/app/components/raising_formats_component.rb b/test/sandbox/app/components/raising_formats_component.rb new file mode 100644 index 000000000..7641c9e3c --- /dev/null +++ b/test/sandbox/app/components/raising_formats_component.rb @@ -0,0 +1,4 @@ +# frozen_string_literal: true + +class RaisingFormatsComponent < ViewComponent::Base +end diff --git a/test/sandbox/app/components/raising_formats_component.text.erb b/test/sandbox/app/components/raising_formats_component.text.erb new file mode 100644 index 000000000..16adabac6 --- /dev/null +++ b/test/sandbox/app/components/raising_formats_component.text.erb @@ -0,0 +1,2 @@ +line one +<%= raise ArgumentError, "oh no" %> diff --git a/test/sandbox/test/inline_template_test.rb b/test/sandbox/test/inline_template_test.rb index dcb5741ec..3762f24a2 100644 --- a/test/sandbox/test/inline_template_test.rb +++ b/test/sandbox/test/inline_template_test.rb @@ -119,6 +119,17 @@ class InlineComponentDerivedFromComponentSupportingVariants < Level2Component assert_match %r{test/sandbox/test/inline_template_test.rb:22}, error.backtrace[0] end + test "error backtrace locations work when template annotations are disabled" do + error = nil + + without_template_annotations do + InlineRaiseErbComponent.__vc_compile(force: true) + error = assert_raises(ArgumentError) { render_inline(InlineRaiseErbComponent.new("Fox Mulder")) } + end + + assert_match %r{test/sandbox/test/inline_template_test.rb:22}, error.backtrace[0] + end + test "error backtrace locations work in slim" do error = assert_raises ArgumentError do render_inline(InlineRaiseSlimComponent.new("Fox Mulder")) diff --git a/test/sandbox/test/template_annotation_test.rb b/test/sandbox/test/template_annotation_test.rb new file mode 100644 index 000000000..4d291ada1 --- /dev/null +++ b/test/sandbox/test/template_annotation_test.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +require "test_helper" + +class TemplateAnnotationTest < ViewComponent::TestCase + # Rails only prepends the `` annotation, and the newline it + # contains, when annotations are enabled *and* the template's format is HTML. + # The line number compensation ViewComponent applies has to match those same + # conditions, otherwise backtraces point at the wrong line and, when coverage + # is running, template source is stripped away. + def setup + skip unless Rails::VERSION::MAJOR >= 8 && Rails::VERSION::MINOR > 0 + end + + def teardown + [RaisingFormatsComponent, MultipleFormatsComponent].each { |c| c.__vc_compile(force: true) } + end + + def test_backtrace_line_number_for_html_template_with_annotations + RaisingFormatsComponent.__vc_compile(force: true) + + error = assert_raises(ArgumentError) { render_inline(RaisingFormatsComponent.new) } + + assert_equal 2, template_line_number(error, "raising_formats_component.html.erb") + end + + def test_backtrace_line_number_for_html_template_without_annotations + error = nil + + without_template_annotations do + RaisingFormatsComponent.__vc_compile(force: true) + error = assert_raises(ArgumentError) { render_inline(RaisingFormatsComponent.new) } + end + + assert_equal 2, template_line_number(error, "raising_formats_component.html.erb") + end + + def test_backtrace_line_number_for_non_html_template + RaisingFormatsComponent.__vc_compile(force: true) + + error = assert_raises(ArgumentError) do + with_format(:text) { render_inline(RaisingFormatsComponent.new) } + end + + assert_equal 2, template_line_number(error, "raising_formats_component.text.erb") + end + + def test_non_html_template_renders_its_content_when_coverage_is_running + with_coverage_running { MultipleFormatsComponent.__vc_compile(force: true) } + + with_format(:css) { render_inline(MultipleFormatsComponent.new) } + + assert_includes @rendered_content, "Hello, CSS!" + end + + private + + def template_line_number(error, template_file) + entry = error.backtrace.find { |line| line.include?(template_file) } + + refute_nil entry, "Expected #{template_file} in backtrace:\n#{error.backtrace.first(5).join("\n")}" + + entry[/:(\d+):/, 1].to_i + end + + def with_coverage_running + require "coverage" + already_running = Coverage.running? + Coverage.start unless already_running + yield + ensure + Coverage.result unless already_running + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index 7c2ab7ad4..bdbdf2a54 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -155,7 +155,7 @@ def without_template_annotations(&block) app.reloader.reload! if defined?(app) with_new_cache(&block) - +ensure ActionView::Base.annotate_rendered_view_with_filenames = old_value app.reloader.reload! if defined?(app) end