[RFC] Disable Rails/ContentTag

[jesseclark]

What

Disable the Rails/ContentTag cop.

Why

There is an open PR for Rubocop that makes a pretty compelling argument that this cop targets the wrong method.

And you can see in the Rails docs that there is a legacy syntax section for the #tag method but no deprecation or legacy syntax is mentioned for #content_tag.

cc: @cookpad/web-chapter

[balvig]

Interesting find!

I'm not sure about this - in particular the "targeting the wrong method" bit 🤔

I recall tag.something being introduced as "the new hotness" back in Rails 5.1, to replace both content_tag(:something) and tag(:something):

# To produce <span class="bookmark"></span> before:
content_tag :span, nil, class: 'bookmark'
# After (because we will keep an index of tags that need closing, like <span>, vs those that do not, like <br>)
tag.span class: 'bookmark'

# To produce <div id="post_1"> before:
content_tag :div, post.title, id: dom_id(post)  
# After:
tag.div post.title, id: dom_id(post)

# To produce <br> before:
tag :br, nil, true
# After (becomes like span, we know this tag doesn't need closing)
tag.br

# Would be nice to support nesting too, so to produce <div id="header"><span>hello</span></div>, you'd:
tag.div(id: 'header') { |tag| tag.span 'hello' }

• Provide HTML tag dynamic proxy rails/rails#25195
• New syntax for tag helpers i.e. tag.br instead of tag('br') #25195 rails/rails#25543

I certainly prefer the syntax of tag.something.

The slowness is a little concerning, but I suppose we don't use Rails or Ruby mainly for its speed and ultimately any Rails HTML helper will always be slower than a raw string.

I'm not good enough with numbers to tell from the benchmarks what using tag.something means in terms of total rendering speed on an "average page" in Real Life™️, whether for the kind of pages we build it constitutes enough of a performance problem that we should avoid it entirely?

However, the general approach we've taken so far is to use any syntactic sugar that's available in the framework and only drop down when/if needed (for hotspots that would usually mean going all the way to writing "manual" HTML).

So, not really concluding anything here 😅, but while it's true that it's hard to find any mention of what the future holds for content_tag, it seems like the intent was for it to be replaced by tag.something and if the performance aspect isn't clearly a major problem, it might still be worth encouraging the nicer syntax through linting?

[davidstosik]

Yeah, I think the Rubocop cop might have been a bit misleading, and there are multiple levels to look at:

