Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable send_preload_links_header outright #51436

Open
stanhu opened this issue Mar 28, 2024 · 1 comment · May be fixed by #51441
Open

Disable send_preload_links_header outright #51436

stanhu opened this issue Mar 28, 2024 · 1 comment · May be fixed by #51441

Comments

@stanhu
Copy link
Contributor

stanhu commented Mar 28, 2024

Steps to reproduce

Suppose you have a partial like this:

= cache([ActionController::Base.asset_host], expires_in: 1.minute) do
    = preload_link_tag(path_to_stylesheet('application')

If the partial is not cached, then preload_link_tag will call send_preload_links_header, which modifies the Link HTTP header.

However, if the partial is cached, then the Link HTTP header is not sent.

This is a surprising side effect of caching preload_link_tag. I was surprised that send_preload_links_header is even called in the first place.

#48405 lowered the max header size from 8K to 1000 bytes, but I think Rails should go farther than that: provide an option to disable this sending altogether, perhaps as an argument to preload_link_tag. For example:

diff --git a/actionview/lib/action_view/helpers/asset_tag_helper.rb b/actionview/lib/action_view/helpers/asset_tag_helper.rb
index 11528eb33d..0dd3ff517a 100644
--- a/actionview/lib/action_view/helpers/asset_tag_helper.rb
+++ b/actionview/lib/action_view/helpers/asset_tag_helper.rb
@@ -325,6 +325,7 @@ def preload_link_tag(source, options = {})
         crossorigin = "anonymous" if crossorigin == true || (crossorigin.blank? && as_type == "font")
         integrity = options[:integrity]
         nopush = options.delete(:nopush) || false
+        send_link_header = options.key?(:preload_links_header) ? options.delete(:preload_links_header) : true
         rel = mime_type == "module" ? "modulepreload" : "preload"
 
         link_tag = tag.link(**{
@@ -341,7 +342,7 @@ def preload_link_tag(source, options = {})
         preload_link += "; integrity=#{integrity}" if integrity
         preload_link += "; nopush" if nopush
 
-        send_preload_links_header([preload_link])
+        send_preload_links_header([preload_link]) if send_link_header
 
         link_tag
       end
diff --git a/actionview/test/template/asset_tag_helper_test.rb b/actionview/test/template/asset_tag_helper_test.rb
index f0c8a10f57..cd6cdd8925 100644
--- a/actionview/test/template/asset_tag_helper_test.rb
+++ b/actionview/test/template/asset_tag_helper_test.rb
@@ -628,6 +628,13 @@ def test_should_set_preload_early_hints_with_rel_modulepreload
     end
   end
 
+  def test_should_not_set_preload_link_with_rel_modulepreload
+    with_preload_links_header do
+      preload_link_tag("http://example.com/all.js", type: "module", preload_links_header: false)
+      assert_nil @response.headers["Link"]
+    end
+  end
+
   def test_should_set_preload_links_with_integrity_hashes
     with_preload_links_header do
       stylesheet_link_tag("http://example.com/style.css", integrity: "sha256-AbpHGcgLb+kRsJGnwFEktk7uzpZOCcBY74+YBdrKVGs")
@stanhu
Copy link
Contributor Author

stanhu commented Mar 28, 2024

I see there is also ActionView::Helpers::AssetTagHelper.preload_links_header defined. Perhaps we can just check that?

stanhu added a commit to stanhu/rails that referenced this issue Mar 28, 2024
This commit adds a `preload_links_header` option to `preload_link_tag`
to disable sending of the Link header in the HTTP response.

Currently `ActionView::Helpers::AssetTagHelper.preload_links_header`
only controls whether `javascript_include_tag` and
`stylesheet_link_tag` send the Link header, but there is no way to
control the behavior of `preload_link_tag`. We could just check that
variable, but existing applications might be relying on
`preload_link_tag` to add the header. Users also might want control
over this on a per call basis.

Closes rails#51436
stanhu added a commit to stanhu/rails that referenced this issue Mar 28, 2024
This commit adds a `preload_links_header` option to `preload_link_tag`
to disable sending of the Link header in the HTTP response.

Currently `ActionView::Helpers::AssetTagHelper.preload_links_header`
only controls whether `javascript_include_tag` and
`stylesheet_link_tag` send the Link header, but there is no way to
control the behavior of `preload_link_tag`. We could just check that
variable, but existing applications might be relying on
`preload_link_tag` to add the header. Users also might want control
over this on a per call basis.

Closes rails#51436
stanhu added a commit to stanhu/rails that referenced this issue Mar 28, 2024
This commit adds a `preload_links_header` option to `preload_link_tag`
to disable sending of the Link header in the HTTP response.

Currently `ActionView::Helpers::AssetTagHelper.preload_links_header`
only controls whether `javascript_include_tag` and
`stylesheet_link_tag` send the Link header, but there is no way to
control the behavior of `preload_link_tag`. We could just check that
variable, but existing applications might be relying on
`preload_link_tag` to add the header. Users also might want control
over this on a per call basis.

Closes rails#51436
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants