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
This commit is contained in:
George Kalpakas 2020-03-19 13:50:25 +02:00 committed by Misko Hevery
parent 55dac05cf2
commit b14ac96750
2 changed files with 25 additions and 18 deletions

View File

@ -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<string, any>();
/** Set of inputs that were not initially set when the component was created. */
private readonly uninitializedInputs = new Set<string>();
/**
* 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<string>();
constructor(private componentFactory: ComponentFactory<any>, 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);

View File

@ -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`);
}
});
}