Skip to content
Permalink
Browse files

fix(ngcc): do not output duplicate ɵprov properties (#34085)

Previously, the Angular AOT compiler would always add a
`ɵprov` to injectables. But in ngcc this resulted in duplicate `ɵprov`
properties since published libraries already have this property.

Now in ngtsc, trying to add a duplicate `ɵprov` property is an error,
while in ngcc the additional property is silently not added.

// FW-1750

PR Close #34085
  • Loading branch information
petebacondarwin authored and mhevery committed Nov 27, 2019
1 parent 955a312 commit 5a8d25dfc6c1e1977fadca42aced72d4f8d3d88d
@@ -12,7 +12,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 2987,
"main-es2015": 464734,
"main-es2015": 463671,
"polyfills-es2015": 52503
}
}
@@ -39,7 +39,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 2289,
"main-es2015": 267979,
"main-es2015": 267389,
"polyfills-es2015": 36808,
"5-es2015": 751
}
@@ -49,7 +49,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 2289,
"main-es2015": 227232,
"main-es2015": 226528,
"polyfills-es2015": 36808,
"5-es2015": 779
}
@@ -96,7 +96,7 @@ export class DecorationAnalyzer {
this.isCore, /* annotateForClosureCompiler */ false),
new InjectableDecoratorHandler(
this.reflectionHost, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore,
/* strictCtorDeps */ false),
/* strictCtorDeps */ false, /* errorOnDuplicateProv */ false),
new NgModuleDecoratorHandler(
this.reflectionHost, this.evaluator, this.fullMetaReader, this.fullRegistry,
this.scopeRegistry, this.referencesRegistry, this.isCore, /* routeAnalyzer */ null,
@@ -175,6 +175,36 @@ runInEachFileSystem(() => {
expect(jsContents).toMatch(/\bvar _c0 =/);
});

it('should add ɵfac but not duplicate ɵprov properties on injectables', () => {
compileIntoFlatEs5Package('test-package', {
'/index.ts': `
import {Injectable, ɵɵdefineInjectable} from '@angular/core';
export const TestClassToken = 'TestClassToken';
@Injectable({providedIn: 'module'})
export class TestClass {
static ɵprov = ɵɵdefineInjectable({ factory: () => {}, token: TestClassToken, providedIn: "module" });
}
`,
});

const before = fs.readFile(_(`/node_modules/test-package/index.js`));
const originalProp = /ɵprov[^;]+/.exec(before) ![0];
mainNgcc({
basePath: '/node_modules',
targetEntryPointPath: 'test-package',
propertiesToConsider: ['main'],
});
const after = fs.readFile(_(`/node_modules/test-package/index.js`));

expect(before).toContain(originalProp);
expect(countOccurrences(before, 'ɵprov')).toEqual(1);
expect(countOccurrences(before, 'ɵfac')).toEqual(0);

expect(after).toContain(originalProp);
expect(countOccurrences(after, 'ɵprov')).toEqual(1);
expect(countOccurrences(after, 'ɵfac')).toEqual(1);
});

it('should add generic type for ModuleWithProviders and generate exports for private modules',
() => {
compileIntoApf('test-package', {
@@ -1231,3 +1261,12 @@ runInEachFileSystem(() => {
}
});
});

function countOccurrences(haystack: string, needle: string): number {
const regex = new RegExp(needle, 'g');
let count = 0;
while (regex.exec(haystack)) {
count++;
}
return count;
}
@@ -32,7 +32,14 @@ export class InjectableDecoratorHandler implements
DecoratorHandler<InjectableHandlerData, Decorator> {
constructor(
private reflector: ReflectionHost, private defaultImportRecorder: DefaultImportRecorder,
private isCore: boolean, private strictCtorDeps: boolean) {}
private isCore: boolean, private strictCtorDeps: boolean,
/**
* What to do if the injectable already contains a ɵprov property.
*
* If true then an error diagnostic is reported.
* If false then there is no error and a new ɵprov property is not added.
*/
private errorOnDuplicateProv = true) {}

readonly precedence = HandlerPrecedence.SHARED;

@@ -93,11 +100,18 @@ export class InjectableDecoratorHandler implements
results.push(factoryRes);
}

results.push({
name: 'ɵprov',
initializer: res.expression, statements,
type: res.type,
});
const ɵprov = this.reflector.getMembersOfClass(node).find(member => member.name === 'ɵprov');
ifprov !== undefined && this.errorOnDuplicateProv) {
throw new FatalDiagnosticError(
ErrorCode.INJECTABLE_DUPLICATE_PROV, ɵprov.nameNode || ɵprov.node || node,
'Injectables cannot contain a static ɵprov property, because the compiler is going to generate one.');
}

