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

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

Merged
merged 10 commits into from
Jul 8, 2024

Conversation

AhmarZaidi
Copy link
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
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
Copy link

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
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.

@westonruter
Copy link
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
- Deduplicate adjacent links
- Add media attributes
Copy link
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!

@AhmarZaidi
Copy link
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
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.

plugins/optimization-detective/tests/test-optimization.php Outdated Show resolved Hide resolved
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
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
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
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

@westonruter westonruter changed the title Add support for sending preload links via HTTP headers Jul 8, 2024
@westonruter westonruter merged commit 04d05d3 into WordPress:trunk Jul 8, 2024
16 checks passed
@westonruter
Copy link
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