The Wayback Machine - https://web.archive.org/web/20250519222246/https://github.com/angular/angular/pull/46053
Skip to content

feat(common): verify that priority images have corresponding preconnect links #46053

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 7, 2022
Merged

feat(common): verify that priority images have corresponding preconnect links #46053

merged 3 commits into from
Jun 7, 2022

Conversation

AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented May 19, 2022 •
edited
Loading

This commit updates the NgOptimizedImage directive to add a logic to detect whether an image, marked with the "priority" attribute has corresponding <link rel="preconnect"> tag in the document.head.

PR Type

What kind of change does this PR introduce?

  • Feature

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir 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 PullApprove: disable target: feature This PR is targeted for a feature branch (outside of main and semver branches) labels May 19, 2022
@ngbot ngbot bot added this to the Backlog milestone May 19, 2022
@pkozlowski-opensource
Copy link
Member

@AndrewKushnir would you mind rebasing this PR? Kind of hard to review it in the current state.

@AndrewKushnir
Copy link
Contributor Author

@pkozlowski-opensource the PR is now rebased, thanks for the comment.

@khempenius
Copy link
Contributor

Summarizing offline discussion:

  • I think it makes sense to hold off on checking for preload resource hints until Angular CLI can auto-insert them. Without automated tooling it could be difficult to ensure that the preload tags stay in sync with page content & if they were to get out of sync, this would be bad for performance.
  • We will still verify the presence of preconnect resource hints - however this can be opted out on a per-loader basis. For example, sites that use different origins to serve the main document vs. images during development - but serve both from the same origin during production, would probably want to use this opt-out.

@AndrewKushnir AndrewKushnir force-pushed the preload-check branch 2 times, most recently from a118df6 to c340be2 Compare June 3, 2022 05:24
@AndrewKushnir AndrewKushnir changed the title feat(common): verify that priority images have preload/preconnect links configured feat(common): verify that priority images have corresponding preconnect links Jun 3, 2022
AndrewKushnir
AndrewKushnir commented Jun 3, 2022
View reviewed changes
packages/private/testing/src/render3.ts Outdated
* @param blockFn function to wrap. The function can return promise or be `async`.
* @publicApi
*/
export function withHead<T extends Function>(html: string, blockFn: T): T {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to the reviewers: I will create a separate PR against the main Angular branches and land it. This PR would not contain this change, it's just there at this moment for completeness (to make things work).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR with those changes for the main Angular branches: #46250.

@AndrewKushnir AndrewKushnir mentioned this pull request Jun 3, 2022
Closed
3 tasks
kara
kara reviewed Jun 4, 2022
View reviewed changes
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

Thanks for making all the changes! A few minor things below

@AndrewKushnir
feat(common): verify that priority images have preconnect links
3968f0a 
This commit updates the `NgOptimizedImage` directive to add a logic to detect whether an image, marked with the "priority" attribute has a corresponding `<link rel="preconnect">` tag in the `document.head` for better performance.
@AndrewKushnir AndrewKushnir force-pushed the preload-check branch 2 times, most recently from adde7c5 to f77dee3 Compare June 7, 2022 00:38
@AndrewKushnir AndrewKushnir requested a review from kara June 7, 2022 00:40
@AndrewKushnir AndrewKushnir force-pushed the preload-check branch 2 times, most recently from 491a693 to e67331b Compare June 7, 2022 01:46
@AndrewKushnir
feat(common): provide an ability to exclude origins from preconnect c…
08b5322 
…hecks in NgOptimizedImage

This commit adds an extra logic to add an ability to exclude origins from preconnect checks in NgOptimizedImage by configuring the `PRECONNECT_CHECK_BLOCKLIST` multi-provider.
@AndrewKushnir
Copy link
Contributor Author

@kara @pkozlowski-opensource FYI this PR is updated and is ready for the final review. Thank you.

pkozlowski-opensource
pkozlowski-opensource reviewed Jun 7, 2022
View reviewed changes
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

@AndrewKushnir I did another pass on this PR and left a couple of nit / question comments but things LGTM overall.

The only open question I've got is the usage of a multiprovider for the preconnect blocklist. Since the application author / "integrator" is in charge of adding the procennect links it should be, IMO, an app responsibility to configure loaders, add preconnect tags etc.

In my head other modules (ex. 3rd party loaded from NPM) should have no say in this blocklist. Let's discuss in person as we've got a dedicated meeting today.

@AndrewKushnir
Copy link
Contributor Author

Quick update: after an offline discussion, the PRECONNECT_CHECK_BLOCKLIST token remains a multi-provider one to take into account a situation when loaders are provided on a component level (thus there is a potential for multiple host names that we'd need to check). The rest of the feedback is addressed in this fixup commit: d7a3b91. @pkozlowski-opensource could you please take another look?

pkozlowski-opensource
pkozlowski-opensource approved these changes Jun 7, 2022
View reviewed changes
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

LGTM

@AndrewKushnir AndrewKushnir merged commit 7e8be93 into angular:image-directive Jun 7, 2022
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource approved these changes

@kara kara Awaiting requested review from kara

Assignees
No one assigned
Labels
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 PullApprove: disable target: feature This PR is targeted for a feature branch (outside of main and semver branches)
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.
4 participants
@AndrewKushnir @pkozlowski-opensource @khempenius @kara