From b14ac967501c494dec04a749a0b4ac29201f3626 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Thu, 19 Mar 2020 13:50:25 +0200 Subject: [PATCH] fix(elements): correctly set `SimpleChange#firstChange` for pre-existing inputs (#36140) Previously, when an input property was set on an `NgElement` before instantiating the underlying component, the `SimpleChange` object passed to `ngOnChanges()` would have `firstChange` set to false, even if this was the first change (as far as the component instance was concerned). This commit fixes this by ensuring `SimpleChange#firstChange` is set to true on first change, regardless if the property was set before or after instantiating the component. This alignthe behavior of Angular custom elements with that of the corresponding components when used directly (not as custom elements). Jira issue: [FW-2007](https://angular-team.atlassian.net/browse/FW-2007) Fixes #36130 PR Close #36140 --- .../src/component-factory-strategy.ts | 23 ++++++++++++------- .../test/component-factory-strategy_spec.ts | 20 ++++++++-------- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/packages/elements/src/component-factory-strategy.ts b/packages/elements/src/component-factory-strategy.ts index 8f27a721c9..6480045dae 100644 --- a/packages/elements/src/component-factory-strategy.ts +++ b/packages/elements/src/component-factory-strategy.ts @@ -66,8 +66,11 @@ export class ComponentNgElementStrategy implements NgElementStrategy { /** Initial input values that were set before the component was created. */ private readonly initialInputValues = new Map(); - /** Set of inputs that were not initially set when the component was created. */ - private readonly uninitializedInputs = new Set(); + /** + * 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.) + */ + private readonly unchangedInputs = new Set(); constructor(private componentFactory: ComponentFactory, private injector: Injector) {} @@ -164,12 +167,16 @@ 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()`. this.setInputValue(propName, this.initialInputValues.get(propName)); - } else { - // Keep track of inputs that were not initialized in case we need to know this for - // calling ngOnChanges with SimpleChanges - this.uninitializedInputs.add(propName); } }); @@ -235,8 +242,8 @@ export class ComponentNgElementStrategy implements NgElementStrategy { return; } - const isFirstChange = this.uninitializedInputs.has(property); - this.uninitializedInputs.delete(property); + 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); diff --git a/packages/elements/test/component-factory-strategy_spec.ts b/packages/elements/test/component-factory-strategy_spec.ts index 54b8bd8f8a..4a99800a6b 100644 --- a/packages/elements/test/component-factory-strategy_spec.ts +++ b/packages/elements/test/component-factory-strategy_spec.ts @@ -93,21 +93,21 @@ describe('ComponentFactoryNgElementStrategy', () => { it('should call ngOnChanges with the change', () => { expectSimpleChanges(componentRef.instance.simpleChanges[0], { - fooFoo: new SimpleChange(undefined, 'fooFoo-1', false), - falsyNull: new SimpleChange(undefined, null, false), - falsyEmpty: new SimpleChange(undefined, '', false), - falsyFalse: new SimpleChange(undefined, false, false), - falsyZero: new SimpleChange(undefined, 0, false), + fooFoo: new SimpleChange(undefined, 'fooFoo-1', true), + falsyNull: new SimpleChange(undefined, null, true), + falsyEmpty: new SimpleChange(undefined, '', true), + falsyFalse: new SimpleChange(undefined, false, true), + falsyZero: new SimpleChange(undefined, 0, true), }); }); it('should call ngOnChanges with proper firstChange value', fakeAsync(() => { - strategy.setInputValue('falsyUndefined', 'notanymore'); + strategy.setInputValue('fooFoo', 'fooFoo-2'); strategy.setInputValue('barBar', 'barBar-1'); tick(16); // scheduler waits 16ms if RAF is unavailable (strategy as any).detectChanges(); expectSimpleChanges(componentRef.instance.simpleChanges[1], { - falsyUndefined: new SimpleChange(undefined, 'notanymore', false), + fooFoo: new SimpleChange('fooFoo-1', 'fooFoo-2', false), barBar: new SimpleChange(undefined, 'barBar-1', true), }); })); @@ -296,9 +296,9 @@ function expectSimpleChanges(actual: SimpleChanges, expected: SimpleChanges) { Object.keys(expected).forEach(key => { expect(actual[key]).toBeTruthy(`Change should have included key ${key}`); if (actual[key]) { - expect(actual[key].previousValue).toBe(expected[key].previousValue); - expect(actual[key].currentValue).toBe(expected[key].currentValue); - expect(actual[key].firstChange).toBe(expected[key].firstChange); + expect(actual[key].previousValue).toBe(expected[key].previousValue, `${key}.previousValue`); + expect(actual[key].currentValue).toBe(expected[key].currentValue, `${key}.currentValue`); + expect(actual[key].firstChange).toBe(expected[key].firstChange, `${key}.firstChange`); } }); }