diff --git a/integration/_payload-limits.json b/integration/_payload-limits.json index c2f9b2f006..1cf14f2116 100644 --- a/integration/_payload-limits.json +++ b/integration/_payload-limits.json @@ -12,7 +12,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 15267, + "main-es2015": 15783, "polyfills-es2015": 36808 } } diff --git a/packages/core/src/render3/assert.ts b/packages/core/src/render3/assert.ts index 4415935875..e822917971 100644 --- a/packages/core/src/render3/assert.ts +++ b/packages/core/src/render3/assert.ts @@ -72,3 +72,14 @@ export function assertFirstCreatePass(tView: TView, errMessage?: string) { assertEqual( tView.firstCreatePass, true, errMessage || 'Should only be called in first create pass.'); } + +/** + * This is a basic sanity check that an object is probably a directive def. DirectiveDef is + * an interface, so we can't do a direct instanceof check. + */ +export function assertDirectiveDef(obj: any) { + if (obj.type === undefined || obj.selectors == undefined || obj.inputs === undefined) { + throwError( + `Expected a DirectiveDef/ComponentDef and this object does not seem to have the expected shape.`); + } +} diff --git a/packages/core/src/render3/component.ts b/packages/core/src/render3/component.ts index 4e847ecd2e..2e2825fd26 100644 --- a/packages/core/src/render3/component.ts +++ b/packages/core/src/render3/component.ts @@ -255,7 +255,6 @@ export function LifecycleHooksFeature(component: any, def: ComponentDef): v const rootTView = readPatchedLView(component) ![TVIEW]; const dirIndex = rootTView.data.length - 1; - registerPreOrderHooks(dirIndex, def, rootTView, -1, -1, -1); // TODO(misko): replace `as TNode` with createTNode call. (needs refactoring to lose dep on // LNode). registerPostOrderHooks( diff --git a/packages/core/src/render3/di.ts b/packages/core/src/render3/di.ts index c565dd11e5..57b8cb8058 100644 --- a/packages/core/src/render3/di.ts +++ b/packages/core/src/render3/di.ts @@ -15,8 +15,10 @@ import {InjectFlags} from '../di/interface/injector'; import {Type} from '../interface/type'; import {assertDefined, assertEqual} from '../util/assert'; +import {assertDirectiveDef} from './assert'; import {getFactoryDef} from './definition'; import {NG_ELEMENT_ID, NG_FACTORY_DEF} from './fields'; +import {registerPreOrderHooks} from './hooks'; import {DirectiveDef, FactoryFn} from './interfaces/definition'; import {NO_PARENT_INJECTOR, NodeInjectorFactory, PARENT_INJECTOR, RelativeInjectorLocation, RelativeInjectorLocationFlags, TNODE, isFactory} from './interfaces/injector'; import {AttributeMarker, TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TNode, TNodeProviderIndexes, TNodeType} from './interfaces/node'; @@ -471,7 +473,7 @@ function searchTokensOnInjector( const injectableIdx = locateDirectiveOrProvider( tNode, currentTView, token, canAccessViewProviders, isHostSpecialCase); if (injectableIdx !== null) { - return getNodeInjectable(currentTView.data, lView, injectableIdx, tNode as TElementNode); + return getNodeInjectable(lView, currentTView, injectableIdx, tNode as TElementNode); } else { return NOT_FOUND; } @@ -519,15 +521,16 @@ export function locateDirectiveOrProvider( } /** -* Retrieve or instantiate the injectable from the `lData` at particular `index`. +* Retrieve or instantiate the injectable from the `LView` at particular `index`. * * This function checks to see if the value has already been instantiated and if so returns the * cached `injectable`. Otherwise if it detects that the value is still a factory it * instantiates the `injectable` and caches the value. */ export function getNodeInjectable( - tData: TData, lView: LView, index: number, tNode: TDirectiveHostNode): any { + lView: LView, tView: TView, index: number, tNode: TDirectiveHostNode): any { let value = lView[index]; + const tData = tView.data; if (isFactory(value)) { const factory: NodeInjectorFactory = value; if (factory.resolving) { @@ -542,6 +545,16 @@ export function getNodeInjectable( enterDI(lView, tNode); try { value = lView[index] = factory.factory(undefined, tData, lView, tNode); + // This code path is hit for both directives and providers. + // For perf reasons, we want to avoid searching for hooks on providers. + // It does no harm to try (the hooks just won't exist), but the extra + // checks are unnecessary and this is a hot path. So we check to see + // if the index of the dependency is in the directive range for this + // tNode. If it's not, we know it's a provider and skip hook registration. + if (tView.firstCreatePass && index >= tNode.directiveStart) { + ngDevMode && assertDirectiveDef(tData[index]); + registerPreOrderHooks(index, tData[index] as DirectiveDef, tView); + } } finally { if (factory.injectImpl) setInjectImplementation(previousInjectImplementation); setIncludeViewProviders(previousIncludeViewProviders); diff --git a/packages/core/src/render3/di_setup.ts b/packages/core/src/render3/di_setup.ts index f400d40ecc..e0815cc58b 100644 --- a/packages/core/src/render3/di_setup.ts +++ b/packages/core/src/render3/di_setup.ts @@ -215,13 +215,14 @@ function multiProvidersFactoryResolver( * This factory knows how to concatenate itself with the existing `multi` `providers`. */ function multiViewProvidersFactoryResolver( - this: NodeInjectorFactory, _: undefined, tData: TData, lData: LView, + this: NodeInjectorFactory, _: undefined, tData: TData, lView: LView, tNode: TDirectiveHostNode): any[] { const factories = this.multi !; let result: any[]; if (this.providerFactory) { const componentCount = this.providerFactory.componentProviders !; - const multiProviders = getNodeInjectable(tData, lData, this.providerFactory !.index !, tNode); + const multiProviders = + getNodeInjectable(lView, lView[TVIEW], this.providerFactory !.index !, tNode); // Copy the section of the array which contains `multi` `providers` from the component result = multiProviders.slice(0, componentCount); // Insert the `viewProvider` instances. diff --git a/packages/core/src/render3/hooks.ts b/packages/core/src/render3/hooks.ts index 8bedf39135..eebaed60aa 100644 --- a/packages/core/src/render3/hooks.ts +++ b/packages/core/src/render3/hooks.ts @@ -27,29 +27,11 @@ import {getCheckNoChangesMode} from './state'; * @param directiveIndex The index of the directive in LView * @param directiveDef The definition containing the hooks to setup in tView * @param tView The current TView - * @param nodeIndex The index of the node to which the directive is attached - * @param initialPreOrderHooksLength the number of pre-order hooks already registered before the - * current process, used to know if the node index has to be added to the array. If it is -1, - * the node index is never added. - * @param initialPreOrderCheckHooksLength same as previous for pre-order check hooks */ export function registerPreOrderHooks( - directiveIndex: number, directiveDef: DirectiveDef, tView: TView, nodeIndex: number, - initialPreOrderHooksLength: number, initialPreOrderCheckHooksLength: number): void { + directiveIndex: number, directiveDef: DirectiveDef, tView: TView): void { ngDevMode && assertFirstCreatePass(tView); const {onChanges, onInit, doCheck} = directiveDef; - if (initialPreOrderHooksLength >= 0 && - (!tView.preOrderHooks || initialPreOrderHooksLength === tView.preOrderHooks.length) && - (onChanges || onInit || doCheck)) { - (tView.preOrderHooks || (tView.preOrderHooks = [])).push(nodeIndex); - } - - if (initialPreOrderCheckHooksLength >= 0 && - (!tView.preOrderCheckHooks || - initialPreOrderCheckHooksLength === tView.preOrderCheckHooks.length) && - (onChanges || doCheck)) { - (tView.preOrderCheckHooks || (tView.preOrderCheckHooks = [])).push(nodeIndex); - } if (onChanges) { (tView.preOrderHooks || (tView.preOrderHooks = [])).push(directiveIndex, onChanges); diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index f2288a477f..f11ae30d21 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -1057,8 +1057,7 @@ export function instantiateRootComponent(tView: TView, lView: LView, def: Com generateExpandoInstructionBlock(tView, rootTNode, 1); baseResolveDirective(tView, lView, def); } - const directive = - getNodeInjectable(tView.data, lView, lView.length - 1, rootTNode as TElementNode); + const directive = getNodeInjectable(lView, tView, lView.length - 1, rootTNode as TElementNode); attachPatchData(directive, lView); const native = getNativeByTNode(rootTNode, lView); if (native) { @@ -1097,14 +1096,11 @@ export function resolveDirectives( if (def.providersResolver) def.providersResolver(def); } generateExpandoInstructionBlock(tView, tNode, directives.length); - const initialPreOrderHooksLength = (tView.preOrderHooks && tView.preOrderHooks.length) || 0; - const initialPreOrderCheckHooksLength = - (tView.preOrderCheckHooks && tView.preOrderCheckHooks.length) || 0; - const nodeIndex = tNode.index - HEADER_OFFSET; + let preOrderHooksFound = false; + let preOrderCheckHooksFound = false; for (let i = 0; i < directives.length; i++) { const def = directives[i] as DirectiveDef; - const directiveDefIdx = tView.data.length; baseResolveDirective(tView, lView, def); saveNameToExportMap(tView.data !.length - 1, def, exportsMap); @@ -1112,11 +1108,21 @@ export function resolveDirectives( if (def.contentQueries !== null) tNode.flags |= TNodeFlags.hasContentQuery; if (def.hostBindings !== null) tNode.flags |= TNodeFlags.hasHostBindings; - // Init hooks are queued now so ngOnInit is called in host components before - // any projected components. - registerPreOrderHooks( - directiveDefIdx, def, tView, nodeIndex, initialPreOrderHooksLength, - initialPreOrderCheckHooksLength); + // Only push a node index into the preOrderHooks array if this is the first + // pre-order hook found on this node. + if (!preOrderHooksFound && (def.onChanges || def.onInit || def.doCheck)) { + // We will push the actual hook function into this array later during dir instantiation. + // We cannot do it now because we must ensure hooks are registered in the same + // order that directives are created (i.e. injection order). + (tView.preOrderHooks || (tView.preOrderHooks = [])).push(tNode.index - HEADER_OFFSET); + preOrderHooksFound = true; + } + + if (!preOrderCheckHooksFound && (def.onChanges || def.doCheck)) { + (tView.preOrderCheckHooks || (tView.preOrderCheckHooks = [ + ])).push(tNode.index - HEADER_OFFSET); + preOrderCheckHooksFound = true; + } } initializeInputAndOutputAliases(tView, tNode); @@ -1148,7 +1154,7 @@ function instantiateAllDirectives( addComponentLogic(lView, tNode as TElementNode, def as ComponentDef); } - const directive = getNodeInjectable(tView.data, lView, i, tNode); + const directive = getNodeInjectable(lView, tView, i, tNode); attachPatchData(directive, lView); if (initialInputs !== null) { diff --git a/packages/core/src/render3/query.ts b/packages/core/src/render3/query.ts index b6d43f18a0..0717eeae84 100644 --- a/packages/core/src/render3/query.ts +++ b/packages/core/src/render3/query.ts @@ -281,7 +281,7 @@ function createResultForNode(lView: LView, tNode: TNode, matchingIdx: number, re return createSpecialToken(lView, tNode, read); } else { // read a token - return getNodeInjectable(lView[TVIEW].data, lView, matchingIdx, tNode as TElementNode); + return getNodeInjectable(lView, lView[TVIEW], matchingIdx, tNode as TElementNode); } } diff --git a/packages/core/test/acceptance/lifecycle_spec.ts b/packages/core/test/acceptance/lifecycle_spec.ts index 5175d3dd7d..359be58390 100644 --- a/packages/core/test/acceptance/lifecycle_spec.ts +++ b/packages/core/test/acceptance/lifecycle_spec.ts @@ -535,7 +535,7 @@ describe('onChanges', () => { ]); }); - it('should be called on directives after component', () => { + it('should be called on directives after component by default', () => { const events: any[] = []; @Directive({ @@ -607,6 +607,138 @@ describe('onChanges', () => { ]); }); + it('should be called on directives before component if component injects directives', () => { + + const events: any[] = []; + + @Directive({ + selector: '[dir]', + }) + class Dir { + @Input() + dir = ''; + + ngOnChanges(changes: SimpleChanges) { events.push({name: 'dir', changes}); } + } + + @Component({ + selector: 'comp', + template: `

{{val}}

`, + }) + class Comp { + @Input() + val = ''; + + constructor(public dir: Dir) {} + + ngOnChanges(changes: SimpleChanges) { events.push({name: 'comp', changes}); } + } + + @Component({ + template: ``, + }) + class App { + val = 'a'; + } + + TestBed.configureTestingModule({ + declarations: [App, Comp, Dir], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(events).toEqual([ + { + name: 'dir', + changes: { + dir: new SimpleChange(undefined, 'a', true), + } + }, + { + name: 'comp', + changes: { + val: new SimpleChange(undefined, 'a', true), + } + } + ]); + + events.length = 0; + fixture.componentInstance.val = 'b'; + fixture.detectChanges(); + + expect(events).toEqual([ + { + name: 'dir', + changes: { + dir: new SimpleChange('a', 'b', false), + } + }, + { + name: 'comp', + changes: { + val: new SimpleChange('a', 'b', false), + } + } + ]); + + }); + + it('should be called on multiple directives in injection order', () => { + + const events: any[] = []; + + @Directive({ + selector: '[dir]', + }) + class Dir { + @Input() + dir = ''; + + ngOnChanges(changes: SimpleChanges) { events.push({name: 'dir', changes}); } + } + + @Directive({ + selector: '[injectionDir]', + }) + class InjectionDir { + @Input() + injectionDir = ''; + + constructor(public dir: Dir) {} + + ngOnChanges(changes: SimpleChanges) { events.push({name: 'injectionDir', changes}); } + } + + @Component({ + template: `
`, + }) + class App { + val = 'a'; + } + + TestBed.configureTestingModule({ + declarations: [App, InjectionDir, Dir], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(events).toEqual([ + { + name: 'dir', + changes: { + dir: new SimpleChange(undefined, 'a', true), + } + }, + { + name: 'injectionDir', + changes: { + injectionDir: new SimpleChange(undefined, 'a', true), + } + } + ]); + }); + + it('should be called on directives on an element', () => { const events: any[] = []; @@ -1412,7 +1544,7 @@ describe('onInit', () => { expect(initialized).toEqual(['app', 'comp 1', 'projected 1', 'comp 2', 'projected 2']); }); - it('should be called on directives after component', () => { + it('should be called on directives after component by default', () => { const initialized: string[] = []; @Directive({ @@ -1455,6 +1587,95 @@ describe('onInit', () => { expect(initialized).toEqual(['app', 'comp 1', 'dir 1', 'comp 2', 'dir 2']); }); + it('should be called on multiple directives in injection order', () => { + + const events: any[] = []; + + @Directive({ + selector: '[dir]', + }) + class Dir { + @Input() + dir = ''; + + ngOnInit() { events.push('dir'); } + } + + @Directive({ + selector: '[injectionDir]', + }) + class InjectionDir { + @Input() + injectionDir = ''; + + constructor(public dir: Dir) {} + + ngOnInit() { events.push('injectionDir'); } + } + + @Component({ + template: `
`, + }) + class App { + val = 'a'; + + ngOnInit() { events.push('app'); } + } + + TestBed.configureTestingModule({ + declarations: [App, InjectionDir, Dir], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(events).toEqual(['app', 'dir', 'injectionDir']); + }); + + it('should be called on directives before component if component injects directives', () => { + const initialized: string[] = []; + + @Directive({ + selector: '[dir]', + }) + class Dir { + @Input('dir-name') + name = ''; + + ngOnInit() { initialized.push('dir ' + this.name); } + } + + @Component({ + selector: 'comp', + template: `

`, + }) + class Comp { + @Input() + name = ''; + + constructor(public dir: Dir) {} + + ngOnInit() { initialized.push('comp ' + this.name); } + } + + @Component({ + template: ` + + + ` + }) + class App { + ngOnInit() { initialized.push('app'); } + } + + TestBed.configureTestingModule({ + declarations: [App, Comp, Dir], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(initialized).toEqual(['app', 'dir 1', 'comp 1', 'dir 2', 'comp 2']); + }); + it('should be called on directives on an element', () => { const initialized: string[] = []; @@ -1662,7 +1883,7 @@ describe('doCheck', () => { expect(events).toEqual(['onInit', 'doCheck']); }); - it('should be called on directives after component', () => { + it('should be called on directives after component by default', () => { const doChecks: string[] = []; @Directive({ selector: '[dir]', @@ -1704,6 +1925,94 @@ describe('doCheck', () => { expect(doChecks).toEqual(['app', 'comp 1', 'dir 1', 'comp 2', 'dir 2']); }); + it('should be called on directives before component if component injects directives', () => { + const doChecks: string[] = []; + @Directive({ + selector: '[dir]', + }) + class Dir { + @Input('dir') + name = ''; + + ngDoCheck() { doChecks.push('dir ' + this.name); } + } + + @Component({ + selector: 'comp', + template: `