• the tag(:div) (no block, no content) was announced as soon deprecated, in favor of tag.div (but it has been for 5 years 😅 )
• content_tag(:div (with content passed either with a string or a block) is commented as "legacy syntax" (aaand, that comes from the same PR too... 🤦🏻‍♂️ )
• the new tag.div syntax might be relatively slower because of its use of method_missing but seems to be recommended above both the above

About performance, it's always a bit tricky because we always want to keep things fast and optimized, but it's important to put it in relation to its cost in terms of development comfort.

In the linked issue's benchmark, there seems to be an average 1.3ms difference between the fastest and slowest of equivalent calls using different syntaxes (tag.span("hey") vs content_tag(:span, "hey")). That could add up if we use either a lot: if we have a page that makes 100 such calls in a single render, then that means we'd find a way to reduce render time by 130ms.

100 calls in a single page render are not unrealistic for a page with no cache, but I would expect it to be a lot less when making good use of partial caching.

I want to also mention that I feel that we may have been abusing tag or content_tag, in situations where pure HTML would have been just fine, and should have been preferred above either.

-<%= tag :meta, content: "False", itemprop: "isAccessibleForFree" %>
+<meta content="False" itemprop="isAccessibleForFree">

-<%= content_tag :div, nil, class: "text-center sm:text-left card p-sm", data: { controller: "banner" } do %>
+<div class="text-center sm:text-left card p-sm" data-controller="banner">

Here's another data point: it looks like we currently have 120 of such calls to either tag or content_tag in our whole codebase (ag -s ' (tag[. (]|content_tag)' app/{helpers,views} | wc -l).

In the end, I agree that the performance issue might be a concern, which we may benefit from looking into, but I also think that we should try to steer away from syntaxes that feel less comfortable to use (and may be legacy, although that kinda needs to be reassessed...).

If performance is really a concern, would there be an easy way for us to profile calls to tag and content_tag using ScoutAPM?

I also wonder if it would be worth it starting a conversation on rails/rails regarding whether content_tag and tag(:span are actually "legacy" or not. (Rails itself still uses the so-called "legacy" syntaxes a lot. 😅)

After all this, I'm not sure anymore whether the Rubocop cop is justified, but I'm not sure either it would make sense to keep so many ways of doing the same thing. 🤔

[jesseclark]

content_tag(:div (with content passed either with a string or a block) is commented as "legacy syntax" (aaand, that comes from the same PR too... 🤦🏻‍♂️ )

I missed that line in the comments/description. I was scanning for a "Legacy syntax" section in the docs as they added for the old flavor of tag

On that note, what does "legacy syntax" even mean? Is this an official designation or does it just indicate "we like this other way better and maybe someday might deprecate these other ways"?

Either way, if there is possibly some performance gain to using content_tag, and it has not been deprecated, do we want Rubocop telling us we have to change it to the possibly slower new form of tag at this time?

Maybe if it does actually become deprecated we can address changing content_tag to tag at that point?

I'm not sure either it would make sense to keep so many ways of doing the same thing. 🤔

Isn't "many ways of doing the same thing" kinda part of the core philosophy or Ruby? 😉 😄

[davidstosik]

Isn't "many ways of doing the same thing" kinda part of the core philosophy or Ruby? 😉 😄

I was expecting that comment, thank you for not letting me down! 😄
Yeah, I guess you're right, but in a way, our style guides (and Rubocop automation too) are here in order to add a bit of consistency to the codebase. 🤔

I agree though that the "legacy" state of both content_tag and tag's old syntax is dubious at best. Not getting a single deprecation warning in 5 years after declaring "legacy" makes me think that they might be here to stay... 🤔

[jesseclark]

I was expecting that comment, thank you for not letting me down! 😄

Happy to have fulfilled the expected trolling function! 😆

Yeah, I guess you're right, but in a way, our style guides (and Rubocop automation too) are here in order to add a bit of consistency to the codebase. 🤔

I totally agree on consistency in general especially with regards to higher-level patterns and problematic syntax.

In this instance, forcing consistency feels a bit low-level and unnecessary to me. However, if the team is in favor of requiring the new tag syntax (despite the possibly minor performance issues), I am fine with that.

I just found it a bit annoying to have Rubocop ping me about an already existing usage of content_tag while I am extracting a partial when the usage doesn't really seem to have any significant impact.

[balvig]

found it a bit annoying to have Rubocop ping me about an already existing usage

Hehe, yes I know that feeling!
I think that's maybe more of a general problem? 😓

If you're just moving things around, it feels "unfair" to have a bot (or human!) tell you to fix old code that just happens to be inside the block you're moving. 😅

(You're welcome to tell the bot "listen, I'm just moving this" in those cases btw!)

As for content_tag :something vs tag.something, although it's maybe not the biggest difference in the world, I guess all else being equal I do like the latter syntax better and there's enough evidence to suggest that it might be The Future.

In that regard, our linter helps not only with consistency, but also in making us aware of alternatives (at any level) that, even if the difference is minor, we sort of accept are "better" (or at least not worth having to think about.)

I also agree with @davidstosik that a lot of the time, plain text HTML is perfectly adequate and even preferable! (otherwise we would be writing HAML 😅 )

Therefore, in the context of performance optimization, it would probably make more sense to go from tag.something to <something>...</something> rather than the intermediate step of moving to content_tag :something?

(incidentally, seems to me that if method_missing is a significant bottleneck, then it maybe wouldn't be unheard of to at least define static methods within Rails for some of the more common tags? 🤔 )

All that is to say that, I guess having talked this through, I'm personally leaning towards keeping the cop? 🤔

[davidstosik]

(incidentally, seems to me that if method_missing is a significant bottleneck, then it maybe wouldn't be unheard of to at least define static methods within Rails for some of the more common tags? 🤔 )

Great point!

I wanted to give it a try, and first thing I did was to write down a benchmark (basically the same that is in the PR @jesseclark linked), and that is when I realized the submitter's benchmark is using Benchmark.ms (an ActiveSupport extension that "benchmarks realtime in milliseconds") not Ruby's method (which display time in seconds). 😅

So it turns out that the values he showed are not seconds but milliseconds, and that the timings I mentioned in my previous comment must be divided by 1000! I thought my conclusions were a bit surprising...

So we're actually talking about a 1.3µs* (microsecond or 0.0000013s) difference on average between a call to content_tag and a call to tag.something. We could have hundreds, if not thousands of such calls in a single page render without having a humanly noticeable impact on the rendering time.

In conclusion, I don't think the performance issue is a concern for us.

Here's a stand-alone benchmark script:

#/usr/bin/env rails runner

SAMPLE_SIZE = 50_000

def helper
  ApplicationController.helpers
end

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

Running via Spring preloader in process 13173
Rehearsal ----------------------------------------------------------
content_tag              0.193552   0.083911   0.277463 (  0.278568)
content_tag with block   0.251993   0.047979   0.299972 (  0.300957)
tag                      0.185263   0.000837   0.186100 (  0.186436)
tag with block           0.275467   0.001495   0.276962 (  0.277528)
------------------------------------------------- total: 1.040497sec

                             user     system      total        real
content_tag              0.097321   0.000257   0.097578 (  0.097907)
content_tag with block   0.192518   0.002618   0.195136 (  0.195436)
tag                      0.141737   0.000855   0.142592 (  0.143032)
tag with block           0.281842   0.001570   0.283412 (  0.283975)

(* On the PR's submitter's machine. 1.8µs on mine.)

---

I was curious about @balvig's suggestion for optimization by skipping missing_method, so I finished the thought above anyway, and my results are not so conclusive
In the code 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

# check that the method was properly defined
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.099976   0.020933   0.120909 (  0.121246)
tag.span                 0.196209   0.038664   0.234873 (  0.235145)
tag.fast_span            0.173272   0.003499   0.176771 (  0.177052)
------------------------------------------------- total: 0.532553sec

                             user     system      total        real
content_tag(:span        0.094540   0.001354   0.095894 (  0.096130)
tag.span                 0.141989   0.000936   0.142925 (  0.143383)
tag.fast_span            0.139464   0.001009   0.140473 (  0.141305)

The results are surprising: there is an improvement, but very tiny and not even close to being on par with content_tag. The performance loss of using tag.span instead of content_tag(:span does not seem to be due to the use of method_missing.

[jesseclark]

Ok. Thanks for the discussion, everyone. And @davidstosik thanks for the performance investigations!

The cop stays and I will close this ticket. 👍🏼

[balvig]

Thank you for raising @jesseclark. 🙏
Although we ended up leaving as is for now, I learned quite a bit from the discussion!👌