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

Add config.action_view.preload_links_header option #40882

Merged
merged 1 commit into from
Dec 21, 2020

Conversation

pixeltrix
Copy link
Contributor

PR #39939 added support for the Link header being generated automatically when using stylesheet_link_tag and
javascript_include_tag. However not everything should be preloaded, e.g. a link to a legacy IE stylesheet has no need to be preloaded because IE doesn't support the header and in some browsers it will trigger the preload even though it's not used since it's inside an IE conditional comment. This leads to increased bandwith costs and slower application performance.

To allow more flexibility for sites that may have complex needs for the Link header this commit adds a configuration option that disables it completely and leaves it up to the application to decide how to handle generating a Link header.

NOTE: The intention is to backport this to 6-1-stable and add a new entry to new_framework_defaults_6_1.rb so that upgrading applications can opt into the default option of it being enabled since they're the most likely to be affected by the legacy IE issue.

@rafaelfranca
Copy link
Member

It is missing documentation of config.action_view.preload_links_header what are the default values in the different version of rails in the configuring guide. Otherwise looks good to me.

@pixeltrix pixeltrix force-pushed the add-preload-links-header-config branch from e438206 to cffdc5c Compare December 19, 2020 05:16
@rails-bot rails-bot bot added the docs label Dec 19, 2020
PR #39939 added support for the `Link` header being generated
automatically when using `stylesheet_link_tag` and
`javascript_include_tag`. However not everything should be
preloaded, e.g. a link to a legacy IE stylesheet has no need to be
preloaded because IE doesn't support the header and in some browsers it
will trigger the preload even though it's not used since it's inside an
IE conditional comment. This leads to increased bandwith costs and
slower application performance.

To allow more flexibility for sites that may have complex needs for the
`Link` header this commit adds a configuration option that disables it
completely and leaves it up to the application to decide how to handle
generating a `Link` header.
@pixeltrix pixeltrix force-pushed the add-preload-links-header-config branch from cffdc5c to 200083c Compare December 19, 2020 05:24
@pixeltrix
Copy link
Contributor Author

@rafaelfranca Added to the 6.1 release notes and the configuration guide - was there anywhere else you were thinking? I wondered whether to add it to the assets.rb initializer as a commented out option to turn it off so that it's a bit more visible but decided against it since it seemed a bit tangential to assets and don't really want to put it in an initializer by itself.

@pixeltrix pixeltrix merged commit 71bc414 into master Dec 21, 2020
@pixeltrix pixeltrix deleted the add-preload-links-header-config branch December 21, 2020 06:41
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Jan 8, 2021
…der` `nil` without Rails 6.1 defaults

In rails#39939 we automatically set `Link` header for each stylesheet and script.
In rails#40882 we  added `config.action_view.preload_links_header` option
to configure whether `stylesheet_link_tag` and `javascript_include_tag`
should set automatically `Link` header.

This commit adds test to make sure that it is disabled by default
for updated apps that haven't adopted new '6.1' defaults.
Also, added changes to Configuring guide
 - Mentioned this option in "Results of `config.load_defaults`" section
 - Mentioned this option in "Baseline defaults:" section (see ff88113)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants