fix(compiler): throw descriptive error meesage for invalid NgModule providers (#10947)

Fixes #10714

modules/@angular/compiler/src/metadata_resolver.ts

@@ -146,8 +146,8 @@ export class CompileMetadataResolver {
         changeDetectionStrategy = cmpMeta.changeDetection;
         if (isPresent(dirMeta.viewProviders)) {
           viewProviders = this.getProvidersMetadata(
-              verifyNonBlankProviders(directiveType, dirMeta.viewProviders, 'viewProviders'),
+              dirMeta.viewProviders, entryComponentMetadata,
-              entryComponentMetadata);
+              `viewProviders for "${stringify(directiveType)}"`);
         }
         moduleUrl = componentModuleUrl(this._reflector, directiveType, cmpMeta);
         if (cmpMeta.entryComponents) {
@@ -169,8 +169,8 @@ export class CompileMetadataResolver {
       var providers: Array<cpl.CompileProviderMetadata|cpl.CompileTypeMetadata|any[]> = [];
       if (isPresent(dirMeta.providers)) {
         providers = this.getProvidersMetadata(
-            verifyNonBlankProviders(directiveType, dirMeta.providers, 'providers'),
+            dirMeta.providers, entryComponentMetadata,
-            entryComponentMetadata);
+            `providers for "${stringify(directiveType)}"`);
       }
       var queries: cpl.CompileQueryMetadata[] = [];
       var viewQueries: cpl.CompileQueryMetadata[] = [];
@@ -227,8 +227,9 @@ export class CompileMetadataResolver {
             const moduleWithProviders: ModuleWithProviders = importedType;
             importedModuleType = moduleWithProviders.ngModule;
             if (moduleWithProviders.providers) {
-              providers.push(
+              providers.push(...this.getProvidersMetadata(
-                  ...this.getProvidersMetadata(moduleWithProviders.providers, entryComponents));
+                  moduleWithProviders.providers, entryComponents,
+                  `provider for the NgModule '${stringify(importedModuleType)}'`));
             }
           }
           if (importedModuleType) {
@@ -295,7 +296,9 @@ export class CompileMetadataResolver {
       // The providers of the module have to go last
       // so that they overwrite any other provider we already added.
       if (meta.providers) {
-        providers.push(...this.getProvidersMetadata(meta.providers, entryComponents));
+        providers.push(...this.getProvidersMetadata(
+            meta.providers, entryComponents,
+            `provider for the NgModule '${stringify(moduleType)}'`));
       }
       if (meta.entryComponents) {
         entryComponents.push(
@@ -550,17 +553,18 @@ export class CompileMetadataResolver {
     return compileToken;
   }
 
-  getProvidersMetadata(providers: Provider[], targetEntryComponents: cpl.CompileTypeMetadata[]):
+  getProvidersMetadata(
-      Array<cpl.CompileProviderMetadata|cpl.CompileTypeMetadata|any[]> {
+      providers: Provider[], targetEntryComponents: cpl.CompileTypeMetadata[],
+      debugInfo?: string): Array<cpl.CompileProviderMetadata|cpl.CompileTypeMetadata|any[]> {
     const compileProviders: Array<cpl.CompileProviderMetadata|cpl.CompileTypeMetadata|any[]> = [];
-    providers.forEach((provider: any) => {
+    providers.forEach((provider: any, providerIdx: number) => {
       provider = resolveForwardRef(provider);
       if (provider && typeof provider == 'object' && provider.hasOwnProperty('provide')) {
         provider = new cpl.ProviderMeta(provider.provide, provider);
       }
       let compileProvider: cpl.CompileProviderMetadata|cpl.CompileTypeMetadata|any[];
       if (isArray(provider)) {
-        compileProvider = this.getProvidersMetadata(provider, targetEntryComponents);
+        compileProvider = this.getProvidersMetadata(provider, targetEntryComponents, debugInfo);
       } else if (provider instanceof cpl.ProviderMeta) {
         let tokenMeta = this.getTokenMetadata(provider.token);
         if (tokenMeta.equalsTo(identifierToken(Identifiers.ANALYZE_FOR_ENTRY_COMPONENTS))) {
@@ -571,8 +575,22 @@ export class CompileMetadataResolver {
       } else if (isValidType(provider)) {
         compileProvider = this.getTypeMetadata(provider, staticTypeModuleUrl(provider));
       } else {
+        let providersInfo = (<string[]>providers.reduce(
+                                 (soFar: string[], seenProvider: any, seenProviderIdx: number) => {
+                                   if (seenProviderIdx < providerIdx) {
+                                     soFar.push(`${stringify(seenProvider)}`);
+                                   } else if (seenProviderIdx == providerIdx) {
+                                     soFar.push(`?${stringify(seenProvider)}?`);
+                                   } else if (seenProviderIdx == providerIdx + 1) {
+                                     soFar.push('...');
+                                   }
+                                   return soFar;
+                                 },
+                                 []))
+                                .join(', ');
+
         throw new BaseException(
-            `Invalid provider - only instances of Provider and Type are allowed, got: ${stringify(provider)}`);
+            `Invalid ${debugInfo ? debugInfo : 'provider'} - only instances of Provider and Type are allowed, got: [${providersInfo}]`);
       }
       if (compileProvider) {
         compileProviders.push(compileProvider);
@@ -696,23 +714,6 @@ function flattenArray(tree: any[], out: Array<any> = []): Array<any> {
   return out;
 }
 
-function verifyNonBlankProviders(
-    directiveType: Type<any>, providersTree: any[], providersType: string): any[] {
-  var flat: any[] = [];
-  var errMsg: string;
-
-  flattenArray(providersTree, flat);
-  for (var i = 0; i < flat.length; i++) {
-    if (isBlank(flat[i])) {
-      errMsg = flat.map(provider => isBlank(provider) ? '?' : stringify(provider)).join(', ');
-      throw new BaseException(
-          `One or more of ${providersType} for "${stringify(directiveType)}" were not defined: [${errMsg}].`);
-    }
-  }
-
-  return providersTree;
-}
-
 function isValidType(value: any): boolean {
   return cpl.isStaticSymbol(value) || (value instanceof Type);
 }

modules/@angular/compiler/test/metadata_resolver_spec.ts

@@ -130,14 +130,14 @@ export function main() {
          inject([CompileMetadataResolver], (resolver: CompileMetadataResolver) => {
            expect(() => resolver.getDirectiveMetadata(MyBrokenComp3))
                .toThrowError(
-                   `One or more of providers for "MyBrokenComp3" were not defined: [?, SimpleService, ?].`);
+                   `Invalid providers for "MyBrokenComp3" - only instances of Provider and Type are allowed, got: [SimpleService, ?null?, ...]`);
          }));
 
       it('should throw with descriptive error message when one of viewProviders is not present',
          inject([CompileMetadataResolver], (resolver: CompileMetadataResolver) => {
            expect(() => resolver.getDirectiveMetadata(MyBrokenComp4))
                .toThrowError(
-                   `One or more of viewProviders for "MyBrokenComp4" were not defined: [?, SimpleService, ?].`);
+                   `Invalid viewProviders for "MyBrokenComp4" - only instances of Provider and Type are allowed, got: [?null?, ...]`);
          }));
 
       it('should throw an error when the interpolation config has invalid symbols',
@@ -220,7 +220,7 @@ class MyBrokenComp2 {
 class SimpleService {
 }
 
-@Component({selector: 'my-broken-comp', template: '', providers: [null, SimpleService, [null]]})
+@Component({selector: 'my-broken-comp', template: '', providers: [SimpleService, null, [null]]})
 class MyBrokenComp3 {
 }
 

modules/@angular/core/test/linker/ng_module_integration_spec.ts

@@ -732,7 +732,13 @@ function declareTests({useJit}: {useJit: boolean}) {
       it('should throw when given invalid providers', () => {
         expect(() => createInjector(<any>['blah']))
             .toThrowError(
-                'Invalid provider - only instances of Provider and Type are allowed, got: blah');
+                `Invalid provider for the NgModule 'SomeModule' - only instances of Provider and Type are allowed, got: [?blah?]`);
+      });
+
+      it('should throw when given blank providers', () => {
+        expect(() => createInjector(<any>[null, {provide: 'token', useValue: 'value'}]))
+            .toThrowError(
+                `Invalid provider for the NgModule 'SomeModule' - only instances of Provider and Type are allowed, got: [?null?, ...]`);
       });
 
       it('should provide itself', () => {
@@ -1104,6 +1110,20 @@ function declareTests({useJit}: {useJit: boolean}) {
              const injector = createModule(SomeModule).injector;
              expect(injector.get('token1')).toBe('imported2');
            });
+
+        it('should throw when given invalid providers in an imported ModuleWithProviders', () => {
+          @NgModule()
+          class ImportedModule1 {
+          }
+
+          @NgModule({imports: [{ngModule: ImportedModule1, providers: [<any>'broken']}]})
+          class SomeModule {
+          }
+
+          expect(() => createModule(SomeModule).injector)
+              .toThrowError(
+                  `Invalid provider for the NgModule 'ImportedModule1' - only instances of Provider and Type are allowed, got: [?broken?]`);
+        });
       });
     });
   });