Skip to content

Send preload links via HTTP Link headers in addition to LINK tags#1323

Merged
westonruter merged 10 commits into
WordPress:trunkfrom
AhmarZaidi:preload-links
Jul 8, 2024
Merged

Send preload links via HTTP Link headers in addition to LINK tags#1323
westonruter merged 10 commits into
WordPress:trunkfrom
AhmarZaidi:preload-links

Conversation

@AhmarZaidi
Copy link
Copy Markdown
Contributor

Summary

Fixes #1321

Relevant technical choices

  • Added a new method get_headers() to OD_Preload_Link_Collection to construct the Link HTTP response headers for preloading resources.
  • Updated od_optimize_template_output_buffer to call headers() if there are preload links available and headers have not been sent already.
  • Sent HTTP Link headers separately (above appending the link tags) to prevent Cannot modify header information - headers already sent error due to output before sending headers.
@AhmarZaidi
Copy link
Copy Markdown
Contributor Author

Please add type label and milestone.

@swissspidy swissspidy added the [Type] Enhancement A suggestion for improvement of an existing feature label Jun 29, 2024
@AhmarZaidi AhmarZaidi marked this pull request as ready for review July 1, 2024 05:53
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jul 1, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: AhmarZaidi <ahmarzaidi@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Copy Markdown
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Great start.

Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
Comment thread plugins/optimization-detective/class-od-preload-link-collection.php
Comment thread plugins/optimization-detective/optimization.php Outdated
Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
@westonruter
Copy link
Copy Markdown
Member

Oh, we'll also need to add tests for this new method.

- Remove code duplication
- Update escaping functions
- Update documentation
- Handle no headers case
Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
- Deduplicate adjacent links
- Add media attributes
Copy link
Copy Markdown
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting close!

Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
@AhmarZaidi
Copy link
Copy Markdown
Contributor Author

Oh, we'll also need to add tests for this new method.

Had some doubts regarding tests implementation:

  1. I believe get_html() is tested by using the output of od_optimize_template_output_buffer() since the walker adds links to the markup and compares it with the data from data provider.
    Now, how would I test for get_response_header() since they are added to the http response and not the markup.
    I explored xdebug_get_headers() but that doesn't seem to work. Could separately test get_response_header() function's output but I don't think that's the right way.

  2. If we implement it like this in optimization.php:

// Send any preload links in a Link response header and in a LINK tag injected at the end of the HEAD.
if ( count( $preload_links ) > 0 ) {
	if ( ! headers_sent() ) {
		header( $preload_links->get_response_header() );
	}
	$walker->append_head_html( $preload_links->get_html() );
}

Then headers() won't be called while testing because headers_sent() would be true due to prior output, so how would we test it?

- Update documentation
- Fix preload link without href using about:blank fallback
@westonruter
Copy link
Copy Markdown
Member

@AhmarZaidi Yeah, testing the output of header() is a pain. I don't think we can practically test it being output when calling od_optimize_template_output_buffer(). I think it's fine to just test that get_response_header() method in isolation.

Comment thread plugins/optimization-detective/tests/test-optimization.php Outdated
1200
);

$expected_header = 'Link: <https://example.com/foo.jpg>; rel="preload"; as="image"; fetchpriority="high"; imagesrcset="https%3A%2F%2Fexample.com%2Ffoo-480w.jpg%20480w%2C%20https%3A%2F%2Fexample.com%2Ffoo-800w.jpg%20800w"; imagesizes="%28max-width%3A%20600px%29%20480px%2C%20800px"; crossorigin="anonymous"; media="screen", <https://example.com/bar.jpg>; rel="preload"; as="image"; fetchpriority="high"; imagesrcset="https%3A%2F%2Fexample.com%2Fbar-480w.jpg%20480w%2C%20https%3A%2F%2Fexample.com%2Fbar-800w.jpg%20800w"; imagesizes="%28max-width%3A%20600px%29%20480px%2C%20800px"; crossorigin="anonymous"; media="screen%20and%20%28min-width%3A%20600px%29%20and%20%28max-width%3A%201200px%29"';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The imagesrcset, imagesizes, and media attribute values here seem wrong being URL-encoded. Do these work being encoded like this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I found it doesn't work, for imagesrcset at least. I tried sending that Link header on a Glitch and I saw a network request attempted for:

https://alert-sustaining-acrylic.glitch.me/https%3A%2F%2Fexample.com%2Ffoo-480w.jpg%20480w%2C%20https%3A%2F%2Fexample.com%2Ffoo-800w.jpg%20800w
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got this. See d1cd8d4

Comment thread plugins/optimization-detective/class-od-preload-link-collection.php Outdated
@westonruter westonruter changed the title Add support for sending preload links via HTTP headers Send preload links via HTTP Link headers in addition to LINK tags Jul 8, 2024
@westonruter westonruter merged commit 04d05d3 into WordPress:trunk Jul 8, 2024
@westonruter
Copy link
Copy Markdown
Member

Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Enhancement A suggestion for improvement of an existing feature

3 participants