fix(elements): update the view of an `OnPush` component when inputs change (#39452)

As with regular Angular components, Angular elements are expected to
have their views update when inputs change.

Previously, Angular Elements views were not updated if the underlying
component used the `OnPush` change detection strategy.

This commit fixes this by calling `markForCheck()` on the component
view's `ChangeDetectorRef`.

NOTE:
This is similar to how `@angular/upgrade` does it:
3236ae0ee1/packages/upgrade/src/common/src/downgrade_component_adapter.ts (L146).

Fixes #38948

PR Close #39452

goldens/size-tracking/aio-payloads.json

@@ -12,7 +12,7 @@
     "master": {
       "uncompressed": {
         "runtime-es2015": 3037,
-        "main-es2015": 448615,
+        "main-es2015": 448922,
         "polyfills-es2015": 52415
       }
     }
@@ -21,7 +21,7 @@
     "master": {
       "uncompressed": {
         "runtime-es2015": 3157,
-        "main-es2015": 431750,
+        "main-es2015": 432199,
         "polyfills-es2015": 52415
       }
     }

integration/ng_elements/e2e/app.e2e-spec.ts

@@ -5,7 +5,7 @@ describe('Element E2E Tests', function () {
   describe('Hello World Elements', () => {
     beforeEach(() => browser.get('hello-world.html'));
 
-    describe('(with default view encapsulation)', () => {
+    describe('(with default CD strategy and view encapsulation)', () => {
       const helloWorldEl = element(by.css('hello-world-el'));
 
       it('should display "Hello World!"', function () {
@@ -21,6 +21,22 @@ describe('Element E2E Tests', function () {
       });
     });
 
+    describe('(with `OnPush` CD strategy)', () => {
+      const helloWorldOnpushEl = element(by.css('hello-world-onpush-el'));
+
+      it('should display "Hello World!"', function () {
+        expect(helloWorldOnpushEl.getText()).toBe('Hello World!');
+      });
+
+      it('should display "Hello Foo!" via name attribute', function () {
+        const input = element(by.css('input[type=text]'));
+        input.sendKeys('Foo');
+
+        // Make tests less flaky on CI by waiting up to 5s for the element text to be updated.
+        browser.wait(EC.textToBePresentInElement(helloWorldOnpushEl, 'Hello Foo!'), 5000);
+      });
+    });
+
     describe('(with `ShadowDom` view encapsulation)', () => {
       const helloWorldShadowEl = element(by.css('hello-world-shadow-el'));
       const getShadowDomText = (el: ElementFinder) =>

integration/ng_elements/src/app.ts

@@ -2,17 +2,29 @@ import {Injector, NgModule} from '@angular/core';
 import {createCustomElement} from '@angular/elements';
 import {BrowserModule} from '@angular/platform-browser';
 
-import {HelloWorldComponent, HelloWorldShadowComponent, TestCardComponent} from './elements';
+import {HelloWorldComponent, HelloWorldOnpushComponent, HelloWorldShadowComponent, TestCardComponent} from './elements';
 
 
 @NgModule({
-  declarations: [HelloWorldComponent, HelloWorldShadowComponent, TestCardComponent],
-  entryComponents: [HelloWorldComponent, HelloWorldShadowComponent, TestCardComponent],
+  declarations: [
+    HelloWorldComponent,
+    HelloWorldOnpushComponent,
+    HelloWorldShadowComponent,
+    TestCardComponent,
+  ],
+  entryComponents: [
+    HelloWorldComponent,
+    HelloWorldOnpushComponent,
+    HelloWorldShadowComponent,
+    TestCardComponent,
+  ],
   imports: [BrowserModule],
 })
 export class AppModule {
   constructor(injector: Injector) {
     customElements.define('hello-world-el', createCustomElement(HelloWorldComponent, {injector}));
+    customElements.define(
+        'hello-world-onpush-el', createCustomElement(HelloWorldOnpushComponent, {injector}));
     customElements.define(
         'hello-world-shadow-el', createCustomElement(HelloWorldShadowComponent, {injector}));
     customElements.define('test-card', createCustomElement(TestCardComponent, {injector}));

integration/ng_elements/src/elements.ts

@@ -1,4 +1,4 @@
-import {Component, Input, ViewEncapsulation} from '@angular/core';
+import {ChangeDetectionStrategy, Component, Input, ViewEncapsulation} from '@angular/core';
 
 @Component({
   selector: 'hello-world-el',
@@ -8,6 +8,15 @@ export class HelloWorldComponent {
   @Input() name: string = 'World';
 }
 
+@Component({
+  selector: 'hello-world-onpush-el',
+  template: 'Hello {{name}}!',
+  changeDetection: ChangeDetectionStrategy.OnPush,
+})
+export class HelloWorldOnpushComponent {
+  @Input() name: string = 'World';
+}
+
 @Component({
   selector: 'hello-world-shadow-el',
   template: 'Hello {{name}}!',

integration/ng_elements/src/hello-world.html

@@ -10,6 +10,7 @@
 <body>
   <input type="text">
   <hello-world-el></hello-world-el>
+  <hello-world-onpush-el></hello-world-onpush-el>
   <hello-world-shadow-el></hello-world-shadow-el>
   <script src="dist/bundle.js"></script>
 </body>

integration/ng_elements/src/main.ts

@@ -5,9 +5,11 @@ platformBrowser().bootstrapModuleFactory(AppModuleNgFactory, {ngZone: 'noop'});
 
 const input = document.querySelector('input')!;
 const helloWorld = document.querySelector('hello-world-el')!;
+const helloWorldOnpush = document.querySelector('hello-world-onpush-el')!;
 const helloWorldShadow = document.querySelector('hello-world-shadow-el')!;
 
 input.addEventListener('input', () => {
   helloWorld.setAttribute('name', input.value);
+  helloWorldOnpush.setAttribute('name', input.value);
   helloWorldShadow.setAttribute('name', input.value);
 });

packages/elements/src/component-factory-strategy.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ApplicationRef, ComponentFactory, ComponentFactoryResolver, ComponentRef, EventEmitter, Injector, NgZone, OnChanges, SimpleChange, SimpleChanges, Type} from '@angular/core';
+import {ApplicationRef, ChangeDetectorRef, ComponentFactory, ComponentFactoryResolver, ComponentRef, EventEmitter, Injector, NgZone, OnChanges, SimpleChange, SimpleChanges, Type} from '@angular/core';
 import {merge, Observable, ReplaySubject} from 'rxjs';
 import {map, switchMap} from 'rxjs/operators';
 
@@ -52,10 +52,19 @@ export class ComponentNgElementStrategy implements NgElementStrategy {
   /** Reference to the component that was created on connect. */
   private componentRef: ComponentRef<any>|null = null;
 
-  /** Changes that have been made to the component ref since the last time onChanges was called. */
+  /** Reference to the component view's `ChangeDetectorRef`. */
+  private viewChangeDetectorRef: ChangeDetectorRef|null = null;
+
+  /**
+   * Changes that have been made to component inputs since the last change detection run.
+   * (NOTE: These are only recorded if the component implements the `OnChanges` interface.)
+   */
   private inputChanges: SimpleChanges|null = null;
 
-  /** Whether the created component implements the onChanges function. */
+  /** Whether changes have been made to component inputs since the last change detection run. */
+  private hasInputChanges = false;
+
+  /** Whether the created component implements the `OnChanges` interface. */
   private implementsOnChanges = false;
 
   /** Whether a change detection has been scheduled to run on the component. */
@@ -68,10 +77,12 @@ export class ComponentNgElementStrategy implements NgElementStrategy {
   private readonly initialInputValues = new Map<string, any>();
 
   /**
-   * Set of component inputs that have not yet changed, i.e. for which `ngOnChanges()` has not
-   * fired. (This is used to determine the value of `fistChange` in `SimpleChange` instances.)
+   * Set of component inputs that have not yet changed, i.e. for which `recordInputChange()` has not
+   * fired.
+   * (This helps detect the first change of an input, even if it is explicitly set to `undefined`.)
    */
-  private readonly unchangedInputs = new Set<string>();
+  private readonly unchangedInputs =
+      new Set<string>(this.componentFactory.inputs.map(({propName}) => propName));
 
   /** Service for setting zone context. */
   private readonly ngZone = this.injector.get<NgZone>(NgZone);
@@ -119,6 +130,7 @@ export class ComponentNgElementStrategy implements NgElementStrategy {
         if (this.componentRef !== null) {
           this.componentRef.destroy();
           this.componentRef = null;
+          this.viewChangeDetectorRef = null;
         }
       }, DESTROY_DELAY);
     });
@@ -157,7 +169,13 @@ export class ComponentNgElementStrategy implements NgElementStrategy {
         return;
       }
 
+      // Record the changed value and update internal state to reflect the fact that this input has
+      // changed.
       this.recordInputChange(property, value);
+      this.unchangedInputs.delete(property);
+      this.hasInputChanges = true;
+
+      // Update the component instance and schedule change detection.
       this.componentRef.instance[property] = value;
       this.scheduleDetectChanges();
     });
@@ -172,6 +190,7 @@ export class ComponentNgElementStrategy implements NgElementStrategy {
     const projectableNodes =
         extractProjectableNodes(element, this.componentFactory.ngContentSelectors);
     this.componentRef = this.componentFactory.create(childInjector, projectableNodes, element);
+    this.viewChangeDetectorRef = this.componentRef.injector.get(ChangeDetectorRef);
 
     this.implementsOnChanges = isFunction((this.componentRef.instance as OnChanges).ngOnChanges);
 
@@ -187,12 +206,6 @@ export class ComponentNgElementStrategy implements NgElementStrategy {
   /** Set any stored initial inputs on the component's properties. */
   protected initializeInputs(): void {
     this.componentFactory.inputs.forEach(({propName}) => {
-      if (this.implementsOnChanges) {
-        // If the component implements `ngOnChanges()`, keep track of which inputs have never
-        // changed so far.
-        this.unchangedInputs.add(propName);
-      }
-
       if (this.initialInputValues.has(propName)) {
         // Call `setInputValue()` now that the component has been instantiated to update its
         // properties and fire `ngOnChanges()`.
@@ -227,6 +240,17 @@ export class ComponentNgElementStrategy implements NgElementStrategy {
     (componentRef.instance as OnChanges).ngOnChanges(inputChanges);
   }
 
+  /**
+   * Marks the component view for check, if necessary.
+   * (NOTE: This is required when the `ChangeDetectionStrategy` is set to `OnPush`.)
+   */
+  protected markViewForCheck(viewChangeDetectorRef: ChangeDetectorRef): void {
+    if (this.hasInputChanges) {
+      this.hasInputChanges = false;
+      viewChangeDetectorRef.markForCheck();
+    }
+  }
+
   /**
    * Schedules change detection to run on the component.
    * Ignores subsequent calls if already scheduled.
@@ -247,8 +271,7 @@ export class ComponentNgElementStrategy implements NgElementStrategy {
    */
   protected recordInputChange(property: string, currentValue: any): void {
     // Do not record the change if the component does not implement `OnChanges`.
-    // (We can only determine that after the component has been instantiated.)
-    if (this.componentRef !== null && !this.implementsOnChanges) {
+    if (!this.implementsOnChanges) {
       return;
     }
 
@@ -257,7 +280,7 @@ export class ComponentNgElementStrategy implements NgElementStrategy {
     }
 
     // If there already is a change, modify the current value to match but leave the values for
-    // previousValue and isFirstChange.
+    // `previousValue` and `isFirstChange`.
     const pendingChange = this.inputChanges[property];
     if (pendingChange) {
       pendingChange.currentValue = currentValue;
@@ -265,8 +288,6 @@ export class ComponentNgElementStrategy implements NgElementStrategy {
     }
 
     const isFirstChange = this.unchangedInputs.has(property);
-    this.unchangedInputs.delete(property);
-
     const previousValue = isFirstChange ? undefined : this.getInputValue(property);
     this.inputChanges[property] = new SimpleChange(previousValue, currentValue, isFirstChange);
   }
@@ -278,6 +299,7 @@ export class ComponentNgElementStrategy implements NgElementStrategy {
     }
 
     this.callNgOnChanges(this.componentRef);
+    this.markViewForCheck(this.viewChangeDetectorRef!);
     this.componentRef.changeDetectorRef.detectChanges();
   }
 

packages/elements/test/component-factory-strategy_spec.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ApplicationRef, ComponentFactory, ComponentFactoryResolver, ComponentRef, Injector, NgModuleRef, NgZone, SimpleChange, SimpleChanges, Type} from '@angular/core';
+import {ApplicationRef, ChangeDetectorRef, ComponentFactory, ComponentFactoryResolver, ComponentRef, Injector, NgModuleRef, NgZone, SimpleChange, SimpleChanges, Type} from '@angular/core';
 import {fakeAsync, tick} from '@angular/core/testing';
 import {Subject} from 'rxjs';
 
@@ -180,8 +180,11 @@ describe('ComponentFactoryNgElementStrategy', () => {
   });
 
   describe('when inputs change and is connected', () => {
+    let viewChangeDetectorRef: ChangeDetectorRef;
+
     beforeEach(() => {
       strategy.connect(document.createElement('div'));
+      viewChangeDetectorRef = componentRef.injector.get(ChangeDetectorRef);
     });
 
     it('should be set on the component instance', () => {
@@ -209,6 +212,22 @@ describe('ComponentFactoryNgElementStrategy', () => {
          expect(componentRef.changeDetectorRef.detectChanges).toHaveBeenCalledTimes(2);
        }));
 
+    it('should not detect changes if the input is set to the same value', fakeAsync(() => {
+         (componentRef.changeDetectorRef.detectChanges as jasmine.Spy).calls.reset();
+
+         strategy.setInputValue('fooFoo', 'fooFoo-1');
+         strategy.setInputValue('barBar', 'barBar-1');
+         tick(16);  // scheduler waits 16ms if RAF is unavailable
+         expect(componentRef.changeDetectorRef.detectChanges).toHaveBeenCalledTimes(1);
+
+         (componentRef.changeDetectorRef.detectChanges as jasmine.Spy).calls.reset();
+
+         strategy.setInputValue('fooFoo', 'fooFoo-1');
+         strategy.setInputValue('barBar', 'barBar-1');
+         tick(16);  // scheduler waits 16ms if RAF is unavailable
+         expect(componentRef.changeDetectorRef.detectChanges).not.toHaveBeenCalled();
+       }));
+
     it('should call ngOnChanges', fakeAsync(() => {
          strategy.setInputValue('fooFoo', 'fooFoo-1');
          tick(16);  // scheduler waits 16ms if RAF is unavailable
@@ -246,6 +265,22 @@ describe('ComponentFactoryNgElementStrategy', () => {
          });
        }));
 
+    it('should not call ngOnChanges if the inout is set to the same value', fakeAsync(() => {
+         const ngOnChangesSpy = spyOn(componentRef.instance, 'ngOnChanges');
+
+         strategy.setInputValue('fooFoo', 'fooFoo-1');
+         strategy.setInputValue('barBar', 'barBar-1');
+         tick(16);  // scheduler waits 16ms if RAF is unavailable
+         expect(ngOnChangesSpy).toHaveBeenCalledTimes(1);
+
+         ngOnChangesSpy.calls.reset();
+
+         strategy.setInputValue('fooFoo', 'fooFoo-1');
+         strategy.setInputValue('barBar', 'barBar-1');
+         tick(16);  // scheduler waits 16ms if RAF is unavailable
+         expect(ngOnChangesSpy).not.toHaveBeenCalled();
+       }));
+
     it('should not try to call ngOnChanges if not present on the component', fakeAsync(() => {
          const factory2 = new FakeComponentFactory(FakeComponentWithoutNgOnChanges);
          const strategy2 = new ComponentNgElementStrategy(factory2, injector);
@@ -261,6 +296,71 @@ describe('ComponentFactoryNgElementStrategy', () => {
          // been thrown and `changeDetectorRef2.detectChanges()` would not have been called.
          expect(changeDetectorRef2.detectChanges).toHaveBeenCalledTimes(1);
        }));
+
+    it('should mark the view for check', fakeAsync(() => {
+         expect(viewChangeDetectorRef.markForCheck).not.toHaveBeenCalled();
+
+         strategy.setInputValue('fooFoo', 'fooFoo-1');
+         tick(16);  // scheduler waits 16ms if RAF is unavailable
+
+         expect(viewChangeDetectorRef.markForCheck).toHaveBeenCalledTimes(1);
+       }));
+
+    it('should mark the view for check once for multiple input changes', fakeAsync(() => {
+         strategy.setInputValue('fooFoo', 'fooFoo-1');
+         strategy.setInputValue('barBar', 'barBar-1');
+         tick(16);  // scheduler waits 16ms if RAF is unavailable
+
+         expect(viewChangeDetectorRef.markForCheck).toHaveBeenCalledTimes(1);
+       }));
+
+    it('should mark the view for check twice for changes in different rounds with previous values',
+       fakeAsync(() => {
+         strategy.setInputValue('fooFoo', 'fooFoo-1');
+         strategy.setInputValue('barBar', 'barBar-1');
+         tick(16);  // scheduler waits 16ms if RAF is unavailable
+
+         expect(viewChangeDetectorRef.markForCheck).toHaveBeenCalledTimes(1);
+
+         strategy.setInputValue('fooFoo', 'fooFoo-2');
+         strategy.setInputValue('barBar', 'barBar-2');
+         tick(16);  // scheduler waits 16ms if RAF is unavailable
+
+         expect(viewChangeDetectorRef.markForCheck).toHaveBeenCalledTimes(2);
+       }));
+
+    it('should mark the view for check even if ngOnChanges is not present on the component',
+       fakeAsync(() => {
+         const factory2 = new FakeComponentFactory(FakeComponentWithoutNgOnChanges);
+         const strategy2 = new ComponentNgElementStrategy(factory2, injector);
+         const viewChangeDetectorRef2 = factory2.componentRef.injector.get(ChangeDetectorRef);
+
+         strategy2.connect(document.createElement('div'));
+         (viewChangeDetectorRef2.markForCheck as jasmine.Spy).calls.reset();
+
+         strategy2.setInputValue('fooFoo', 'fooFoo-1');
+         expect(() => tick(16)).not.toThrow();  // scheduler waits 16ms if RAF is unavailable
+
+         // If the strategy would have tried to call `component.ngOnChanges()`, an error would have
+         // been thrown and `viewChangeDetectorRef2.markForCheck()` would not have been called.
+         expect(viewChangeDetectorRef2.markForCheck).toHaveBeenCalledTimes(1);
+       }));
+
+    it('should not mark the view for check if the input is set to the same value', fakeAsync(() => {
+         (viewChangeDetectorRef.markForCheck as jasmine.Spy).calls.reset();
+
+         strategy.setInputValue('fooFoo', 'fooFoo-1');
+         strategy.setInputValue('barBar', 'barBar-1');
+         tick(16);  // scheduler waits 16ms if RAF is unavailable
+         expect(viewChangeDetectorRef.markForCheck).toHaveBeenCalledTimes(1);
+
+         (viewChangeDetectorRef.markForCheck as jasmine.Spy).calls.reset();
+
+         strategy.setInputValue('fooFoo', 'fooFoo-1');
+         strategy.setInputValue('barBar', 'barBar-1');
+         tick(16);  // scheduler waits 16ms if RAF is unavailable
+         expect(viewChangeDetectorRef.markForCheck).not.toHaveBeenCalled();
+       }));
   });
 
   describe('disconnect', () => {
@@ -345,6 +445,9 @@ export class FakeComponentFactory<T extends Type<any>> extends ComponentFactory<
       {
         changeDetectorRef: jasmine.createSpyObj('changeDetectorRef', ['detectChanges']),
         hostView: {},
+        injector: jasmine.createSpyObj('injector', {
+          get: jasmine.createSpyObj('viewChangeDetectorRef', ['markForCheck']),
+        }),
         instance: new this.ComponentClass(),
       });