Add uncapped preload headers for style, script assets #11504

aduth · 2024-11-13T22:03:30Z

🛠 Summary of changes

Updates asset helpers to bypass internal stylesheet_link_tag and javascript_include_tag logic for handling preload response headers, appending preload headers without caps. As of rails/rails#48405 (Rails 7.1), preload headers added by tag helpers are capped at 1kb, which is very limiting, especially given our approach of loading many, smaller assets targeted as sidecar assets for UI components.

While the caps have created some incentives to be more intentional with preloading (e.g. #10612), we've already addressed the lowest hanging fruit, and a number of critical scripts are currently not being preloaded (e.g. reCAPTCHA behaviors on the sign-in screen).

References:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Link

📜 Testing Plan

Verify that preload headers are included for all scripts and assets, including integrity or crossorigin directives as appropriate. Compare against https://secure.login.gov

curl -I http://localhost:3000

Verify that there are no style or script regressions in browsing the application, either in local development or in a review application.

app/services/asset_preload_linker.rb

zachmargolis · 2024-11-13T22:41:03Z

app/helpers/script_helper.rb

@@ -16,12 +16,19 @@ def render_javascript_pack_once_tags(...)
      concat javascript_assets_tag
      @scripts.each do |name, (url_params, attributes)|
        asset_sources.get_sources(name).each do |source|
+          crossorigin = true if local_crossorigin_sources?


we should be able to directly assign this right? I'd go a step further an inline the entire thing, but that would make us have to wrap line 23 below

Suggested change

crossorigin = true if local_crossorigin_sources?

crossorigin = local_crossorigin_sources?

This branch was actually a few months old, and that change was one of the first things I tried editing when I resurrected it. But there's a significant difference between nil and false in how it's used with javascript_include_tag, which is the reason for the true if. I can't recall off the top of my head, but I'll make sure to add some test coverage for it when I write those.

ah looks like the source code checks for "not nil" and "true"

maybe it would be easier if we updated local_crossorigin_sources? to match that expected format?

# Fake boolean to match expected values for `:crossorigin` option in `javascript_include_tag` # @see ActionView::Helpers::AssetTagHelper#javascript_include_tag # @return [true, nil] def crossorigin true if Rails.env.development? && ENV['WEBPACK_PORT'].present? end

I tinkered with this a bit, and landed toward a small revision to use presence in 4e23c1e.

Partly for micro-optimization, since the value is referenced twice and would prefer to avoid evaluating that logic twice. But moreso because the true / nil vs. true / false is less to do with some internal logic specific to javascript_include_tag, and moreso about the crossorigin attribute itself, where false would apply an empty string attribute value, which is significant since it's an HTML boolean attribute.

zachmargolis · 2024-11-13T22:43:04Z

app/helpers/script_helper.rb

+          crossorigin = true if local_crossorigin_sources?
+          integrity = asset_sources.get_integrity(source)
+
+          if attributes[:preload_links_header] != false


this means that preload_links_header: nil will add the preload header, that seems counterintuitive to me?

this means that preload_links_header: nil will add the preload header, that seems counterintuitive to me?

That would be the same behavior as the default javascript_include_tag behavior [1] [2], though technically it's configurable and respects the configured default.

shoudl we use the same presence trick?

Suggested change

if attributes[:preload_links_header] != false

if attributes[:preload_links_header].present?

I think that'd be the opposite behavior from what we expect, since we want :preload_links_header to be opt-out, not opt-in.

changelog: Internal, Performance, Add preload headers for all style, script assets

aduth · 2024-11-20T20:44:43Z

spec/helpers/script_helper_spec.rb

-          '</application.js>; rel=preload; as=script,' \
-            '</document-capture.js>; rel=preload; as=script',
+          '</application.js>;rel=preload;as=script,' \
+            '</document-capture.js>;rel=preload;as=script',


Whitespace is optional per the spec, which allows us to salvage back some of the bytesize increase these changes could incur.

link-value = "<" URI-Reference ">" *( OWS ";" OWS link-param )

https://httpwg.org/specs/rfc8288.html#header

("OWS" being defined as "optional whitespace")

See: https://github.com/18F/identity-idp/pull/11504/files#r1841273216 Co-authored-by: Zach Margolis <[email protected]>

We don't need any of this anymore: - Asset pipeline lookup - Preload headers handling - Server push - Multiple source concatenation - Nonce handling crossorigin as a boolean attribute is same as crossorigin=anonymous

Previously evaluated per script, even though the result would always be the same

aduth · 2024-11-21T14:38:34Z

I did some local benchmarking and pushed a few more iterations for some small optimizations. The end result is pretty nice, about 75% improvement.

$ RAILS_ENV=production rails runner benchmark.rb # production to cache AssetSources
Warming up --------------------------------------
              Before     3.005k i/100ms
               After     5.416k i/100ms
Calculating -------------------------------------
              Before     29.963k (± 2.0%) i/s -    150.250k in   5.016574s
               After     52.449k (± 7.4%) i/s -    265.384k in   5.099126s

Comparison:
               After:    52449.4 i/s
              Before:    29963.2 i/s - 1.75x  slower

(Benchmark script as HTML comment of this message)

zachmargolis · 2024-11-21T17:06:27Z

app/helpers/script_helper.rb

+          crossorigin = true if local_crossorigin_sources?
+          integrity = asset_sources.get_integrity(source)
+
+          if attributes[:preload_links_header] != false


shoudl we use the same presence trick?

Suggested change

if attributes[:preload_links_header] != false

if attributes[:preload_links_header].present?

* Add unlimited preload headers for style, script assets changelog: Internal, Performance, Add preload headers for all style, script assets * Update spec assertions for dropped whitespace * Add specs * Append with string mutation See: https://github.com/18F/identity-idp/pull/11504/files#r1841273216 Co-authored-by: Zach Margolis <[email protected]> * Use plain tag helper for generating script tag We don't need any of this anymore: - Asset pipeline lookup - Preload headers handling - Server push - Multiple source concatenation - Nonce handling crossorigin as a boolean attribute is same as crossorigin=anonymous * Add spec for preload_links_header attribute handling * Evaluate crossorigin once Previously evaluated per script, even though the result would always be the same * Micro-optimize append * Lowercase header key * Use presence to toggle between true/nil * Sync specs to use lowercase link key --------- Co-authored-by: Zach Margolis <[email protected]>

zachmargolis reviewed Nov 13, 2024

View reviewed changes

app/services/asset_preload_linker.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Nov 13, 2024

View reviewed changes

aduth added 2 commits November 20, 2024 15:41

Add unlimited preload headers for style, script assets

7f3b711

changelog: Internal, Performance, Add preload headers for all style, script assets

Update spec assertions for dropped whitespace

8183cb1

aduth force-pushed the aduth-preload-headers branch from ea5090e to 8183cb1 Compare November 20, 2024 20:43

aduth commented Nov 20, 2024

View reviewed changes

aduth and others added 8 commits November 20, 2024 15:53

Add specs

1a66ebd

Append with string mutation

2db75bc

See: https://github.com/18F/identity-idp/pull/11504/files#r1841273216 Co-authored-by: Zach Margolis <[email protected]>

Use plain tag helper for generating script tag

56d07c0

We don't need any of this anymore: - Asset pipeline lookup - Preload headers handling - Server push - Multiple source concatenation - Nonce handling crossorigin as a boolean attribute is same as crossorigin=anonymous

Add spec for preload_links_header attribute handling

5a5d896

Evaluate crossorigin once

fc534f9

Previously evaluated per script, even though the result would always be the same

Micro-optimize append

f45ce10

Lowercase header key

5e44cf5

Use presence to toggle between true/nil

4e23c1e

aduth marked this pull request as ready for review November 21, 2024 14:38

Sync specs to use lowercase link key

3a0ecf8

aduth added the performance label Nov 21, 2024

zachmargolis approved these changes Nov 21, 2024

View reviewed changes

aduth merged commit 0ad50f9 into main Nov 25, 2024
2 checks passed

aduth deleted the aduth-preload-headers branch November 25, 2024 12:59

amirbey mentioned this pull request Nov 26, 2024

Deploy RC 434 to Production #11562

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add uncapped preload headers for style, script assets #11504

Add uncapped preload headers for style, script assets #11504

aduth commented Nov 13, 2024 •

edited

Loading

zachmargolis Nov 13, 2024

aduth Nov 14, 2024 •

edited

Loading

zachmargolis Nov 14, 2024

aduth Nov 21, 2024

zachmargolis Nov 13, 2024

aduth Nov 14, 2024

zachmargolis Nov 21, 2024

aduth Nov 21, 2024

aduth Nov 20, 2024

aduth commented Nov 21, 2024

zachmargolis Nov 21, 2024

	crossorigin = true if local_crossorigin_sources?
	crossorigin = local_crossorigin_sources?

	if attributes[:preload_links_header] != false
	if attributes[:preload_links_header].present?

Add uncapped preload headers for style, script assets #11504

Add uncapped preload headers for style, script assets #11504

Conversation

aduth commented Nov 13, 2024 • edited Loading

🛠 Summary of changes

📜 Testing Plan

Choose a reason for hiding this comment

aduth Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Nov 21, 2024

Choose a reason for hiding this comment

aduth commented Nov 13, 2024 •

edited

Loading

aduth Nov 14, 2024 •

edited

Loading