diff --git a/packages/compiler-cli/src/ngtsc/scope/src/local.ts b/packages/compiler-cli/src/ngtsc/scope/src/local.ts index 5d04d2f145..ad505ed1b6 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/local.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/local.ts @@ -296,9 +296,10 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop const exportPipes = new Map(); // The algorithm is as follows: - // 1) Add directives/pipes declared in the NgModule to the compilation scope. - // 2) Add all of the directives/pipes from each NgModule imported into the current one to the - // compilation scope. At this point, the compilation scope is complete. + // 1) Add all of the directives/pipes from each NgModule imported into the current one to the + // compilation scope. + // 2) Add directives/pipes declared in the NgModule to the compilation scope. At this point, the + // compilation scope is complete. // 3) For each entry in the NgModule's exports: // a) Attempt to resolve it as an NgModule with its own exported directives/pipes. If it is // one, add them to the export scope of this NgModule. @@ -307,31 +308,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop // c) If it's neither an NgModule nor a directive/pipe in the compilation scope, then this // is an error. - // 1) add declarations. - for (const decl of ngModule.declarations) { - const directive = this.localReader.getDirectiveMetadata(decl); - const pipe = this.localReader.getPipeMetadata(decl); - if (directive !== null) { - compilationDirectives.set(decl.node, {...directive, ref: decl}); - } else if (pipe !== null) { - compilationPipes.set(decl.node, {...pipe, ref: decl}); - } else { - this.taintedModules.add(ngModule.ref.node); - - const errorNode = decl.getOriginForDiagnostics(ngModule.rawDeclarations !); - diagnostics.push(makeDiagnostic( - ErrorCode.NGMODULE_INVALID_DECLARATION, errorNode, - `The class '${decl.node.name.text}' is listed in the declarations of the NgModule '${ngModule.ref.node.name.text}', but is not a directive, a component, or a pipe. - -Either remove it from the NgModule's declarations, or add an appropriate Angular decorator.`, - [{node: decl.node.name, messageText: `'${decl.node.name.text}' is declared here.`}])); - continue; - } - - declared.add(decl.node); - } - - // 2) process imports. + // 1) process imports. for (const decl of ngModule.imports) { const importScope = this.getExportedScope(decl, diagnostics, ref.node, 'import'); if (importScope === null) { @@ -353,6 +330,30 @@ Either remove it from the NgModule's declarations, or add an appropriate Angular } } + // 2) add declarations. + for (const decl of ngModule.declarations) { + const directive = this.localReader.getDirectiveMetadata(decl); + const pipe = this.localReader.getPipeMetadata(decl); + if (directive !== null) { + compilationDirectives.set(decl.node, {...directive, ref: decl}); + } else if (pipe !== null) { + compilationPipes.set(decl.node, {...pipe, ref: decl}); + } else { + this.taintedModules.add(ngModule.ref.node); + + const errorNode = decl.getOriginForDiagnostics(ngModule.rawDeclarations !); + diagnostics.push(makeDiagnostic( + ErrorCode.NGMODULE_INVALID_DECLARATION, errorNode, + `The class '${decl.node.name.text}' is listed in the declarations ` + + `of the NgModule '${ngModule.ref.node.name.text}', but is not a directive, a component, or a pipe. ` + + `Either remove it from the NgModule's declarations, or add an appropriate Angular decorator.`, + [{node: decl.node.name, messageText: `'${decl.node.name.text}' is declared here.`}])); + continue; + } + + declared.add(decl.node); + } + // 3) process exports. // Exports can contain modules, components, or directives. They're processed differently. // Modules are straightforward. Directives and pipes from exported modules are added to the diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 76c840ab42..94958bd920 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -598,6 +598,213 @@ runInEachFileSystem(os => { expect(jsContents).toContain('outputs: { output: "output" }'); }); + it('should pick a Pipe defined in `declarations` over imported Pipes', () => { + env.tsconfig({}); + env.write('test.ts', ` + import {Component, Pipe, NgModule} from '@angular/core'; + + // ModuleA classes + + @Pipe({name: 'number'}) + class PipeA {} + + @NgModule({ + declarations: [PipeA], + exports: [PipeA] + }) + class ModuleA {} + + // ModuleB classes + + @Pipe({name: 'number'}) + class PipeB {} + + @Component({ + selector: 'app', + template: '{{ count | number }}' + }) + export class App {} + + @NgModule({ + imports: [ModuleA], + declarations: [PipeB, App], + }) + class ModuleB {} + `); + + env.driveMain(); + + const jsContents = trim(env.getContents('test.js')); + expect(jsContents).toContain('pipes: [PipeB]'); + }); + + it('should respect imported module order when selecting Pipe (last imported Pipe is used)', + () => { + env.tsconfig({}); + env.write('test.ts', ` + import {Component, Pipe, NgModule} from '@angular/core'; + + // ModuleA classes + + @Pipe({name: 'number'}) + class PipeA {} + + @NgModule({ + declarations: [PipeA], + exports: [PipeA] + }) + class ModuleA {} + + // ModuleB classes + + @Pipe({name: 'number'}) + class PipeB {} + + @NgModule({ + declarations: [PipeB], + exports: [PipeB] + }) + class ModuleB {} + + // ModuleC classes + + @Component({ + selector: 'app', + template: '{{ count | number }}' + }) + export class App {} + + @NgModule({ + imports: [ModuleA, ModuleB], + declarations: [App], + }) + class ModuleC {} + `); + + env.driveMain(); + + const jsContents = trim(env.getContents('test.js')); + expect(jsContents).toContain('pipes: [PipeB]'); + }); + + it('should add Directives and Components from `declarations` at the end of the list', () => { + env.tsconfig({}); + env.write('test.ts', ` + import {Component, Directive, NgModule} from '@angular/core'; + + // ModuleA classes + + @Directive({selector: '[dir]'}) + class DirectiveA {} + + @Component({ + selector: 'comp', + template: '...' + }) + class ComponentA {} + + @NgModule({ + declarations: [DirectiveA, ComponentA], + exports: [DirectiveA, ComponentA] + }) + class ModuleA {} + + // ModuleB classes + + @Directive({selector: '[dir]'}) + class DirectiveB {} + + @Component({ + selector: 'comp', + template: '...', + }) + export class ComponentB {} + + @Component({ + selector: 'app', + template: \` +
+ + \`, + }) + export class App {} + + @NgModule({ + imports: [ModuleA], + declarations: [DirectiveB, ComponentB, App], + }) + class ModuleB {} + `); + + env.driveMain(); + + const jsContents = trim(env.getContents('test.js')); + expect(jsContents).toContain('directives: [DirectiveA, DirectiveB, ComponentA, ComponentB]'); + }); + + it('should respect imported module order while processing Directives and Components', () => { + env.tsconfig({}); + env.write('test.ts', ` + import {Component, Directive, NgModule} from '@angular/core'; + + // ModuleA classes + + @Directive({selector: '[dir]'}) + class DirectiveA {} + + @Component({ + selector: 'comp', + template: '...' + }) + class ComponentA {} + + @NgModule({ + declarations: [DirectiveA, ComponentA], + exports: [DirectiveA, ComponentA] + }) + class ModuleA {} + + // ModuleB classes + + @Directive({selector: '[dir]'}) + class DirectiveB {} + + @Component({ + selector: 'comp', + template: '...' + }) + class ComponentB {} + + @NgModule({ + declarations: [DirectiveB, ComponentB], + exports: [DirectiveB, ComponentB] + }) + class ModuleB {} + + // ModuleC classes + + @Component({ + selector: 'app', + template: \` +
+ + \`, + }) + export class App {} + + @NgModule({ + imports: [ModuleA, ModuleB], + declarations: [App], + }) + class ModuleC {} + `); + + env.driveMain(); + + const jsContents = trim(env.getContents('test.js')); + expect(jsContents).toContain('directives: [DirectiveA, DirectiveB, ComponentA, ComponentB]'); + }); + it('should compile Components with a templateUrl in a different rootDir', () => { env.tsconfig({}, ['./extraRootDir']); env.write('extraRootDir/test.html', '

Hello World

'); diff --git a/packages/core/src/render3/jit/module.ts b/packages/core/src/render3/jit/module.ts index d0a4a194ac..49b93eefd1 100644 --- a/packages/core/src/render3/jit/module.ts +++ b/packages/core/src/render3/jit/module.ts @@ -455,19 +455,6 @@ export function transitiveScopesFor(moduleType: Type): NgModuleTransitiveS }, }; - maybeUnwrapFn(def.declarations).forEach(declared => { - const declaredWithDefs = declared as Type& { ɵpipe?: any; }; - - if (getPipeDef(declaredWithDefs)) { - scopes.compilation.pipes.add(declared); - } else { - // Either declared has a ɵcmp or ɵdir, or it's a component which hasn't - // had its template compiled yet. In either case, it gets added to the compilation's - // directives. - scopes.compilation.directives.add(declared); - } - }); - maybeUnwrapFn(def.imports).forEach((imported: Type) => { const importedType = imported as Type& { // If imported is an @NgModule: @@ -485,6 +472,19 @@ export function transitiveScopesFor(moduleType: Type): NgModuleTransitiveS importedScope.exported.pipes.forEach(entry => scopes.compilation.pipes.add(entry)); }); + maybeUnwrapFn(def.declarations).forEach(declared => { + const declaredWithDefs = declared as Type& { ɵpipe?: any; }; + + if (getPipeDef(declaredWithDefs)) { + scopes.compilation.pipes.add(declared); + } else { + // Either declared has a ɵcmp or ɵdir, or it's a component which hasn't + // had its template compiled yet. In either case, it gets added to the compilation's + // directives. + scopes.compilation.directives.add(declared); + } + }); + maybeUnwrapFn(def.exports).forEach((exported: Type) => { const exportedType = exported as Type& { // Components, Directives, NgModules, and Pipes can all be exported. diff --git a/packages/core/test/acceptance/directive_spec.ts b/packages/core/test/acceptance/directive_spec.ts index 41ba900960..2903f56d1a 100644 --- a/packages/core/test/acceptance/directive_spec.ts +++ b/packages/core/test/acceptance/directive_spec.ts @@ -7,7 +7,7 @@ */ import {CommonModule} from '@angular/common'; -import {Component, Directive, ElementRef, EventEmitter, Output, TemplateRef, ViewChild, ViewContainerRef} from '@angular/core'; +import {Component, Directive, ElementRef, EventEmitter, NgModule, Output, TemplateRef, ViewChild, ViewContainerRef} from '@angular/core'; import {Input} from '@angular/core/src/metadata'; import {TestBed} from '@angular/core/testing'; import {By} from '@angular/platform-browser'; @@ -633,4 +633,96 @@ describe('directives', () => { expect(div.getAttribute('title')).toBe('a'); }); }); + + describe('directives with the same selector', () => { + it('should process Directives from `declarations` list after imported ones', () => { + const log: string[] = []; + @Directive({selector: '[dir]'}) + class DirectiveA { + constructor() { log.push('DirectiveA.constructor'); } + ngOnInit() { log.push('DirectiveA.ngOnInit'); } + } + + @NgModule({ + declarations: [DirectiveA], + exports: [DirectiveA], + }) + class ModuleA { + } + + @Directive({selector: '[dir]'}) + class DirectiveB { + constructor() { log.push('DirectiveB.constructor'); } + ngOnInit() { log.push('DirectiveB.ngOnInit'); } + } + + @Component({ + selector: 'app', + template: '
', + }) + class App { + } + + TestBed.configureTestingModule({ + imports: [ModuleA], + declarations: [DirectiveB, App], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(log).toEqual([ + 'DirectiveA.constructor', 'DirectiveB.constructor', 'DirectiveA.ngOnInit', + 'DirectiveB.ngOnInit' + ]); + }); + + it('should respect imported module order', () => { + const log: string[] = []; + @Directive({selector: '[dir]'}) + class DirectiveA { + constructor() { log.push('DirectiveA.constructor'); } + ngOnInit() { log.push('DirectiveA.ngOnInit'); } + } + + @NgModule({ + declarations: [DirectiveA], + exports: [DirectiveA], + }) + class ModuleA { + } + + @Directive({selector: '[dir]'}) + class DirectiveB { + constructor() { log.push('DirectiveB.constructor'); } + ngOnInit() { log.push('DirectiveB.ngOnInit'); } + } + + @NgModule({ + declarations: [DirectiveB], + exports: [DirectiveB], + }) + class ModuleB { + } + + @Component({ + selector: 'app', + template: '
', + }) + class App { + } + + TestBed.configureTestingModule({ + imports: [ModuleA, ModuleB], + declarations: [App], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(log).toEqual([ + 'DirectiveA.constructor', 'DirectiveB.constructor', 'DirectiveA.ngOnInit', + 'DirectiveB.ngOnInit' + ]); + }); + + }); }); diff --git a/packages/core/test/acceptance/pipe_spec.ts b/packages/core/test/acceptance/pipe_spec.ts index 3dd1be88eb..da9a561d2b 100644 --- a/packages/core/test/acceptance/pipe_spec.ts +++ b/packages/core/test/acceptance/pipe_spec.ts @@ -110,6 +110,86 @@ describe('pipe', () => { expect(fixture.nativeElement.textContent).toEqual('value a b default 0 1 2 3'); }); + it('should pick a Pipe defined in `declarations` over imported Pipes', () => { + @Pipe({name: 'number'}) + class PipeA implements PipeTransform { + transform(value: any) { return `PipeA: ${value}`; } + } + + @NgModule({ + declarations: [PipeA], + exports: [PipeA], + }) + class ModuleA { + } + + @Pipe({name: 'number'}) + class PipeB implements PipeTransform { + transform(value: any) { return `PipeB: ${value}`; } + } + + @Component({ + selector: 'app', + template: '{{ count | number }}', + }) + class App { + count = 10; + } + + TestBed.configureTestingModule({ + imports: [ModuleA], + declarations: [PipeB, App], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('PipeB: 10'); + }); + + it('should respect imported module order when selecting Pipe (last imported Pipe is used)', + () => { + @Pipe({name: 'number'}) + class PipeA implements PipeTransform { + transform(value: any) { return `PipeA: ${value}`; } + } + + @NgModule({ + declarations: [PipeA], + exports: [PipeA], + }) + class ModuleA { + } + + @Pipe({name: 'number'}) + class PipeB implements PipeTransform { + transform(value: any) { return `PipeB: ${value}`; } + } + + @NgModule({ + declarations: [PipeB], + exports: [PipeB], + }) + class ModuleB { + } + + @Component({ + selector: 'app', + template: '{{ count | number }}', + }) + class App { + count = 10; + } + + TestBed.configureTestingModule({ + imports: [ModuleA, ModuleB], + declarations: [App], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('PipeB: 10'); + }); + it('should do nothing when no change', () => { let calls: any[] = [];