The Rails/ContentTag should only target the deprecated tag helper

[czj]

Hello.

I'm quite uncertain about the deprecation of the content_tag helper method targeted by the new Rails/ContentTag cop in #242 as it seems to me, reading Rails source code, that only the legacy tag method is deprecated.

The difference between tag and content_tag helper methods

When reading the Rails documentation I see that the legacy tag syntax use with arguments is deprecated and will be removed. It is called like so :

tag(:img, src: "/flower.jpg")

<img src="/flower.jpg" />

It returns a tag that is by default not open, and XHTML compliant because it ends with a /> at the end.

When you look at the source of the method, you see why tag is deprecated and will be replaced:

def tag(name = nil, options = nil, open = false, escape = true)
  if name.nil?
    tag_builder
  else
    "<#{name}#{tag_builder.tag_options(options, escape) if options}#{open ? ">" : " />"}".html_safe
  end
end

This method builds its HTML string by itself and checks for name nullity to piggy back on tag_builder

The content_tag helper method on the other hand generates an HTML 5 compliant output with a closing tag at the end.

The source code shows it utilises tag_builder all the time and outputs good HTML code:

# File actionview/lib/action_view/helpers/tag_helper.rb, line 270
def content_tag(name, content_or_options_with_block = nil, options = nil, escape = true, &block)
  if block_given?
    options = content_or_options_with_block if content_or_options_with_block.is_a?(Hash)
    tag_builder.content_tag_string(name, capture(&block), options, escape)
  else
    tag_builder.content_tag_string(name, content_or_options_with_block, options, escape)
  end
end

Solution

I'm proposing changing this cop to only warn of usage of the the tag helper method with arguments like :

tag :br

And not when used with the argument-less syntax.

tag.br

Why I think it matters

Usage of tag.something is way slower than using content_tag(:something) as you can see with this simple benchmark :

50_000.times { tag.span("hey") } }
# 132.25800002692267

50_000.times { content_tag(:span, "hey") } }
# 69.53899998916313

50_000.times { tag.span { "hey" } } }
# 199.94600000791252

50_000.times { content_tag(:span) { "hey" } } }
# 161.7930000065826

You can reproduce it by dropping this code inside any view :

Rails.logger.debug %(50_000.times { tag.span("hey") } })
Rails.logger.debug(Benchmark.ms { 50_000.times { tag.span("hey") } })

Rails.logger.debug %(50_000.times { tag.span { "hey" } } })
Rails.logger.debug(Benchmark.ms { 50_000.times { tag.span { "hey" } } })

Rails.logger.debug %(50_000.times { content_tag(:span, "hey") } })
Rails.logger.debug(Benchmark.ms { 50_000.times { content_tag(:span, "hey") } })

Rails.logger.debug %(50_000.times { content_tag(:span) { "hey" } } })
Rails.logger.debug(Benchmark.ms { 50_000.times { content_tag(:span) { "hey" } } })

Avoiding suggesting to switch to an almost 2 times slower method would save a ton of CPU time in thousands of Rails applications. The framework is not particularly focused on pure speed, so every little bit of performance improvements helps, I think.

Why tag.something is slow

The new tag helper relies method_missing on tag_builder which is very slow. compared to passing a string.

https://github.com/rails/rails/blob/a5cda0407fffe4214db3c40487d9d6394b391bc1/actionview/lib/action_view/helpers/tag_helper.rb#L119

[davidstosik]

Hello, I've been looking into this, as in my team we are currently discussing whether to use content_tag or tag., and even though I don't have a definite answer on the topic, there are two things I wanted to add to the conversation.

1. Scale of the performance problem

In the benchmarks @czj shared, it is important to note the results are in milliseconds for 50,000 calls. That gives us 1.3µs (microsecond or 0.0000013s) of difference on average, between a call to content_tag and tag.. I focused on the equivalent syntaxes passing a string as the focus is on content_tag vs tag (not on string vs block syntax).

