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

Automatically set Link header for each stylesheet and script #39939

Merged
merged 1 commit into from
Aug 17, 2020

Conversation

casperisfine
Copy link
Contributor

elements can be serialized in Link headers to allow the browser to preload them before it parsed the HTML body.

It is particularly useful for scripts included at the bottom of the document.

Implementation

This piggy backs on Early Hints as they are fundamentally the same feature, Early Hint simply tell the browser in advance what the Link header is likely to look like.

In term of serialization, we can either send multiple Link headers, or set multiple comma separated values in a single header. I chose the later as it's a bit more compact.

cc @eileencodes @tenderlove since you implemented early hints, you might have an opinion on this.
cc @rafaelfranca

<link rel="preload"> elements[0] can be serialized in `Link`
headers[1] to allow the browser to preload them before
it parsed the HTML body.

It is particularly useful for scripts included at the bottom
of the document.

[0] https://developer.mozilla.org/en-US/docs/Web/HTML/Preloading_content
[1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Link
@rails-bot rails-bot bot added the actionview label Jul 28, 2020
@eileencodes eileencodes merged commit 443c847 into rails:master Aug 17, 2020
@eileencodes
Copy link
Member

Sorry I missed this awhile back! Looks good, thanks!

@casperisfine
Copy link
Contributor Author

Thanks!

@fleck
Copy link
Contributor

fleck commented Aug 20, 2020

@casperisfine @eileencodes
This is a great step forward for a fast by default experience, but there are some quirks to it the way it's currently implemented.

@casperisfine
Copy link
Contributor Author

@fleck both good points. I'm swamped just now, but I'll see if I can come up with something early next week, unless you beat me to it.

Causing pushes is definitely a bad idea, and if people want to do that then it should definitely be explicit.

@kirs kirs deleted the link-preload-headers branch December 7, 2020 11:54
pixeltrix added a commit that referenced this pull request Dec 18, 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 added a commit that referenced this pull request 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 added a commit that referenced this pull request Dec 21, 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.

(cherry picked from commit 200083c)
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)
@zavan
Copy link
Contributor

zavan commented Feb 26, 2021

Just in case anyone else bumps into this:

This header can get very large in a dev environment with a lot of assets (like the Spree admin). If you're using Apache in front of a Rails dev server, like I am (in a VM, to test how it would work in prod) the header may overflow, Apache will truncate it to less than 8KiB and possibly break the response. In my case Apache was sending an incomplete response (missing the Content-Type header), the only way to fix it was by disabling this feature in config/environments/development.rb (#40882):

config.action_view.preload_links_header = false

It's probably not a problem in production because CSS and JS will usually be concatenated into less files.

Also,

In term of serialization, we can either send multiple Link headers, or set multiple comma separated values in a single header. I chose the later as it's a bit more compact.

I don't know if sending multiple Link headers would make a difference in this case, probably not.
Maybe Rails should only send the header if it doesn't exceed a certain limit? Apache has an 8 KiB limit for the headers, for nginx it's 4-8 KiB. I can open a separate issue if this is something to be considered.

@LilyReile
Copy link
Contributor

@zavan Any idea why apache chooses 8KB as the response limit?

@zavan
Copy link
Contributor

zavan commented Apr 20, 2021

@zavan Any idea why apache chooses 8KB as the response limit?

I don't know, the HTTP spec itself does not enforce or recommends a limit.
It may be related to security concerns, see https://security.stackexchange.com/a/113365/237786 and https://maxchadwick.xyz/blog/http-response-header-size-limits.

A limit is needed, for sure, so maybe they just chose 8KB a long time ago and never really changed it, even though it could be larger nowadays without problems.

Also note that this limit is for the headers only, not the entire response, of course.

@LilyReile
Copy link
Contributor

LilyReile commented Apr 22, 2021

@zavan Based on your links it looks like every major tech that handles HTTP imposes a limit of 8192 bytes for headers. I'm guessing this adheres to the original RFC that recommends that any such limit be a minimum of 8 KB.

@casperisfine
Copy link
Contributor Author

I might have a patch for this: #42056

Would one of you be willing to test it?

@mjtko
Copy link
Contributor

mjtko commented May 10, 2021

I'm happy to open a new issue/redirect to a forum to cover this if it's not suitable to add a comment/question here, but this functionality can cause an awful lot of data to get sent in headers when stylesheet_link_tag is used with a data URL.

In my use case, this is happening with rendering a PDF -- the Link header is populated with >100KiB of Base64-ed CSS which breaks clients that don't want to process headers that large! I see that #42056 will break the header down, but it seems to me like there would still be a lot of data being sent in headers that, certainly in my case, isn't needed.

Does it definitely make sense to include data URLs in the Link header? Please forgive my lack of understanding, I'm not familiar with the mechanism, or what it's intending to achieve.

@casperisfine
Copy link
Contributor Author

Does it definitely make sense to include data URLs in the Link header?

No it doesn't that's an oversight and should be fixed.

chadlwilson added a commit to chadlwilson/gocd that referenced this pull request Aug 16, 2022
Currently this leads to a massive response header in development setup which breaches current default Jetty limits (of 8 kB)

rails/rails#39939
chadlwilson added a commit to chadlwilson/gocd that referenced this pull request Aug 17, 2022
Currently this leads to a massive response header in development setup which breaches current default Jetty limits (of 8 kB)

rails/rails#39939
chadlwilson added a commit to chadlwilson/gocd that referenced this pull request Aug 17, 2022
Currently this leads to a massive response header in development setup which breaches current default Jetty limits (of 8 kB)

rails/rails#39939
chadlwilson added a commit to chadlwilson/gocd that referenced this pull request Aug 21, 2022
Currently this leads to a massive response header in development setup which breaches current default Jetty limits (of 8 kB)

rails/rails#39939
chadlwilson added a commit to chadlwilson/gocd that referenced this pull request Aug 22, 2022
Currently this leads to a massive response header in development setup which breaches current default Jetty limits (of 8 kB)

rails/rails#39939
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

7 participants