From 94b8aaeba8822b519af02a3aaf38a000b6c1bd5e Mon Sep 17 00:00:00 2001 From: Marc Laval Date: Wed, 6 Feb 2019 15:03:42 +0100 Subject: [PATCH] fix(ivy): ngOnChanges should be inherited from super class (#28563) PR Close #28563 --- .../features/inherit_definition_feature.ts | 5 ++ packages/core/src/render3/jit/directive.ts | 4 +- .../inherit_definition_feature_spec.ts | 60 +++++++++++++++++++ .../inherit_definition_feature_spec.ts | 40 ------------- .../ts/full/e2e_test/static_full_spec.ts | 39 ++++++------ .../ts/lite/e2e_test/static_lite_spec.ts | 46 +++++++------- 6 files changed, 105 insertions(+), 89 deletions(-) create mode 100644 packages/core/test/acceptance/inherit_definition_feature_spec.ts diff --git a/packages/core/src/render3/features/inherit_definition_feature.ts b/packages/core/src/render3/features/inherit_definition_feature.ts index a1af15752f..fabd73f4ef 100644 --- a/packages/core/src/render3/features/inherit_definition_feature.ts +++ b/packages/core/src/render3/features/inherit_definition_feature.ts @@ -12,6 +12,7 @@ import {fillProperties} from '../../util/property'; import {EMPTY_ARRAY, EMPTY_OBJ} from '../empty'; import {ComponentDef, DirectiveDef, DirectiveDefFeature, RenderFlags} from '../interfaces/definition'; +import {NgOnChangesFeature} from './ng_onchanges_feature'; /** @@ -168,6 +169,10 @@ export function InheritDefinitionFeature(definition: DirectiveDef| Componen definition.doCheck = definition.doCheck || superPrototype.ngDoCheck; definition.onDestroy = definition.onDestroy || superPrototype.ngOnDestroy; definition.onInit = definition.onInit || superPrototype.ngOnInit; + + if (superPrototype.ngOnChanges) { + NgOnChangesFeature()(definition); + } } } diff --git a/packages/core/src/render3/jit/directive.ts b/packages/core/src/render3/jit/directive.ts index 2f53453aa9..57a217b87f 100644 --- a/packages/core/src/render3/jit/directive.ts +++ b/packages/core/src/render3/jit/directive.ts @@ -151,9 +151,7 @@ function directiveMetadata(type: Type, metadata: Directive): R3DirectiveMet inputs: metadata.inputs || EMPTY_ARRAY, outputs: metadata.outputs || EMPTY_ARRAY, queries: extractQueriesMetadata(type, propMetadata, isContentQuery), - lifecycle: { - usesOnChanges: type.prototype.ngOnChanges !== undefined, - }, + lifecycle: {usesOnChanges: type.prototype.hasOwnProperty('ngOnChanges')}, typeSourceSpan: null !, usesInheritance: !extendsDirectlyFromObject(type), exportAs: extractExportAs(metadata.exportAs), diff --git a/packages/core/test/acceptance/inherit_definition_feature_spec.ts b/packages/core/test/acceptance/inherit_definition_feature_spec.ts new file mode 100644 index 0000000000..f062439ff7 --- /dev/null +++ b/packages/core/test/acceptance/inherit_definition_feature_spec.ts @@ -0,0 +1,60 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {Component, Directive, Input, OnChanges, Type} from '@angular/core'; +import {TestBed} from '@angular/core/testing'; + +describe('ngOnChanges', () => { + it('should be inherited when super is a directive', () => { + const log: string[] = []; + + @Directive({selector: '[superDir]'}) + class SuperDirective implements OnChanges { + @Input() someInput = ''; + + ngOnChanges() { log.push('on changes!'); } + } + + @Directive({selector: '[subDir]'}) + class SubDirective extends SuperDirective { + } + + TestBed.configureTestingModule({declarations: [AppComp, SubDirective]}); + TestBed.overrideComponent( + AppComp, {set: new Component({template: '
'})}); + const fixture = TestBed.createComponent(AppComp); + fixture.detectChanges(); + + expect(log).toEqual(['on changes!']); + }); + + it('should be inherited when super is a simple class', () => { + const log: string[] = []; + + class SuperClass { + ngOnChanges() { log.push('on changes!'); } + } + + @Directive({selector: '[subDir]'}) + class SubDirective extends SuperClass { + @Input() someInput = ''; + } + + TestBed.configureTestingModule({declarations: [AppComp, SubDirective]}); + TestBed.overrideComponent( + AppComp, {set: new Component({template: '
'})}); + const fixture = TestBed.createComponent(AppComp); + fixture.detectChanges(); + + expect(log).toEqual(['on changes!']); + }); +}); + +@Component({selector: 'app-comp', template: ``}) +class AppComp { +} diff --git a/packages/core/test/render3/inherit_definition_feature_spec.ts b/packages/core/test/render3/inherit_definition_feature_spec.ts index cafa58a4a4..d5c823fe4b 100644 --- a/packages/core/test/render3/inherit_definition_feature_spec.ts +++ b/packages/core/test/render3/inherit_definition_feature_spec.ts @@ -662,46 +662,6 @@ describe('InheritDefinitionFeature', () => { }).toThrowError('Directives cannot inherit Components'); }); - it('should inherit ngOnChanges', () => { - const log: string[] = []; - let subDir !: SubDirective; - - class SuperDirective { - someInput = ''; - - ngOnChanges() { log.push('on changes!'); } - - static ngDirectiveDef = defineDirective({ - type: SuperDirective, - selectors: [['', 'superDir', '']], - factory: () => new SuperDirective(), - features: [NgOnChangesFeature()], - inputs: {someInput: 'someInput'} - }); - } - - class SubDirective extends SuperDirective { - static ngDirectiveDef = defineDirective({ - type: SubDirective, - selectors: [['', 'subDir', '']], - factory: () => subDir = new SubDirective(), - features: [InheritDefinitionFeature], - }); - } - - const App = createComponent('app', (rf: RenderFlags, ctx: any) => { - if (rf & RenderFlags.Create) { - element(0, 'div', ['subDir', '']); - } - if (rf & RenderFlags.Update) { - elementProperty(0, 'someInput', bind(1)); - } - }, 1, 1, [SubDirective]); - - const fixture = new ComponentFixture(App); - expect(log).toEqual(['on changes!']); - }); - it('should NOT inherit providers', () => { let otherDir !: OtherDirective; diff --git a/packages/examples/upgrade/static/ts/full/e2e_test/static_full_spec.ts b/packages/examples/upgrade/static/ts/full/e2e_test/static_full_spec.ts index cfc230424c..442802edd7 100644 --- a/packages/examples/upgrade/static/ts/full/e2e_test/static_full_spec.ts +++ b/packages/examples/upgrade/static/ts/full/e2e_test/static_full_spec.ts @@ -6,7 +6,6 @@ * found in the LICENSE file at https://angular.io/license */ -import {fixmeIvy} from '@angular/private/testing'; import {browser, by, element} from 'protractor'; import {verifyNoBrowserErrors} from '../../../../../test-utils'; @@ -29,28 +28,26 @@ describe('upgrade/static (full)', () => { expect(heroComponents.count()).toEqual(3); }); - fixmeIvy('unknown; component does not seem to render name & description') - .it('should add a new hero when the "Add Hero" button is pressed', () => { - const addHeroButton = element.all(by.css('button')).last(); - expect(addHeroButton.getText()).toEqual('Add Hero'); - addHeroButton.click(); - const heroComponents = element.all(by.css('ng1-hero')); - expect(heroComponents.last().element(by.css('h2')).getText()).toEqual('Kamala Khan'); - }); + it('should add a new hero when the "Add Hero" button is pressed', () => { + const addHeroButton = element.all(by.css('button')).last(); + expect(addHeroButton.getText()).toEqual('Add Hero'); + addHeroButton.click(); + const heroComponents = element.all(by.css('ng1-hero')); + expect(heroComponents.last().element(by.css('h2')).getText()).toEqual('Kamala Khan'); + }); - fixmeIvy('unknown; component does not seem to render name & description') - .it('should remove a hero when the "Remove" button is pressed', () => { - let firstHero = element.all(by.css('ng1-hero')).get(0); - expect(firstHero.element(by.css('h2')).getText()).toEqual('Superman'); + it('should remove a hero when the "Remove" button is pressed', () => { + let firstHero = element.all(by.css('ng1-hero')).get(0); + expect(firstHero.element(by.css('h2')).getText()).toEqual('Superman'); - const removeHeroButton = firstHero.element(by.css('button')); - expect(removeHeroButton.getText()).toEqual('Remove'); - removeHeroButton.click(); + const removeHeroButton = firstHero.element(by.css('button')); + expect(removeHeroButton.getText()).toEqual('Remove'); + removeHeroButton.click(); - const heroComponents = element.all(by.css('ng1-hero')); - expect(heroComponents.count()).toEqual(2); + const heroComponents = element.all(by.css('ng1-hero')); + expect(heroComponents.count()).toEqual(2); - firstHero = element.all(by.css('ng1-hero')).get(0); - expect(firstHero.element(by.css('h2')).getText()).toEqual('Wonder Woman'); - }); + firstHero = element.all(by.css('ng1-hero')).get(0); + expect(firstHero.element(by.css('h2')).getText()).toEqual('Wonder Woman'); + }); }); diff --git a/packages/examples/upgrade/static/ts/lite/e2e_test/static_lite_spec.ts b/packages/examples/upgrade/static/ts/lite/e2e_test/static_lite_spec.ts index 42bc5cc332..61bf67d64e 100644 --- a/packages/examples/upgrade/static/ts/lite/e2e_test/static_lite_spec.ts +++ b/packages/examples/upgrade/static/ts/lite/e2e_test/static_lite_spec.ts @@ -6,7 +6,6 @@ * found in the LICENSE file at https://angular.io/license */ -import {fixmeIvy} from '@angular/private/testing'; import {ElementArrayFinder, ElementFinder, browser, by, element} from 'protractor'; import {verifyNoBrowserErrors} from '../../../../../test-utils'; @@ -59,35 +58,32 @@ describe('upgrade/static (lite)', () => { it('should initially not render the heroes', () => expectHeroes(false)); - fixmeIvy('unknown; component does not seem to render name & description') - .it('should toggle the heroes when clicking the "show/hide" button', () => { - showHideBtn.click(); - expectHeroes(true); + it('should toggle the heroes when clicking the "show/hide" button', () => { + showHideBtn.click(); + expectHeroes(true); - showHideBtn.click(); - expectHeroes(false); - }); + showHideBtn.click(); + expectHeroes(false); + }); - fixmeIvy('unknown; component does not seem to render name & description') - .it('should add a new hero when clicking the "add" button', () => { - showHideBtn.click(); - ng2HeroesAddBtn.click(); + it('should add a new hero when clicking the "add" button', () => { + showHideBtn.click(); + ng2HeroesAddBtn.click(); - expectHeroes(true, 4, 'Added hero Kamala Khan'); - expect(ng1Heroes.last()).toHaveName('Kamala Khan'); - }); + expectHeroes(true, 4, 'Added hero Kamala Khan'); + expect(ng1Heroes.last()).toHaveName('Kamala Khan'); + }); - fixmeIvy('unknown; component does not seem to render name & description') - .it('should remove a hero when clicking its "remove" button', () => { - showHideBtn.click(); + it('should remove a hero when clicking its "remove" button', () => { + showHideBtn.click(); - const firstHero = ng1Heroes.first(); - expect(firstHero).toHaveName('Superman'); + const firstHero = ng1Heroes.first(); + expect(firstHero).toHaveName('Superman'); - const removeBtn = firstHero.element(by.buttonText('Remove')); - removeBtn.click(); + const removeBtn = firstHero.element(by.buttonText('Remove')); + removeBtn.click(); - expectHeroes(true, 2, 'Removed hero Superman'); - expect(ng1Heroes.first()).not.toHaveName('Superman'); - }); + expectHeroes(true, 2, 'Removed hero Superman'); + expect(ng1Heroes.first()).not.toHaveName('Superman'); + }); });