This is indeed a performance difference, which adds up when looking at a complete page render, but I think it is necessary to put things in perspective: how many calls to tag. for a single page will you need to have a humanly noticeable impact on the time it takes to render the page? This present GitHub page has a bit less than 2000 HTML tags (at the time of writing, using document.querySelectorAll("*")). Even if all the tags on this page were rendered using tag., the impact of overusing content_tag would be less than 5ms. Bigger pages might reach 5000, 10000 HTML tags, and even then, when all tags are rendered with tag., you'd be seeing an impact no bigger than 20ms, and that was a lot of ifs. 😬

2. Impact of method_missing over performance

As we were curious whether avoiding method_missing by explicitly defining methods for most frequently used tags could improve tag.'s performance, I gave it a try.

In the script below, I defined the fast_span method on TagBuilder, which would behave like the span method, except that it does not need to go through method_missing.

#!/usr/bin/env rails runner

SAMPLE_SIZE = 50_000

require "action_view/helpers/tag_helper"

class ActionView::Helpers::TagHelper::TagBuilder
  def fast_span(*args, **options, &block)
    tag_string(:span, *args, **options, &block)
  end
end

helper = ApplicationController.helpers

raise unless helper.tag.fast_span("hey") == "<span>hey</span>"

Benchmark.bmbm(22) do |x|
  x.report("content_tag(:span") { SAMPLE_SIZE.times { helper.content_tag(:span, "hey") } }
  x.report("tag.span") { SAMPLE_SIZE.times { helper.tag.span("hey") } }
  x.report("tag.fast_span") { SAMPLE_SIZE.times { helper.tag.fast_span("hey") } }
end

Rehearsal ----------------------------------------------------------
content_tag(:span        0.095163   0.022317   0.117480 (  0.117734)
tag.span                 0.187410   0.040893   0.228303 (  0.229270)
tag.fast_span            0.155418   0.000441   0.155859 (  0.156249)
------------------------------------------------- total: 0.501642sec

                             user     system      total        real
content_tag(:span        0.088313   0.000200   0.088513 (  0.088697)
tag.span                 0.135331   0.000831   0.136162 (  0.136659)
tag.fast_span            0.131357   0.000635   0.131992 (  0.132262)

The results are surprising: there is an improvement, but very tiny and not even close to being on par with content_tag. In conclusion, I don't think the performance loss of using tag.span instead of content_tag(:span is due to the use of method_missing.

[koic]

@tabuchi0919 You are the author of Rails/ContentTag cop . Can you take this issue?

[tabuchik]

@koic

Thank you for mentioning, and I will try this issue this weekend.
I read this issue and cookpad/global-style-guides#187, I summarized as below.

• tag(:something) is deprecated. No one wants to use this, as far as I read.
• content_tag(:something) may be deprecated. Someone is suspicious of it, and it is difficult to draw a conclusion.
• content_tag(:something) is faster than tag.something, but tag.something is better rendering HTML5. It handles self closing tag.

So, I want to add options.

• (Current behavior) content_tag(:something) is forbidden unless tag_name is a variable.
• (New option) content_tag(:something) and tag.something are allowed, tag(:something) is fixed to tag.something.
• (New option?) tag.something and tag(:something) are forbidden. tag(:something) and tag.something are fixed to content_tag(something). (but I don't know if I can add this rule 😢)

Any ideas or suggestions would be welcome.

[koic]

@tabuchi0919 Thank you for taking this! I think that APIs that are unclear whether they are deprecated can be allowed by default. Also, auto-correction for non-deprecated APIs should not cause performance regression.
Here is a technical note, the name Rails/ContentTag may not be appropriate, but the cop name cannot be changed to prevent breaking changes.

[ngouy]

With this new version, I have issues with rubocop running on a pure ruby (no HTML or whatsoever) file that plays with act_as_taggable (code contains some .tag calls tag_something etc ...

An error occurred while Rails/ContentTag cop was inspecting /Users/nathangouy/Work/**************/app/models/concerns/tenant_tagging.rb

Bugs on these 2 lines: