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
Implement H2 Early Hints for Rails #30744
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,9 @@ module AssetTagHelper | |
# When the Asset Pipeline is enabled, you can pass the name of your manifest as | ||
# source, and include other JavaScript or CoffeeScript files inside the manifest. | ||
# | ||
# If the server supports Early Hints header links for these assets will be | ||
# automatically pushed. | ||
# | ||
# ==== Options | ||
# | ||
# When the last parameter is a hash you can add HTML attributes using that | ||
|
@@ -77,12 +80,20 @@ module AssetTagHelper | |
def javascript_include_tag(*sources) | ||
options = sources.extract_options!.stringify_keys | ||
path_options = options.extract!("protocol", "extname", "host", "skip_pipeline").symbolize_keys | ||
sources.uniq.map { |source| | ||
early_hints_links = [] | ||
|
||
sources_tags = sources.uniq.map { |source| | ||
href = path_to_javascript(source, path_options) | ||
early_hints_links << "<#{href}>; rel=preload; as=script" | ||
tag_options = { | ||
"src" => path_to_javascript(source, path_options) | ||
"src" => href | ||
}.merge!(options) | ||
content_tag("script".freeze, "", tag_options) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to change this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that desirable in general in internal stuff? The improvement for user code is nice, but it feels like fairly pointless (and comparatively allocation-heavy) indirection for us. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, it's a bit of a back and forth. I was focusing on removing the |
||
}.join("\n").html_safe | ||
|
||
request.send_early_hints("Link" => early_hints_links.join("\n")) | ||
|
||
sources_tags | ||
end | ||
|
||
# Returns a stylesheet link tag for the sources specified as arguments. If | ||
|
@@ -92,6 +103,9 @@ def javascript_include_tag(*sources) | |
# to "screen", so you must explicitly set it to "all" for the stylesheet(s) to | ||
# apply to all media types. | ||
# | ||
# If the server supports Early Hints header links for these assets will be | ||
# automatically pushed. | ||
# | ||
# stylesheet_link_tag "style" | ||
# # => <link href="/assets/style.css" media="screen" rel="stylesheet" /> | ||
# | ||
|
@@ -113,14 +127,22 @@ def javascript_include_tag(*sources) | |
def stylesheet_link_tag(*sources) | ||
options = sources.extract_options!.stringify_keys | ||
path_options = options.extract!("protocol", "host", "skip_pipeline").symbolize_keys | ||
sources.uniq.map { |source| | ||
early_hints_links = [] | ||
|
||
sources_tags = sources.uniq.map { |source| | ||
href = path_to_stylesheet(source, path_options) | ||
early_hints_links << "<#{href}>; rel=preload; as=stylesheet" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the example described in https://w3c.github.io/preload/#as-attribute, it seems that the |
||
tag_options = { | ||
"rel" => "stylesheet", | ||
"media" => "screen", | ||
"href" => path_to_stylesheet(source, path_options) | ||
"href" => href | ||
}.merge!(options) | ||
tag(:link, tag_options) | ||
}.join("\n").html_safe | ||
|
||
request.send_early_hints("Link" => early_hints_links.join("\n")) | ||
|
||
sources_tags | ||
end | ||
|
||
# Returns a link tag that browsers and feed readers can use to auto-detect | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,6 +127,7 @@ class ServerCommand < Base # :nodoc: | |
class_option "dev-caching", aliases: "-C", type: :boolean, default: nil, | ||
desc: "Specifies whether to perform caching in development." | ||
class_option "restart", type: :boolean, default: nil, hide: true | ||
class_option "early_hints", type: :boolean, default: nil, desc: "Enables HTTP/2 early hints." | ||
|
||
def initialize(args = [], local_options = {}, config = {}) | ||
@original_options = local_options | ||
|
@@ -161,7 +162,8 @@ def server_options | |
daemonize: options[:daemon], | ||
pid: pid, | ||
caching: options["dev-caching"], | ||
restart_cmd: restart_command | ||
restart_cmd: restart_command, | ||
early_hints: early_hints | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use it directly like dev-caching / daemon ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I put the documentation in the early hints option. It's passed in like |
||
} | ||
end | ||
end | ||
|
@@ -227,6 +229,10 @@ def restart_command | |
"bin/rails server #{@server} #{@original_options.join(" ")} --restart" | ||
end | ||
|
||
def early_hints | ||
options[:early_hints] | ||
end | ||
|
||
def pid | ||
File.expand_path(options[:pid]) | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could avoid calling doing the same lookup twice:
or
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a hash lookup. I'm sure it'll be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've considered this but agree with @matthewd.
Drying things up isn't a benefit if the code is harder to read and understand and the performance impact of calling a hash lookup twice will be negligible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just curious about the performance impact, so I did a quick benchmarking.
TL;DR
Details
Click to show code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, would have been nice to see how the
&.
syntax compares.