-
Notifications
You must be signed in to change notification settings - Fork 166
Add uncapped preload headers for style, script assets #11504
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
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
7f3b711
Add unlimited preload headers for style, script assets
aduth 8183cb1
Update spec assertions for dropped whitespace
aduth 1a66ebd
Add specs
aduth 2db75bc
Append with string mutation
aduth 56d07c0
Use plain tag helper for generating script tag
aduth 5a5d896
Add spec for preload_links_header attribute handling
aduth fc534f9
Evaluate crossorigin once
aduth f45ce10
Micro-optimize append
aduth 5e44cf5
Lowercase header key
aduth 4e23c1e
Use presence to toggle between true/nil
aduth 3a0ecf8
Sync specs to use lowercase link key
aduth File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| class AssetPreloadLinker | ||
| def self.append(headers:, as:, url:, crossorigin: false, integrity: nil) | ||
| header = +headers['link'].to_s | ||
| header << ',' if header != '' | ||
| header << "<#{url}>;rel=preload;as=#{as}" | ||
| header << ';crossorigin' if crossorigin | ||
| header << ";integrity=#{integrity}" if integrity | ||
| headers['link'] = header | ||
| end | ||
| end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,8 +82,8 @@ | |
| render_javascript_pack_once_tags | ||
|
|
||
| expect(response.headers['link']).to eq( | ||
| '</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', | ||
|
Comment on lines
-85
to
+86
Contributor
Author
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. Whitespace is optional per the spec, which allows us to salvage back some of the bytesize increase these changes could incur.
|
||
| ) | ||
| expect(response.headers['link']).to_not include('nopush') | ||
| end | ||
|
|
@@ -107,6 +107,18 @@ | |
| end | ||
| end | ||
|
|
||
| context 'with preload links header disabled' do | ||
| before do | ||
| javascript_packs_tag_once('application', preload_links_header: false) | ||
| end | ||
|
|
||
| it 'does not append preload header' do | ||
| render_javascript_pack_once_tags | ||
|
|
||
| expect(response.headers['link']).to eq('</document-capture.js>;rel=preload;as=script') | ||
| end | ||
| end | ||
|
|
||
| context 'with attributes' do | ||
| before do | ||
| javascript_packs_tag_once('track-errors', defer: true) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| require 'rails_helper' | ||
|
|
||
| RSpec.describe AssetPreloadLinker do | ||
| describe '.append' do | ||
| let(:link) { nil } | ||
| let(:as) { 'script' } | ||
| let(:url) { '/script.js' } | ||
| let(:crossorigin) { nil } | ||
| let(:integrity) { nil } | ||
| let(:headers) { { 'link' => link } } | ||
| subject(:result) do | ||
| AssetPreloadLinker.append(**{ headers:, as:, url:, crossorigin:, integrity: }.compact) | ||
| end | ||
|
|
||
| context 'with absent link value' do | ||
| let(:link) { nil } | ||
|
|
||
| it 'returns a string with only the appended link' do | ||
| expect(result).to eq('</script.js>;rel=preload;as=script') | ||
| end | ||
| end | ||
|
|
||
| context 'with empty link value' do | ||
| let(:link) { '' } | ||
|
|
||
| it 'returns a string with only the appended link' do | ||
| expect(result).to eq('</script.js>;rel=preload;as=script') | ||
| end | ||
| end | ||
|
|
||
| context 'with non-empty link value' do | ||
| let(:link) { '</a.js>;rel=preload;as=script' } | ||
|
|
||
| it 'returns a comma-separated link value of the new and existing link' do | ||
| expect(result).to eq('</a.js>;rel=preload;as=script,</script.js>;rel=preload;as=script') | ||
| end | ||
|
|
||
| context 'with existing link value as frozen string' do | ||
| let(:link) { '</a.js>;rel=preload;as=script'.freeze } | ||
|
|
||
| it 'returns a comma-separated link value of the new and existing link' do | ||
| expect(result).to eq('</a.js>;rel=preload;as=script,</script.js>;rel=preload;as=script') | ||
| end | ||
| end | ||
| end | ||
|
|
||
| context 'with crossorigin option' do | ||
| let(:crossorigin) { true } | ||
|
|
||
| it 'includes crossorigin link param' do | ||
| expect(result).to eq('</script.js>;rel=preload;as=script;crossorigin') | ||
| end | ||
| end | ||
|
|
||
| context 'with integrity option' do | ||
| let(:integrity) { 'abc123' } | ||
|
|
||
| it 'includes integrity link param' do | ||
| expect(result).to eq('</script.js>;rel=preload;as=script;integrity=abc123') | ||
| end | ||
| end | ||
| end | ||
| end |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
this means that
preload_links_header: nilwill add the preload header, that seems counterintuitive to me?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.
That would be the same behavior as the default
javascript_include_tagbehavior [1] [2], though technically it's configurable and respects the configured default.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.
shoudl we use the same
presencetrick?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 think that'd be the opposite behavior from what we expect, since we want
:preload_links_headerto be opt-out, not opt-in.