ifprov === undefined) {
// Only add a new ɵprov if there is not one already
results.push({name: 'ɵprov', initializer: res.expression, statements, type: res.type});
}


return results;
}
@@ -0,0 +1,87 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {ErrorCode, FatalDiagnosticError, ngErrorCode} from '../../diagnostics';
import {absoluteFrom} from '../../file_system';
import {runInEachFileSystem} from '../../file_system/testing';
import {NOOP_DEFAULT_IMPORT_RECORDER} from '../../imports';
import {TypeScriptReflectionHost, isNamedClassDeclaration} from '../../reflection';
import {getDeclaration, makeProgram} from '../../testing';
import {InjectableDecoratorHandler} from '../src/injectable';

runInEachFileSystem(() => {
describe('InjectableDecoratorHandler', () => {
describe('compile()', () => {
it('should produce a diagnostic when injectable already has a static ɵprov property (with errorOnDuplicateProv true)',
() => {
const {handler, TestClass, ɵprov, analysis} =
setupHandler(/* errorOnDuplicateProv */ true);
try {
handler.compile(TestClass, analysis);
return fail('Compilation should have failed');
} catch (err) {
if (!(err instanceof FatalDiagnosticError)) {
return fail('Error should be a FatalDiagnosticError');
}
const diag = err.toDiagnostic();
expect(diag.code).toEqual(ngErrorCode(ErrorCode.INJECTABLE_DUPLICATE_PROV));
expect(diag.file.fileName.endsWith('entry.ts')).toBe(true);
expect(diag.start).toBeprov.nameNode !.getStart());
}
});

it('should not add new ɵprov property when injectable already has one (with errorOnDuplicateProv false)',
() => {
const {handler, TestClass, ɵprov, analysis} =
setupHandler(/* errorOnDuplicateProv */ false);
const res = handler.compile(TestClass, analysis);
expect(res).not.toContain(jasmine.objectContaining({name: 'ɵprov'}));
});
});

});
});

function setupHandler(errorOnDuplicateProv: boolean) {
const ENTRY_FILE = absoluteFrom('/entry.ts');
const ANGULAR_CORE = absoluteFrom('/node_modules/@angular/core/index.d.ts');
const {program} = makeProgram([
{
name: ANGULAR_CORE,
contents: 'export const Injectable: any; export const ɵɵdefineInjectable: any',
},
{
name: ENTRY_FILE,
contents: `
import {Injectable, ɵɵdefineInjectable} from '@angular/core';
export const TestClassToken = 'TestClassToken';
@Injectable({providedIn: 'module'})
export class TestClass {
static ɵprov = ɵɵdefineInjectable({ factory: () => {}, token: TestClassToken, providedIn: "module" });
}`
},
]);
const checker = program.getTypeChecker();
const reflectionHost = new TypeScriptReflectionHost(checker);
const handler = new InjectableDecoratorHandler(
reflectionHost, NOOP_DEFAULT_IMPORT_RECORDER, /* isCore */ false,
/* strictCtorDeps */ false, errorOnDuplicateProv);
const TestClass = getDeclaration(program, ENTRY_FILE, 'TestClass', isNamedClassDeclaration);
const ɵprov = reflectionHost.getMembersOfClass(TestClass).find(member => member.name === 'ɵprov');
ifprov === undefined) {
throw new Error('TestClass did not contain a `ɵprov` member');
}
const detected = handler.detect(TestClass, reflectionHost.getDecoratorsOfDeclaration(TestClass));
if (detected === undefined) {
throw new Error('Failed to recognize TestClass');
}
const {analysis} = handler.analyze(TestClass, detected.metadata);
if (analysis === undefined) {
throw new Error('Failed to analyze TestClass');
}
return {handler, TestClass, ɵprov, analysis};
}
@@ -94,6 +94,11 @@ export enum ErrorCode {
* No matching pipe was found for a
*/
MISSING_PIPE = 8004,

/**
* An injectable already has a `ɵprov` property.
*/
INJECTABLE_DUPLICATE_PROV = 9001
}

export function ngErrorCode(code: ErrorCode): number {

0 comments on commit 5a8d25d

Please sign in to comment.
You can’t perform that action at this time.