feat(common): Allow ngSrc to be changed post-init (#50683)

Remove thrown error when ngSrc is modified after an NgOptimizedImage image is initialized

PR Close #50683

Author: atcastle

goldens/public-api/common/errors.md

@@ -15,6 +15,8 @@ export const enum RuntimeErrorCode {
     // (undocumented)
     LCP_IMG_MISSING_PRIORITY = 2955,
     // (undocumented)
+    LCP_IMG_NGSRC_MODIFIED = 2964,
+    // (undocumented)
     MISSING_BUILTIN_LOADER = 2962,
     // (undocumented)
     MISSING_NECESSARY_LOADER = 2963,

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

@@ -15,6 +15,13 @@ import {assertDevMode} from './asserts';
 import {imgDirectiveDetails} from './error_helper';
 import {getUrl} from './url';
 
+interface ObservedImageState {
+  priority: boolean;
+  modified: boolean;
+  alreadyWarnedPriority: boolean;
+  alreadyWarnedModified: boolean;
+}
+
 /**
  * Observer that detects whether an image with `NgOptimizedImage`
  * is treated as a Largest Contentful Paint (LCP) element. If so,
@@ -28,9 +35,7 @@ import {getUrl} from './url';
 @Injectable({providedIn: 'root'})
 export class LCPImageObserver implements OnDestroy {
   // Map of full image URLs -> original `ngSrc` values.
-  private images = new Map<string, string>();
-  // Keep track of images for which `console.warn` was produced.
-  private alreadyWarned = new Set<string>();
+  private images = new Map<string, ObservedImageState>();
 
   private window: Window|null = null;
   private observer: PerformanceObserver|null = null;
@@ -65,31 +70,51 @@ export class LCPImageObserver implements OnDestroy {
       // Exclude `data:` and `blob:` URLs, since they are not supported by the directive.
       if (imgSrc.startsWith('data:') || imgSrc.startsWith('blob:')) return;
 
-      const imgNgSrc = this.images.get(imgSrc);
-      if (imgNgSrc && !this.alreadyWarned.has(imgSrc)) {
-        this.alreadyWarned.add(imgSrc);
+      const img = this.images.get(imgSrc);
+      if (!img) return;
+      if (!img.priority && !img.alreadyWarnedPriority) {
+        img.alreadyWarnedPriority = true;
         logMissingPriorityWarning(imgSrc);
       }
+      if (img.modified && !img.alreadyWarnedModified) {
+        img.alreadyWarnedModified = true;
+        logModifiedWarning(imgSrc);
+      }
     });
     observer.observe({type: 'largest-contentful-paint', buffered: true});
     return observer;
   }
 
-  registerImage(rewrittenSrc: string, originalNgSrc: string) {
+  registerImage(rewrittenSrc: string, originalNgSrc: string, isPriority: boolean) {
     if (!this.observer) return;
-    this.images.set(getUrl(rewrittenSrc, this.window!).href, originalNgSrc);
+    const newObservedImageState: ObservedImageState = {
+      priority: isPriority,
+      modified: false,
+      alreadyWarnedModified: false,
+      alreadyWarnedPriority: false
+    };
+    this.images.set(getUrl(rewrittenSrc, this.window!).href, newObservedImageState);
   }
 
   unregisterImage(rewrittenSrc: string) {
     if (!this.observer) return;
     this.images.delete(getUrl(rewrittenSrc, this.window!).href);
   }
 
+  updateImage(originalSrc: string, newSrc: string) {
+    const originalUrl = getUrl(originalSrc, this.window!).href;
+    const img = this.images.get(originalUrl);
+    if (img) {
+      img.modified = true;
+      this.images.set(getUrl(newSrc, this.window!).href, img);
+      this.images.delete(originalUrl);
+    }
+  }
+
   ngOnDestroy() {
     if (!this.observer) return;
     this.observer.disconnect();
     this.images.clear();
-    this.alreadyWarned.clear();
   }
 }
 
@@ -102,3 +127,13 @@ function logMissingPriorityWarning(ngSrc: string) {
           `"priority" in order to prioritize its loading. ` +
           `To fix this, add the "priority" attribute.`));
 }
+
+function logModifiedWarning(ngSrc: string) {
+  const directiveDetails = imgDirectiveDetails(ngSrc);
+  console.warn(formatRuntimeError(
+      RuntimeErrorCode.LCP_IMG_NGSRC_MODIFIED,
+      `${directiveDetails} this image is the Largest Contentful Paint (LCP) ` +
+          `element and has had its "ngSrc" attribute modified. This can cause ` +
+          `slower loading performance. It is recommended not to modify the "ngSrc" ` +
+          `property on any image which could be the LCP element.`));
+}

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

@@ -362,18 +362,17 @@ export class NgOptimizedImage implements OnInit, OnChanges, OnDestroy {
       assertNotMissingBuiltInLoader(this.ngSrc, this.imageLoader);
       assertNoNgSrcsetWithoutLoader(this, this.imageLoader);
       assertNoLoaderParamsWithoutLoader(this, this.imageLoader);
+
+      if (this.lcpObserver !== null) {
+        const ngZone = this.injector.get(NgZone);
+        ngZone.runOutsideAngular(() => {
+          this.lcpObserver!.registerImage(this.getRewrittenSrc(), this.ngSrc, this.priority);
+        });
+      }
+
       if (this.priority) {
         const checker = this.injector.get(PreconnectLinkChecker);
         checker.assertPreconnect(this.getRewrittenSrc(), this.ngSrc);
-      } else {
-        // Monitor whether an image is an LCP element only in case
-        // the `priority` attribute is missing. Otherwise, an image
-        // has the necessary settings and no extra checks are required.
-        if (this.lcpObserver !== null) {
-          ngZone.runOutsideAngular(() => {
-            this.lcpObserver!.registerImage(this.getRewrittenSrc(), this.ngSrc);
-          });
-        }
       }
     }
     this.setHostAttributes();
@@ -400,36 +399,21 @@ export class NgOptimizedImage implements OnInit, OnChanges, OnDestroy {
 
     // The `src` and `srcset` attributes should be set last since other attributes
     // could affect the image's loading behavior.
-    const rewrittenSrc = this.getRewrittenSrc();
-    this.setHostAttribute('src', rewrittenSrc);
-
-    let rewrittenSrcset: string|undefined = undefined;
+    const rewrittenSrcset = this.updateSrcAndSrcset();
 
     if (this.sizes) {
       this.setHostAttribute('sizes', this.sizes);
     }
-
-    if (this.ngSrcset) {
-      rewrittenSrcset = this.getRewrittenSrcset();
-    } else if (this.shouldGenerateAutomaticSrcset()) {
-      rewrittenSrcset = this.getAutomaticSrcset();
-    }
-
-    if (rewrittenSrcset) {
-      this.setHostAttribute('srcset', rewrittenSrcset);
-    }
-
     if (this.isServer && this.priority) {
       this.preloadLinkCreator.createPreloadLinkTag(
-          this.renderer, rewrittenSrc, rewrittenSrcset, this.sizes);
+          this.renderer, this.getRewrittenSrc(), rewrittenSrcset, this.sizes);
     }
   }
 
   /** @nodoc */
   ngOnChanges(changes: SimpleChanges) {
     if (ngDevMode) {
       assertNoPostInitInputChange(this, changes, [
-        'ngSrc',
         'ngSrcset',
         'width',
         'height',
@@ -441,6 +425,17 @@ export class NgOptimizedImage implements OnInit, OnChanges, OnDestroy {
         'disableOptimizedSrcset',
       ]);
     }
+    if (changes['ngSrc'] && !changes['ngSrc'].isFirstChange()) {
+      const oldSrc = this._renderedSrc;
+      this.updateSrcAndSrcset(true);
+      const newSrc = this._renderedSrc;
+      if (this.lcpObserver !== null && oldSrc && newSrc && oldSrc !== newSrc) {
+        const ngZone = this.injector.get(NgZone);
+        ngZone.runOutsideAngular(() => {
+          this.lcpObserver?.updateImage(oldSrc, newSrc);
+        });
+      }
+    }
   }
 
   private callImageLoader(configWithoutCustomParams: Omit<ImageLoaderConfig, 'loaderParams'>):
@@ -508,6 +503,29 @@ export class NgOptimizedImage implements OnInit, OnChanges, OnDestroy {
     return finalSrcs.join(', ');
   }
 
+  private updateSrcAndSrcset(forceSrcRecalc = false): string|undefined {
+    if (forceSrcRecalc) {
+      // Reset cached value, so that the followup `getRewrittenSrc()` call
+      // will recalculate it and update the cache.
+      this._renderedSrc = null;
+    }
+
+    const rewrittenSrc = this.getRewrittenSrc();
+    this.setHostAttribute('src', rewrittenSrc);
+
+    let rewrittenSrcset: string|undefined = undefined;
+    if (this.ngSrcset) {
+      rewrittenSrcset = this.getRewrittenSrcset();
+    } else if (this.shouldGenerateAutomaticSrcset()) {
+      rewrittenSrcset = this.getAutomaticSrcset();
+    }
+
+    if (rewrittenSrcset) {
+      this.setHostAttribute('srcset', rewrittenSrcset);
+    }
+    return rewrittenSrcset;
+  }
+
   private getFixedSrcset(): string {
     const finalSrcs = DENSITY_SRCSET_MULTIPLIERS.map(multiplier => `${this.callImageLoader({
                                                        src: this.ngSrc,

packages/common/src/errors.ts

@@ -36,4 +36,5 @@ export const enum RuntimeErrorCode {
   TOO_MANY_PRELOADED_IMAGES = 2961,
   MISSING_BUILTIN_LOADER = 2962,
   MISSING_NECESSARY_LOADER = 2963,
+  LCP_IMG_NGSRC_MODIFIED = 2964,
 }

packages/common/test/directives/ng_optimized_image_spec.ts

@@ -642,9 +642,8 @@ describe('Image directive', () => {
     });
 
     const inputs = [
-      ['ngSrc', 'new-img.png'], ['width', 10], ['height', 20], ['priority', true], ['fill', true],
-      ['loading', true], ['sizes', '90vw'], ['disableOptimizedSrcset', true],
-      ['loaderParams', '{foo: "test1"}']
+      ['width', 10], ['height', 20], ['priority', true], ['fill', true], ['loading', true],
+      ['sizes', '90vw'], ['disableOptimizedSrcset', true], ['loaderParams', '{foo: "test1"}']
     ];
     inputs.forEach(([inputName, value]) => {
       it(`should throw if the \`${inputName}\` input changed after directive initialized the input`,
@@ -692,6 +691,35 @@ describe('Image directive', () => {
            }).toThrowError(new RegExp(expectedErrorMessage));
          });
     });
+    it(`should not throw if ngSrc changed after directive is initialized`, () => {
+      @Component({
+        selector: 'test-cmp',
+        template: `<img
+              [ngSrc]="ngSrc"
+              [width]="width"
+              [height]="height"
+              [loading]="loading"
+              [sizes]="sizes"
+            >`
+      })
+      class TestComponent {
+        width = 100;
+        height = 50;
+        ngSrc = 'img.png';
+        loading = false;
+        sizes = '100vw';
+      }
+
+      setupTestingModule({component: TestComponent});
+
+      // Initial render
+      const fixture = TestBed.createComponent(TestComponent);
+      fixture.detectChanges();
+      expect(() => {
+        fixture.componentInstance.ngSrc = 'newImg.png';
+        fixture.detectChanges();
+      }).not.toThrowError(new RegExp('was updated after initialization'));
+    });
   });
 
   describe('lazy loading', () => {
@@ -1259,6 +1287,75 @@ describe('Image directive', () => {
          expect(imgs[1].src.trim()).toBe(`${IMG_BASE_URL}/img-2.png`);
        });
 
+    it('should use the image loader to update `src` if `ngSrc` updated', () => {
+      @Component({
+        selector: 'test-cmp',
+        template: `<img
+           [ngSrc]="ngSrc"
+           width="300"
+           height="300"
+         >`
+      })
+      class TestComponent {
+        ngSrc = `img.png`;
+      }
+      const imageLoader = (config: ImageLoaderConfig) => `${IMG_BASE_URL}/${config.src}`;
+      setupTestingModule({imageLoader, component: TestComponent});
+      const fixture = TestBed.createComponent(TestComponent);
+      fixture.detectChanges();
+
+      let nativeElement = fixture.nativeElement as HTMLElement;
+      let imgs = nativeElement.querySelectorAll('img')!;
+      expect(imgs[0].src).toBe(`${IMG_BASE_URL}/img.png`);
+
+      fixture.componentInstance.ngSrc = 'updatedImg.png';
+      fixture.detectChanges();
+      expect(imgs[0].src).toBe(`${IMG_BASE_URL}/updatedImg.png`);
+    });
+
+    it('should use the image loader to update `srcset` if `ngSrc` updated', () => {
+      @Component({
+        selector: 'test-cmp',
+        template: `<img
+           [ngSrc]="ngSrc"
+           width="300"
+           height="300"
+           sizes="100vw"
+         >`
+      })
+      class TestComponent {
+        ngSrc = `img.png`;
+      }
+      const imageLoader = (config: ImageLoaderConfig) => {
+        const width = config.width ? `?w=${config.width}` : ``;
+        return `${IMG_BASE_URL}/${config.src}${width}`;
+      };
+      setupTestingModule({imageLoader, component: TestComponent});
+      const fixture = TestBed.createComponent(TestComponent);
+      fixture.detectChanges();
+
+      let nativeElement = fixture.nativeElement as HTMLElement;
+      let imgs = nativeElement.querySelectorAll('img')!;
+      expect(imgs[0].getAttribute('srcset'))
+          .toBe(`${IMG_BASE_URL}/img.png?w=640 640w, ${IMG_BASE_URL}/img.png?w=750 750w, ${
+              IMG_BASE_URL}/img.png?w=828 828w, ${IMG_BASE_URL}/img.png?w=1080 1080w, ${
+              IMG_BASE_URL}/img.png?w=1200 1200w, ${IMG_BASE_URL}/img.png?w=1920 1920w, ${
+              IMG_BASE_URL}/img.png?w=2048 2048w, ${IMG_BASE_URL}/img.png?w=3840 3840w`);
+
+      fixture.componentInstance.ngSrc = 'updatedImg.png';
+      nativeElement = fixture.nativeElement as HTMLElement;
+      imgs = nativeElement.querySelectorAll('img')!;
+      fixture.detectChanges();
+      expect(imgs[0].getAttribute('srcset'))
+          .toBe(`${IMG_BASE_URL}/updatedImg.png?w=640 640w, ${
+              IMG_BASE_URL}/updatedImg.png?w=750 750w, ${IMG_BASE_URL}/updatedImg.png?w=828 828w, ${
+              IMG_BASE_URL}/updatedImg.png?w=1080 1080w, ${
+              IMG_BASE_URL}/updatedImg.png?w=1200 1200w, ${
+              IMG_BASE_URL}/updatedImg.png?w=1920 1920w, ${
+              IMG_BASE_URL}/updatedImg.png?w=2048 2048w, ${
+              IMG_BASE_URL}/updatedImg.png?w=3840 3840w`);
+    });
+
     it('should pass absolute URLs defined in the `ngSrc` to custom image loaders provided via the `IMAGE_LOADER` token',
        () => {
          const imageLoader = (config: ImageLoaderConfig) => `${config.src}?rewritten=true`;

packages/core/test/bundling/image-directive/e2e/lcp-check/lcp-check.e2e-spec.ts

@@ -15,22 +15,24 @@ import {collectBrowserLogs} from '../browser-logs-util';
 describe('NgOptimizedImage directive', () => {
   it('should log a warning when a `priority` is missing on an LCP image', async () => {
     await browser.get('/e2e/lcp-check');
-
+    // Wait for ngSrc to be modified
+    await new Promise(resolve => setTimeout(resolve, 600));
     // Verify that both images were rendered.
     const imgs = element.all(by.css('img'));
     let srcB = await imgs.get(0).getAttribute('src');
     expect(srcB.endsWith('b.png')).toBe(true);
     const srcA = await imgs.get(1).getAttribute('src');
-    expect(srcA.endsWith('a.png')).toBe(true);
+    expect(srcA.endsWith('logo-500w.jpg')).toBe(true);
     // The `b.png` image is used twice in a template.
     srcB = await imgs.get(2).getAttribute('src');
     expect(srcB.endsWith('b.png')).toBe(true);
 
     // Make sure that only one warning is in the console for image `a.png`,
     // since the `b.png` should be below the fold and not treated as an LCP element.
     const logs = await collectBrowserLogs(logging.Level.WARNING);
-    expect(logs.length).toEqual(1);
+    expect(logs.length).toEqual(2);
     // Verify that the error code and the image src are present in the error message.
     expect(logs[0].message).toMatch(/NG02955.*?a\.png/);
+    expect(logs[1].message).toMatch(/NG02964.*?logo-500w\.jpg/);
   });
 });

packages/core/test/bundling/image-directive/e2e/lcp-check/lcp-check.ts

@@ -23,7 +23,7 @@ import {Component} from '@angular/core';
     <br>
 
     <!-- 'a.png' should be treated as an LCP element -->
-    <img ngSrc="/e2e/a.png" width="2500" height="2500">
+    <img [ngSrc]=imageSrc width="2500" height="2500">
 
     <br>
 
@@ -35,4 +35,11 @@ import {Component} from '@angular/core';
   `,
 })
 export class LcpCheckComponent {
+  imageSrc = '/e2e/a.png';
+
+  ngOnInit() {
+    setTimeout(() => {
+      this.imageSrc = '/e2e/logo-500w.jpg';
+    }, 500);
+  }
 }