feat(common): add <link> preload tag on server for priority img #47343

yharaskrik · 2022-09-04T23:57:41Z

While in Angular Universal, for images that are priority add a preload tag to the to ensure the image is preloaded before it is rendered. This resolves a warning when running Lighthouse.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Currently when using the NgOptimizedImage directive on <img> tags with the priority attribute (as suggested in the docs), Lighthouse will show a warning about preloading some assets to improve LCP metrics.

More here: https://web.dev/preload-critical-assets/#effects-of-preloading-on-core-web-vitals

Issue Number: N/A

What is the new behavior?

While rendering a page in Angular Universal if an image is going to be loaded with priority="true" then add a preload tag to the so that when the browsers renders the static HTML (rendered from the server) there are preload links for the browser to start loading the LCP images ahead of time and thus satisfying Lighthouse.

Does this PR introduce a breaking change?

Yes
No

Other information

Investigation numbers here: https://twitter.com/JayCooperBell/status/1566231614849331200

kara

Thanks for the PR! In general, looks good, but needs a test

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

AndrewKushnir · 2022-09-26T20:01:57Z

@yharaskrik thanks again for working on this feature! We think it'd bring a lot of value in SSR scenarios and we'd like to merge it before the v15 feature freeze date (Oct 12). Please let us know if you may have a chance to look into this and/or you want us to help with some of the changes. If I understand correctly, the remaining items are:

addressing the feedback from Kara regarding the srcset support, see feat(common): add <link> preload tag on server for priority img #47343 (review)
drop the config-related changes (just use 5 as the default)

After that we can start the final code-review round, perform the necessary tests in Google's codebase and we should be ready for merge.

Thank you.

yharaskrik · 2022-09-26T20:13:37Z

@AndrewKushnir sounds good! Just replied to Kara for some additional clarification as unless I'm missing something I don't see what she is referring to currently in the code, but maybe I'm missing it!

I'll make the config related changes tonight hopefully!

AndrewKushnir · 2022-10-03T16:28:23Z

@yharaskrik just wanted to ask if you want us to help somehow with the changes from this PR and/or there are open questions we can help address.

yharaskrik · 2022-10-03T16:32:24Z

@yharaskrik just wanted to ask if you want us to help somehow with the changes from this PR and/or there are open questions we can help address.

Hey @AndrewKushnir! Sorry things got crazy this last week, I do have a question about @kara comment that I have been meaning to post, Kara you are totally right that is is preloading the wrong one, but I am not following how I am going to figure out what one to preload.

We can get the sizes and srcsest as an @Input as you described (we are already reading srcset), but that doesn't tell me what one the user will be loading. I am assuming we don't want to prelaod every size (kinda defeats the purpose) so how should I determine what size that the user is going to be loading, wouldn't it depend on the screensize and what srcset gets matched? Am I misunderstanding the sizes attribute?

Let me know what I am missing here thanks you two!

kara · 2022-10-03T18:26:45Z

@yharaskrik Oh, the whole point of imagesrcset and imagesizes is that you don't have to know which size is being loaded. The browser will determine that. imagesrcset should have the same value as srcset. imagesizes should have the same value as sizes. Just copy them over and the browser will preload the correct URL.

There's more context in the docs here:
https://web.dev/preload-responsive-images/#imagesrcset-and-imagesizes

yharaskrik · 2022-10-03T19:09:58Z

@yharaskrik Oh, the whole point of imagesrcset and imagesizes is that you don't have to know which size is being loaded. The browser will determine that. imagesrcset should have the same value as srcset. imagesizes should have the same value as sizes. Just copy them over and the browser will preload the correct URL.

There's more context in the docs here: https://web.dev/preload-responsive-images/#imagesrcset-and-imagesizes

OH those attributes are on the <link> tag! 🤦 That is why I was so confused.

AndrewKushnir · 2022-10-10T16:26:29Z

@yharaskrik the PR #47547 that adds sizes input as well has landed. Could you please rebase this PR (and resolve the merge conflicts), so that we can complete the final round of code reviews and proceed with further testing in Google's codebase? Thank you.

yharaskrik · 2022-10-10T16:48:10Z

@yharaskrik the PR #47547 that adds sizes input as well has landed. Could you please rebase this PR (and resolve the merge conflicts), so that we can complete the final round of code reviews and proceed with further testing in Google's codebase? Thank you.

There ya go sir.

yharaskrik · 2022-10-10T17:18:36Z

There seems to be some seemingly unrelated test failures now and I cannot for the life of me track them down. It's like there are some leaks between tests as some tests are getting entirely different values than are configured on the <img> tag.

