From d9fe17a3dfd95bc461f657528fcf17501bae8269 Mon Sep 17 00:00:00 2001 From: Matthieu Riegler Date: Sun, 30 Jun 2024 15:20:10 +0200 Subject: [PATCH] refactor(common): Fire priority check on stable. (#56776) To support routing on app init, the directive will now fire the priority check when the apps become stable. fixes #56757 PR Close #56776 --- .../ng_optimized_image/ng_optimized_image.ts | 45 +++++++++++-------- .../directives/ng_optimized_image_spec.ts | 22 +++++---- 2 files changed, 40 insertions(+), 27 deletions(-) diff --git a/packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts b/packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts index 34c9d8cebb7..025ddde05ae 100644 --- a/packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts +++ b/packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts @@ -30,6 +30,8 @@ import { ɵSafeValue as SafeValue, ɵunwrapSafeValue as unwrapSafeValue, ChangeDetectorRef, + ApplicationRef, + ɵwhenStable as whenStable, } from '@angular/core'; import {RuntimeErrorCode} from '../../errors'; @@ -155,6 +157,13 @@ const PRIORITY_COUNT_THRESHOLD = 10; */ let IMGS_WITH_PRIORITY_ATTR_COUNT = 0; +/** + * This function is for testing purpose. + */ +export function resetImagePriorityCount() { + IMGS_WITH_PRIORITY_ATTR_COUNT = 0; +} + /** * Config options used in rendering placeholder images. * @@ -449,12 +458,9 @@ export class NgOptimizedImage implements OnInit, OnChanges, OnDestroy { const checker = this.injector.get(PreconnectLinkChecker); checker.assertPreconnect(this.getRewrittenSrc(), this.ngSrc); - // This leaves the Angular zone to avoid triggering unnecessary change detection cycles when - // document.addEventListener is invoked if (!this.isServer) { - ngZone.runOutsideAngular(() => { - assetPriorityCountBelowThreshold(); - }); + const applicationRef = this.injector.get(ApplicationRef); + assetPriorityCountBelowThreshold(applicationRef); } } } @@ -1278,22 +1284,23 @@ function assertNoLoaderParamsWithoutLoader(dir: NgOptimizedImage, imageLoader: I /** * Warns if the priority attribute is used too often on page load */ -function assetPriorityCountBelowThreshold() { +async function assetPriorityCountBelowThreshold(appRef: ApplicationRef) { if (IMGS_WITH_PRIORITY_ATTR_COUNT === 0) { - document.addEventListener('DOMContentLoaded', () => { - if (IMGS_WITH_PRIORITY_ATTR_COUNT > PRIORITY_COUNT_THRESHOLD) { - console.warn( - formatRuntimeError( - RuntimeErrorCode.TOO_MANY_PRIORITY_ATTRIBUTES, - `NgOptimizedImage: The "priority" attribute is set to true more than ${PRIORITY_COUNT_THRESHOLD} times (${IMGS_WITH_PRIORITY_ATTR_COUNT} times). ` + - `Marking too many images as "high" priority can hurt your application's LCP (https://web.dev/lcp). ` + - `"Priority" should only be set on the image expected to be the page's LCP element.`, - ), - ); - } - }); + IMGS_WITH_PRIORITY_ATTR_COUNT++; + await whenStable(appRef); + if (IMGS_WITH_PRIORITY_ATTR_COUNT > PRIORITY_COUNT_THRESHOLD) { + console.warn( + formatRuntimeError( + RuntimeErrorCode.TOO_MANY_PRIORITY_ATTRIBUTES, + `NgOptimizedImage: The "priority" attribute is set to true more than ${PRIORITY_COUNT_THRESHOLD} times (${IMGS_WITH_PRIORITY_ATTR_COUNT} times). ` + + `Marking too many images as "high" priority can hurt your application's LCP (https://web.dev/lcp). ` + + `"Priority" should only be set on the image expected to be the page's LCP element.`, + ), + ); + } + } else { + IMGS_WITH_PRIORITY_ATTR_COUNT++; } - IMGS_WITH_PRIORITY_ATTR_COUNT++; } /** diff --git a/packages/common/test/directives/ng_optimized_image_spec.ts b/packages/common/test/directives/ng_optimized_image_spec.ts index 2fe6e7e1e3f..cac8ddb3180 100644 --- a/packages/common/test/directives/ng_optimized_image_spec.ts +++ b/packages/common/test/directives/ng_optimized_image_spec.ts @@ -30,6 +30,7 @@ import { NgOptimizedImage, PLACEHOLDER_BLUR_AMOUNT, RECOMMENDED_SRCSET_DENSITY_CAP, + resetImagePriorityCount, } from '../../src/directives/ng_optimized_image/ng_optimized_image'; import {PRECONNECT_CHECK_BLOCKLIST} from '../../src/directives/ng_optimized_image/preconnect_link_checker'; @@ -871,12 +872,16 @@ describe('Image directive', () => { expect(img.getAttribute('fetchpriority')).toBe('auto'); }); - it('should log a warning if the priority attribute is used too often', async () => { - withHead('', () => { + it( + 'should log a warning if the priority attribute is used too often', + withHead('', async () => { + // We need to reset the count as previous test might have incremented it already + resetImagePriorityCount(); + const imageLoader = () => { // We need something different from the `localhost` (as we don't want to produce // a preconnect warning for local environments). - return 'https://angular.io/assets/images/logos/angular/angular.svg'; + return 'https://angular.io/assets/images/logos/path/img.png'; }; setupTestingModule({imageLoader}); @@ -900,9 +905,10 @@ describe('Image directive', () => { const fixture = createTestComponent(template); fixture.detectChanges(); - // We manually fire the event that is listened by the directive - // as it won't fire in the context of our unit test - document.dispatchEvent(new Event('DOMContentLoaded')); + await fixture.whenStable(); + + // trick to wait for the whenStable() to fire in the directive + await Promise.resolve(); if (isBrowser) { expect(consoleWarnSpy.calls.count()).toBe(1); @@ -913,8 +919,8 @@ describe('Image directive', () => { // The warning is only logged on browsers expect(consoleWarnSpy.calls.count()).toBe(0); } - }); - }); + }), + ); }); describe('meta data', () => {