test

`, + }) + class Comp { + @Input() + name = ''; + + constructor(public dir: Dir) {} + + ngDoCheck() { doChecks.push('comp ' + this.name); } + } + + @Component({ + template: ` + + + ` + }) + class App { + ngDoCheck() { doChecks.push('app'); } + } + + TestBed.configureTestingModule({ + declarations: [App, Comp, Dir], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(doChecks).toEqual(['app', 'dir 1', 'comp 1', 'dir 2', 'comp 2']); + }); + + it('should be called on multiple directives in injection order', () => { + + const events: any[] = []; + + @Directive({ + selector: '[dir]', + }) + class Dir { + @Input() + dir = ''; + + ngDoCheck() { events.push('dir'); } + } + + @Directive({ + selector: '[injectionDir]', + }) + class InjectionDir { + @Input() + injectionDir = ''; + + constructor(public dir: Dir) {} + + ngDoCheck() { events.push('injectionDir'); } + } + + @Component({ + template: `
`, + }) + class App { + val = 'a'; + + ngDoCheck() { events.push('app'); } + } + + TestBed.configureTestingModule({ + declarations: [App, InjectionDir, Dir], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(events).toEqual(['app', 'dir', 'injectionDir']); + }); + it('should be called on directives on an element', () => { const doChecks: string[] = []; diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json index 1f6c456c07..7bfaa2eccd 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -416,6 +416,9 @@ { "name": "refreshView" }, + { + "name": "registerPreOrderHooks" + }, { "name": "renderChildComponents" }, diff --git a/packages/forms/test/value_accessor_integration_spec.ts b/packages/forms/test/value_accessor_integration_spec.ts index a8061fe60c..7eac63877d 100644 --- a/packages/forms/test/value_accessor_integration_spec.ts +++ b/packages/forms/test/value_accessor_integration_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Component, Directive, EventEmitter, Input, Output, Type} from '@angular/core'; +import {Component, Directive, EventEmitter, Input, Output, Type, ViewChild} from '@angular/core'; import {ComponentFixture, TestBed, async, fakeAsync, tick} from '@angular/core/testing'; import {AbstractControl, ControlValueAccessor, FormControl, FormGroup, FormsModule, NG_VALIDATORS, NG_VALUE_ACCESSOR, NgControl, NgForm, NgModel, ReactiveFormsModule, Validators} from '@angular/forms'; import {By} from '@angular/platform-browser/src/dom/debug/by'; @@ -1055,6 +1055,16 @@ import {dispatchEvent} from '@angular/platform-browser/testing/src/browser_util' fixture.detectChanges(); expect(fixture.componentInstance.control.status).toEqual('DISABLED'); }); + + it('should populate control in ngOnInit when injecting NgControl', () => { + const fixture = initTest(MyInputForm, MyInput); + fixture.componentInstance.form = new FormGroup({'login': new FormControl('aa')}); + fixture.detectChanges(); + + expect(fixture.componentInstance.myInput !.control).toBeDefined(); + expect(fixture.componentInstance.myInput !.control) + .toEqual(fixture.componentInstance.myInput !.controlDir.control); + }); }); describe('in template-driven forms', () => { @@ -1359,7 +1369,11 @@ export class MyInput implements ControlValueAccessor { // TODO(issue/24571): remove '!'. value !: string; - constructor(cd: NgControl) { cd.valueAccessor = this; } + control: AbstractControl|null = null; + + constructor(public controlDir: NgControl) { controlDir.valueAccessor = this; } + + ngOnInit() { this.control = this.controlDir.control; } writeValue(value: any) { this.value = `!${value}!`; } @@ -1380,6 +1394,7 @@ export class MyInput implements ControlValueAccessor { export class MyInputForm { // TODO(issue/24571): remove '!'. form !: FormGroup; + @ViewChild(MyInput) myInput: MyInput|null = null; } @Component({