From dfb8d21ef424dd3167ac41f0c575833fa119d697 Mon Sep 17 00:00:00 2001 From: Chuck Jazdzewski Date: Tue, 26 Sep 2017 13:40:47 -0700 Subject: [PATCH] feat(compiler): enabled strict checking of parameters to an `@Injectable` (#19412) Added the compiler options `strictInjectionParameters` that defaults to `false`. If enabled the compiler will report errors for parameters of an `@Injectable` that cannot be determined instead of generating a warning. This is planned to be switched to default to `true` for Angular 6.0. --- packages/compiler-cli/src/transformers/api.ts | 8 +++++++ packages/compiler/src/aot/compiler_factory.ts | 1 + packages/compiler/src/aot/compiler_options.ts | 1 + packages/compiler/src/config.ts | 6 +++-- packages/compiler/src/metadata_resolver.ts | 4 ++-- packages/compiler/test/aot/compiler_spec.ts | 22 ++++++++++++++++++- 6 files changed, 37 insertions(+), 5 deletions(-) diff --git a/packages/compiler-cli/src/transformers/api.ts b/packages/compiler-cli/src/transformers/api.ts index 36fc85b528..7829f956f0 100644 --- a/packages/compiler-cli/src/transformers/api.ts +++ b/packages/compiler-cli/src/transformers/api.ts @@ -52,6 +52,14 @@ export interface CompilerOptions extends ts.CompilerOptions { // Don't produce .ngfactory.ts or .ngstyle.ts files skipTemplateCodegen?: boolean; + // Always report errors when the type of a parameter supplied whose injection type cannot + // be determined. When this value option is not provided or is `false`, constructor + // parameters of classes marked with `@Injectable` whose type cannot be resolved will + // produce a warning. With this option `true`, they produce an error. When this option is + // not provided is treated as if it were `false`. In Angular 6.0, if this option is not + // provided, it will be treated as `true`. + strictInjectionParameters?: boolean; + // Whether to generate a flat module index of the given name and the corresponding // flat module metadata. This option is intended to be used when creating flat // modules similar to how `@angular/core` and `@angular/common` are packaged. diff --git a/packages/compiler/src/aot/compiler_factory.ts b/packages/compiler/src/aot/compiler_factory.ts index c794269b05..776688a975 100644 --- a/packages/compiler/src/aot/compiler_factory.ts +++ b/packages/compiler/src/aot/compiler_factory.ts @@ -69,6 +69,7 @@ export function createAotCompiler(compilerHost: AotCompilerHost, options: AotCom enableLegacyTemplate: options.enableLegacyTemplate === true, missingTranslation: options.missingTranslation, preserveWhitespaces: options.preserveWhitespaces, + strictInjectionParameters: options.strictInjectionParameters, }); const normalizer = new DirectiveNormalizer( {get: (url: string) => compilerHost.loadResource(url)}, urlResolver, htmlParser, config); diff --git a/packages/compiler/src/aot/compiler_options.ts b/packages/compiler/src/aot/compiler_options.ts index cf12e2b6a3..1149a4cddf 100644 --- a/packages/compiler/src/aot/compiler_options.ts +++ b/packages/compiler/src/aot/compiler_options.ts @@ -19,4 +19,5 @@ export interface AotCompilerOptions { preserveWhitespaces?: boolean; fullTemplateTypeCheck?: boolean; allowEmptyCodegenFiles?: boolean; + strictInjectionParameters?: boolean; } diff --git a/packages/compiler/src/config.ts b/packages/compiler/src/config.ts index 3be0286523..ed0b088899 100644 --- a/packages/compiler/src/config.ts +++ b/packages/compiler/src/config.ts @@ -21,17 +21,18 @@ export class CompilerConfig { public jitDevMode: boolean; public missingTranslation: MissingTranslationStrategy|null; public preserveWhitespaces: boolean; + public strictInjectionParameters: boolean; constructor( {defaultEncapsulation = ViewEncapsulation.Emulated, useJit = true, jitDevMode = false, - missingTranslation, enableLegacyTemplate, preserveWhitespaces}: { + missingTranslation, enableLegacyTemplate, preserveWhitespaces, strictInjectionParameters}: { defaultEncapsulation?: ViewEncapsulation, useJit?: boolean, jitDevMode?: boolean, missingTranslation?: MissingTranslationStrategy, enableLegacyTemplate?: boolean, preserveWhitespaces?: boolean, - fullTemplateTypeCheck?: boolean + strictInjectionParameters?: boolean, } = {}) { this.defaultEncapsulation = defaultEncapsulation; this.useJit = !!useJit; @@ -39,6 +40,7 @@ export class CompilerConfig { this.missingTranslation = missingTranslation || null; this.enableLegacyTemplate = enableLegacyTemplate === true; this.preserveWhitespaces = preserveWhitespacesDefault(noUndefined(preserveWhitespaces)); + this.strictInjectionParameters = strictInjectionParameters === true; } } diff --git a/packages/compiler/src/metadata_resolver.ts b/packages/compiler/src/metadata_resolver.ts index c30b84a132..b64968eebe 100644 --- a/packages/compiler/src/metadata_resolver.ts +++ b/packages/compiler/src/metadata_resolver.ts @@ -886,10 +886,10 @@ export class CompileMetadataResolver { dependenciesMetadata.map((dep) => dep ? stringifyType(dep.token) : '?').join(', '); const message = `Can't resolve all parameters for ${stringifyType(typeOrFunc)}: (${depsTokens}).`; - if (throwOnUnknownDeps) { + if (throwOnUnknownDeps || this._config.strictInjectionParameters) { this._reportError(syntaxError(message), typeOrFunc); } else { - this._console.warn(`Warning: ${message} This will become an error in Angular v5.x`); + this._console.warn(`Warning: ${message} This will become an error in Angular v6.x`); } } diff --git a/packages/compiler/test/aot/compiler_spec.ts b/packages/compiler/test/aot/compiler_spec.ts index 40f3d3003f..1c8ed4b24a 100644 --- a/packages/compiler/test/aot/compiler_spec.ts +++ b/packages/compiler/test/aot/compiler_spec.ts @@ -205,10 +205,30 @@ describe('compiler (unbundled Angular)', () => { const warnSpy = spyOn(console, 'warn'); compile([FILES, angularFiles]); expect(warnSpy).toHaveBeenCalledWith( - `Warning: Can't resolve all parameters for MyService in /app/app.ts: (?). This will become an error in Angular v5.x`); + `Warning: Can't resolve all parameters for MyService in /app/app.ts: (?). This will become an error in Angular v6.x`); }); + it('should error if not all arguments of an @Injectable class can be resolved if strictInjectionParamters is true', + () => { + const FILES: MockDirectory = { + app: { + 'app.ts': ` + import {Injectable} from '@angular/core'; + + @Injectable() + export class MyService { + constructor(a: boolean) {} + } + ` + } + }; + const warnSpy = spyOn(console, 'warn'); + expect(() => compile([FILES, angularFiles], {strictInjectionParameters: true})) + .toThrowError(`Can't resolve all parameters for MyService in /app/app.ts: (?).`); + expect(warnSpy).not.toHaveBeenCalled(); + }); + it('should be able to suppress a null access', () => { const FILES: MockDirectory = { app: {