diff --git a/packages/core/src/render3/jit/module.ts b/packages/core/src/render3/jit/module.ts index a0f6145254..1ac60246d3 100644 --- a/packages/core/src/render3/jit/module.ts +++ b/packages/core/src/render3/jit/module.ts @@ -109,6 +109,11 @@ export function compileNgModuleDefs( configurable: true, get: () => { if (ngModuleDef === null) { + if (ngDevMode && ngModule.imports && ngModule.imports.indexOf(moduleType) > -1) { + // We need to assert this immediately, because allowing it to continue will cause it to + // go into an infinite loop before we've reached the point where we throw all the errors. + throw new Error(`'${stringifyForError(moduleType)}' module can't import itself`); + } ngModuleDef = getCompilerFacade().compileNgModule( angularCoreEnv, `ng:///${moduleType.name}/ngModuleDef.js`, { type: moduleType, @@ -156,17 +161,28 @@ export function compileNgModuleDefs( } function verifySemanticsOfNgModuleDef( - moduleType: NgModuleType, allowDuplicateDeclarationsInRoot: boolean): void { + moduleType: NgModuleType, allowDuplicateDeclarationsInRoot: boolean, + importingModule?: NgModuleType): void { if (verifiedNgModule.get(moduleType)) return; verifiedNgModule.set(moduleType, true); moduleType = resolveForwardRef(moduleType); - const ngModuleDef = getNgModuleDef(moduleType, true); + let ngModuleDef: NgModuleDef; + if (importingModule) { + ngModuleDef = getNgModuleDef(moduleType) !; + if (!ngModuleDef) { + throw new Error( + `Unexpected value '${moduleType.name}' imported by the module '${importingModule.name}'. Please add a @NgModule annotation.`); + } + } else { + ngModuleDef = getNgModuleDef(moduleType, true); + } const errors: string[] = []; const declarations = maybeUnwrapFn(ngModuleDef.declarations); const imports = maybeUnwrapFn(ngModuleDef.imports); - flatten(imports) - .map(unwrapModuleWithProvidersImports) - .forEach(mod => verifySemanticsOfNgModuleDef(mod, false)); + flatten(imports).map(unwrapModuleWithProvidersImports).forEach(mod => { + verifySemanticsOfNgModuleImport(mod, moduleType); + verifySemanticsOfNgModuleDef(mod, false, moduleType); + }); const exports = maybeUnwrapFn(ngModuleDef.exports); declarations.forEach(verifyDeclarationsHaveDefinitions); const combinedDeclarations: Type[] = [ @@ -180,9 +196,10 @@ function verifySemanticsOfNgModuleDef( const ngModule = getAnnotation(moduleType, 'NgModule'); if (ngModule) { ngModule.imports && - flatten(ngModule.imports) - .map(unwrapModuleWithProvidersImports) - .forEach(mod => verifySemanticsOfNgModuleDef(mod, false)); + flatten(ngModule.imports).map(unwrapModuleWithProvidersImports).forEach(mod => { + verifySemanticsOfNgModuleImport(mod, moduleType); + verifySemanticsOfNgModuleDef(mod, false, moduleType); + }); ngModule.bootstrap && ngModule.bootstrap.forEach(verifyCorrectBootstrapType); ngModule.bootstrap && ngModule.bootstrap.forEach(verifyComponentIsPartOfNgModule); ngModule.entryComponents && ngModule.entryComponents.forEach(verifyComponentIsPartOfNgModule); @@ -260,6 +277,20 @@ function verifySemanticsOfNgModuleDef( } } } + + function verifySemanticsOfNgModuleImport(type: Type, importingModule: Type) { + type = resolveForwardRef(type); + + if (getComponentDef(type) || getDirectiveDef(type)) { + throw new Error( + `Unexpected directive '${type.name}' imported by the module '${importingModule.name}'. Please add a @NgModule annotation.`); + } + + if (getPipeDef(type)) { + throw new Error( + `Unexpected pipe '${type.name}' imported by the module '${importingModule.name}'. Please add a @NgModule annotation.`); + } + } } function unwrapModuleWithProvidersImports( diff --git a/packages/core/test/acceptance/integration_spec.ts b/packages/core/test/acceptance/integration_spec.ts index cd4942877e..af675d3f38 100644 --- a/packages/core/test/acceptance/integration_spec.ts +++ b/packages/core/test/acceptance/integration_spec.ts @@ -6,12 +6,12 @@ * found in the LICENSE file at https://angular.io/license */ import {CommonModule} from '@angular/common'; -import {Component, ContentChild, Directive, ElementRef, EventEmitter, HostBinding, HostListener, Input, NgModule, OnInit, Output, QueryList, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core'; +import {Component, ContentChild, Directive, ElementRef, EventEmitter, HostBinding, HostListener, Input, NgModule, OnInit, Output, Pipe, QueryList, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core'; import {ngDevModeResetPerfCounters} from '@angular/core/src/util/ng_dev_mode'; import {TestBed} from '@angular/core/testing'; import {By} from '@angular/platform-browser'; import {expect} from '@angular/platform-browser/testing/src/matchers'; -import {onlyInIvy} from '@angular/private/testing'; +import {ivyEnabled, onlyInIvy} from '@angular/private/testing'; describe('acceptance integration tests', () => { function stripHtmlComments(str: string) { return str.replace(//g, ''); } @@ -1401,6 +1401,111 @@ describe('acceptance integration tests', () => { }); }); + describe('NgModule assertions', () => { + it('should throw with descriptive error message when a module imports itself', () => { + @Component({template: ''}) + class FixtureComponent { + } + + @NgModule({imports: [SomeModule], declarations: [FixtureComponent]}) + class SomeModule { + } + expect(() => { + TestBed.configureTestingModule({imports: [SomeModule]}).createComponent(FixtureComponent); + }).toThrowError(`'SomeModule' module can't import itself`); + }); + + it('should throw with descriptive error message when a directive is passed to imports', () => { + @Component({template: ''}) + class SomeComponent { + } + + @NgModule({imports: [SomeComponent]}) + class ModuleWithImportedComponent { + } + expect(() => { + TestBed.configureTestingModule({imports: [ModuleWithImportedComponent]}) + .createComponent(SomeComponent); + }) + .toThrowError( + `Unexpected directive 'SomeComponent' imported by the module 'ModuleWithImportedComponent'. Please add a @NgModule annotation.`); + }); + + it('should throw with descriptive error message when a pipe is passed to imports', () => { + @Component({template: ''}) + class FixtureComponent { + } + @Pipe({name: 'somePipe'}) + class SomePipe { + } + @NgModule({imports: [SomePipe], declarations: [FixtureComponent]}) + class ModuleWithImportedPipe { + } + expect(() => { + TestBed.configureTestingModule({imports: [ModuleWithImportedPipe]}) + .createComponent(FixtureComponent); + }) + .toThrowError( + `Unexpected pipe 'SomePipe' imported by the module 'ModuleWithImportedPipe'. Please add a @NgModule annotation.`); + }); + + it('should throw with descriptive error message when a module is passed to declarations', () => { + @Component({template: ''}) + class FixtureComponent { + } + @NgModule({}) + class SomeModule { + } + @NgModule({declarations: [SomeModule, FixtureComponent]}) + class ModuleWithDeclaredModule { + } + + // The error is almost the same in Ivy and ViewEngine, however since Ivy's + // message is more correct it doesn't make sense to align it ViewEngine. + const expectedErrorMessage = ivyEnabled ? + `Unexpected value 'SomeModule' declared by the module 'ModuleWithDeclaredModule'. Please add a @Pipe/@Directive/@Component annotation.` : + `Unexpected module 'SomeModule' declared by the module 'ModuleWithDeclaredModule'. Please add a @Pipe/@Directive/@Component annotation.`; + + expect(() => { + TestBed.configureTestingModule({imports: [ModuleWithDeclaredModule]}) + .createComponent(FixtureComponent); + }).toThrowError(expectedErrorMessage); + }); + + it('should throw with descriptive error message when a declaration is missing annotation', () => { + @Component({template: ''}) + class FixtureComponent { + } + class SomeClass {} + @NgModule({declarations: [SomeClass, FixtureComponent]}) + class SomeModule { + } + expect(() => { + TestBed.configureTestingModule({imports: [SomeModule]}).createComponent(FixtureComponent); + }) + .toThrowError( + `Unexpected value 'SomeClass' declared by the module 'SomeModule'. Please add a @Pipe/@Directive/@Component annotation.`); + }); + + it('should throw with descriptive error message when an imported module is missing annotation', + () => { + @Component({template: ''}) + class FixtureComponent { + } + class SomeModule {} + @NgModule({imports: [SomeModule], declarations: [FixtureComponent]}) + class ModuleWithImportedModule { + } + expect(() => { + TestBed.configureTestingModule({imports: [ModuleWithImportedModule]}) + .createComponent(FixtureComponent); + }) + .toThrowError( + `Unexpected value 'SomeModule' imported by the module 'ModuleWithImportedModule'. Please add a @NgModule annotation.`); + }); + + }); + it('should only call inherited host listeners once', () => { let clicks = 0;