Image directive loaders `ngSrcset` values should set the `srcset` using the `ngSrcset` value with width descriptors FAILED
Expected 'http://localhost:9877/img.png?w=100 1x, http://localhost:9877/img.png?w=200 2x' to be 'http://localhost:9877/img.png?w=100 100w, http://localhost:9877/img.png?w=200 200w'.`

Whereas the <img> tag in this test is configured like so:

const template = `
           <img ngSrc="img.png" ngSrcset="100w, 200w" width="100" height="50">
         `;

whereas the next test down is configured like so

const template = `
           <img ngSrc="img.png" ngSrcset="1x, 2x" width="100" height="50">
         `;

Any help would be appreciated!

AndrewKushnir · 2022-10-10T17:40:42Z

There seems to be some seemingly unrelated test failures now and I cannot for the life of me track them down. It's like there are some leaks between tests as some tests are getting entirely different values than are configured on the tag.

@yharaskrik thanks for highlighting this issue, I will look into this now and let you know.

This commit adds a logic that generates preload tags for priority images, when rendering happens on the server (e.g. Angular Universal).

AndrewKushnir · 2022-10-10T18:42:06Z

@yharaskrik it looks like there was an issue with the rebase and an extra condition that was added in the PR #47547 got inserted in a different place. I've added a fixup commit to resolve this: 2969b99

atcastle

This looks good to me--I checked where the merge conflicts were resolved with the automatic srcset PR and it seems like it'll work fine.

atcastle · 2022-10-10T19:14:43Z

packages/common/test/directives/ng_optimized_image_spec.ts

+      });
+
+      const template =
+          `<img ngSrc="${src}" width="150" height="50" priority sizes="10vw" ngSrcset="100w">`;


Let's also include a test with no ngSrcset to make sure that the link element is picking up the automatically-generated srcset

Ok I'll try and get to that today

@yharaskrik let's address this feedback in a followup PR (adding changes to this PR would reset approvals).

AndrewKushnir · 2022-10-10T19:22:10Z

Presubmit #2 (internal link).

AndrewKushnir

Reviewed-for: public-api

AndrewKushnir · 2022-10-10T20:17:17Z

@yharaskrik FYI, the current state of this PR is "ready for merge", I'm adding it to the merge queue. There is a feedback from @atcastle (see #47343 (comment)), which we can handle in a followup PR. Please do not push any additional changes for now (it'd reset all approvals). I will let you know once this PR is merged.

ngbot · 2022-10-10T20:17:35Z

I see that you just added the action: merge label, but the following checks are still failing:
status "pullapprove" is failing
2 pending code reviews

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

AndrewKushnir · 2022-10-10T20:18:17Z

Merge-assistance (cc @jessicajaniuk): this PR is ready for merge. Pawel's feedback has been addressed, but since he is OOO, we can't change the status.

yharaskrik · 2022-10-10T20:19:20Z

@yharaskrik FYI, the current state of this PR is "ready for merge", I'm adding it to the merge queue. There is a feedback from @atcastle (see #47343 (comment)), which we can handle in a followup PR. Please do not push any additional changes for now (it'd reset all approvals). I will let you know once this PR is merged.

Ok sounds good!

Feedback addressed and reviewer OOO

jessicajaniuk · 2022-10-10T20:48:12Z

This PR was merged into the repository by commit 75e6297.

AndrewKushnir · 2022-10-10T21:54:10Z

@yharaskrik just wanted to say thank you once again for all your work on this PR 👍
The PR is now merged and the change will be released as a part of the upcoming Angular v15.

yharaskrik · 2022-10-10T21:58:27Z

@yharaskrik just wanted to say thank you once again for all your work on this PR 👍
The PR is now merged and the change will be released as a part of the upcoming Angular v15.

Thank you for all the help and guidance! Hope I can contribute again soon! I'd be curious what the future plans of the image directive is as I think that's a great step forward! (I'm in the angular slack as well)

angular-automatic-lock-bot · 2022-11-10T16:06:32Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

pullapprove bot requested a review from alxhub September 4, 2022 23:57

yharaskrik force-pushed the jaybell/create-link-preload-tag-in-universal-for-priority-images branch from 593690d to e0dfadb Compare September 5, 2022 20:47

AndrewKushnir requested review from AndrewKushnir and removed request for alxhub September 6, 2022 04:00

AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package target: major This PR is targeted for the next major release common: image directive labels Sep 6, 2022

ngbot bot added this to the Backlog milestone Sep 6, 2022

kara suggested changes Sep 19, 2022

View reviewed changes

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts Outdated Show resolved Hide resolved

yharaskrik commented Sep 19, 2022

View reviewed changes

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts Show resolved Hide resolved

yharaskrik requested review from kara and removed request for AndrewKushnir September 19, 2022 20:05

pullapprove bot requested a review from pkozlowski-opensource September 19, 2022 20:05

yharaskrik force-pushed the jaybell/create-link-preload-tag-in-universal-for-priority-images branch from c717fb8 to 2951b45 Compare September 19, 2022 20:07

kara reviewed Sep 20, 2022

View reviewed changes

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts Outdated Show resolved Hide resolved

AndrewKushnir modified the milestones: Backlog, v15 feature freeze blockers Sep 29, 2022

yharaskrik force-pushed the jaybell/create-link-preload-tag-in-universal-for-priority-images branch from fce5fb8 to ea32daa Compare October 3, 2022 19:26

yharaskrik requested review from AndrewKushnir and kara and removed request for pkozlowski-opensource and AndrewKushnir October 3, 2022 19:26

angular-robot bot added the feature Issue that requests a new feature label Oct 3, 2022

yharaskrik force-pushed the jaybell/create-link-preload-tag-in-universal-for-priority-images branch from 8bf432e to b3576a8 Compare October 10, 2022 16:47

yharaskrik force-pushed the jaybell/create-link-preload-tag-in-universal-for-priority-images branch from b3576a8 to d0edffd Compare October 10, 2022 17:07

yharaskrik and others added 2 commits October 10, 2022 11:35

feat(common): add <link> preload tag on server for priority img

5047854

This commit adds a logic that generates preload tags for priority images, when rendering happens on the server (e.g. Angular Universal).

fixup! feat(common): add <link> preload tag on server for priority img

2969b99

AndrewKushnir force-pushed the jaybell/create-link-preload-tag-in-universal-for-priority-images branch from d0edffd to 2969b99 Compare October 10, 2022 18:40

atcastle approved these changes Oct 10, 2022

View reviewed changes

AndrewKushnir approved these changes Oct 10, 2022

View reviewed changes

AndrewKushnir requested a review from pkozlowski-opensource October 10, 2022 20:15

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Oct 10, 2022

AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Oct 10, 2022

jessicajaniuk removed the request for review from pkozlowski-opensource October 10, 2022 20:46

jessicajaniuk removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Oct 10, 2022

jessicajaniuk closed this in 75e6297 Oct 10, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(common): add <link> preload tag on server for priority img #47343

feat(common): add <link> preload tag on server for priority img #47343

yharaskrik commented Sep 4, 2022

kara left a comment

AndrewKushnir commented Sep 26, 2022

yharaskrik commented Sep 26, 2022

AndrewKushnir commented Oct 3, 2022

yharaskrik commented Oct 3, 2022

kara commented Oct 3, 2022 •

edited

Loading

yharaskrik commented Oct 3, 2022

AndrewKushnir commented Oct 10, 2022 •

edited

Loading

yharaskrik commented Oct 10, 2022

yharaskrik commented Oct 10, 2022 •

edited

Loading

AndrewKushnir commented Oct 10, 2022

AndrewKushnir commented Oct 10, 2022

atcastle left a comment

atcastle Oct 10, 2022

yharaskrik Oct 10, 2022

AndrewKushnir Oct 10, 2022

yharaskrik Oct 10, 2022

AndrewKushnir commented Oct 10, 2022

AndrewKushnir left a comment

AndrewKushnir commented Oct 10, 2022

ngbot bot commented Oct 10, 2022

AndrewKushnir commented Oct 10, 2022 •

edited

Loading

yharaskrik commented Oct 10, 2022

jessicajaniuk commented Oct 10, 2022

AndrewKushnir commented Oct 10, 2022

yharaskrik commented Oct 10, 2022

angular-automatic-lock-bot bot commented Nov 10, 2022

feat(common): add <link> preload tag on server for priority img #47343

feat(common): add <link> preload tag on server for priority img #47343

Conversation

yharaskrik commented Sep 4, 2022

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

kara left a comment

Choose a reason for hiding this comment

AndrewKushnir commented Sep 26, 2022

yharaskrik commented Sep 26, 2022

AndrewKushnir commented Oct 3, 2022

yharaskrik commented Oct 3, 2022

kara commented Oct 3, 2022 • edited Loading

yharaskrik commented Oct 3, 2022

AndrewKushnir commented Oct 10, 2022 • edited Loading

yharaskrik commented Oct 10, 2022

yharaskrik commented Oct 10, 2022 • edited Loading

AndrewKushnir commented Oct 10, 2022

AndrewKushnir commented Oct 10, 2022

atcastle left a comment

Choose a reason for hiding this comment

atcastle Oct 10, 2022

Choose a reason for hiding this comment

yharaskrik Oct 10, 2022

Choose a reason for hiding this comment

AndrewKushnir Oct 10, 2022

Choose a reason for hiding this comment

yharaskrik Oct 10, 2022

Choose a reason for hiding this comment

AndrewKushnir commented Oct 10, 2022

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir commented Oct 10, 2022

ngbot bot commented Oct 10, 2022

AndrewKushnir commented Oct 10, 2022 • edited Loading

yharaskrik commented Oct 10, 2022

jessicajaniuk commented Oct 10, 2022

AndrewKushnir commented Oct 10, 2022

yharaskrik commented Oct 10, 2022

angular-automatic-lock-bot bot commented Nov 10, 2022

kara commented Oct 3, 2022 •

edited

Loading

AndrewKushnir commented Oct 10, 2022 •

edited

Loading

yharaskrik commented Oct 10, 2022 •

edited

Loading

AndrewKushnir commented Oct 10, 2022 •

edited

Loading