Skip to content

Clean up Build Optimizer whitelists #15239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 2, 2019
Merged

Clean up Build Optimizer whitelists #15239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
757e685
fix(@angular-devkit/build-optimizer): scrub previously whitelisted an…
filipesilva Aug 2, 2019
e366cee
fix(@angular-devkit/build-optimizer): scrub all metadata form `@angul…
filipesilva Aug 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 35 additions & 93 deletions packages/angular_devkit/build_optimizer/src/transforms/scrub-file.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,33 +22,6 @@ export function testScrubFile(content: string) {
return markers.some((marker) => content.indexOf(marker) !== -1);
}

// Don't remove `ctorParameters` from these.
const platformWhitelist = [
'PlatformRef_',
'TestabilityRegistry',
'Console',
'BrowserPlatformLocation',
];

const angularSpecifiers = [
// Class level decorators.
'Component',
'Directive',
'Injectable',
'NgModule',
'Pipe',

// Property level decorators.
'ContentChild',
'ContentChildren',
'HostBinding',
'HostListener',
'Input',
'Output',
'ViewChild',
'ViewChildren',
];

export function getScrubFileTransformer(program: ts.Program): ts.TransformerFactory<ts.SourceFile> {
return scrubFileTransformer(program.getTypeChecker(), false);
}
Expand Down Expand Up @@ -89,8 +62,7 @@ function scrubFileTransformer(checker: ts.TypeChecker, isAngularCoreFile: boolea
if (isPropDecoratorAssignmentExpression(exprStmt)) {
nodes.push(...pickPropDecorationNodesToRemove(exprStmt, ngMetadata, checker));
}
if (isCtorParamsAssignmentExpression(exprStmt)
&& !isCtorParamsWhitelistedService(exprStmt)) {
if (isCtorParamsAssignmentExpression(exprStmt)) {
nodes.push(node);
}
}
Expand Down Expand Up @@ -120,33 +92,18 @@ export function expect<T extends ts.Node>(node: ts.Node, kind: ts.SyntaxKind): T
return node as T;
}

function nameOfSpecifier(node: ts.ImportSpecifier): string {
return node.name && node.name.text || '<unknown>';
}

function findAngularMetadata(node: ts.Node, isAngularCoreFile: boolean): ts.Node[] {
let specs: ts.Node[] = [];
const specs: ts.Node[] = [];
// Find all specifiers from imports of `@angular/core`.
ts.forEachChild(node, (child) => {
if (child.kind === ts.SyntaxKind.ImportDeclaration) {
const importDecl = child as ts.ImportDeclaration;
if (isAngularCoreImport(importDecl, isAngularCoreFile)) {
specs.push(...collectDeepNodes<ts.ImportSpecifier>(node, ts.SyntaxKind.ImportSpecifier)
.filter((spec) => isAngularCoreSpecifier(spec)));
specs.push(...collectDeepNodes<ts.ImportSpecifier>(importDecl, ts.SyntaxKind.ImportSpecifier));
}
}
});

// Check if the current module contains all know `@angular/core` specifiers.
// If it does, we assume it's a `@angular/core` FESM.
if (isAngularCoreFile) {
const localDecl = findAllDeclarations(node)
.filter((decl) => angularSpecifiers.indexOf((decl.name as ts.Identifier).text) !== -1);
if (localDecl.length === angularSpecifiers.length) {
specs = specs.concat(localDecl);
}
}

return specs;
}

Expand Down Expand Up @@ -186,10 +143,6 @@ function isAngularCoreImport(node: ts.ImportDeclaration, isAngularCoreFile: bool
return false;
}

function isAngularCoreSpecifier(node: ts.ImportSpecifier): boolean {
return angularSpecifiers.indexOf(nameOfSpecifier(node)) !== -1;
}

// Check if assignment is `Clazz.decorators = [...];`.
function isDecoratorAssignmentExpression(exprStmt: ts.ExpressionStatement): boolean {
if (exprStmt.expression.kind !== ts.SyntaxKind.BinaryExpression) {
Expand Down Expand Up @@ -369,14 +322,6 @@ function isCtorParamsAssignmentExpression(exprStmt: ts.ExpressionStatement): boo
return true;
}

function isCtorParamsWhitelistedService(exprStmt: ts.ExpressionStatement): boolean {
const expr = exprStmt.expression as ts.BinaryExpression;
const propAccess = expr.left as ts.PropertyAccessExpression;
const serviceId = propAccess.expression as ts.Identifier;

return platformWhitelist.indexOf(serviceId.text) !== -1;
}

// Remove Angular decorators from`Clazz.decorators = [...];`, or expression itself if all are
// removed.
function pickDecorationNodesToRemove(
Expand Down Expand Up @@ -407,7 +352,6 @@ function pickDecorateNodesToRemove(
): ts.Node[] {

const expr = expect<ts.BinaryExpression>(exprStmt.expression, ts.SyntaxKind.BinaryExpression);
const classId = expect<ts.Identifier>(expr.left, ts.SyntaxKind.Identifier);
let callExpr: ts.CallExpression;

if (expr.right.kind === ts.SyntaxKind.CallExpression) {
Expand Down Expand Up @@ -435,42 +379,40 @@ function pickDecorateNodesToRemove(
return identifierIsMetadata(id, ngMetadata, checker);
});

// Only remove constructor parameter metadata on non-whitelisted classes.
if (platformWhitelist.indexOf(classId.text) === -1) {
// Remove __metadata calls of type 'design:paramtypes'.
const metadataCalls = elements.filter((el) => {
if (!isTslibHelper(el, '__metadata', tslibImports, checker)) {
return false;
}
if (el.arguments.length < 2) {
return false;
}
if (el.arguments[0].kind !== ts.SyntaxKind.StringLiteral) {
return false;
}
const metadataTypeId = el.arguments[0] as ts.StringLiteral;
if (metadataTypeId.text !== 'design:paramtypes') {
return false;
}

return true;
});
// Remove all __param calls.
const paramCalls = elements.filter((el) => {
if (!isTslibHelper(el, '__param', tslibImports, checker)) {
return false;
}
if (el.arguments.length != 2) {
return false;
}
if (el.arguments[0].kind !== ts.SyntaxKind.NumericLiteral) {
return false;
}
// Remove __metadata calls of type 'design:paramtypes'.
const metadataCalls = elements.filter((el) => {
if (!isTslibHelper(el, '__metadata', tslibImports, checker)) {
return false;
}
if (el.arguments.length < 2) {
return false;
}
if (el.arguments[0].kind !== ts.SyntaxKind.StringLiteral) {
return false;
}
const metadataTypeId = el.arguments[0] as ts.StringLiteral;
if (metadataTypeId.text !== 'design:paramtypes') {
return false;
}

return true;
});
ngDecoratorCalls.push(...metadataCalls, ...paramCalls);
}
return true;
});
// Remove all __param calls.
const paramCalls = elements.filter((el) => {
if (!isTslibHelper(el, '__param', tslibImports, checker)) {
return false;
}
if (el.arguments.length != 2) {
return false;
}
if (el.arguments[0].kind !== ts.SyntaxKind.NumericLiteral) {
return false;
}

return true;
});
ngDecoratorCalls.push(...metadataCalls, ...paramCalls);

// If all decorators are metadata decorators then return the whole `Class = __decorate([...])'`
// statement so that it is removed in entirety
Expand Down
26 changes: 0 additions & 26 deletions packages/angular_devkit/build_optimizer/src/transforms/scrub-file_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,23 +188,6 @@ describe('scrub-file', () => {
expect(tags.oneLine`${transform(input)}`).toEqual(tags.oneLine`${output}`);
});

it('doesn\t remove constructor parameter metadata for whitelisted classes', () => {
const input = tags.stripIndent`
import { ElementRef } from '@angular/core';
import { LibService } from 'another-lib';
var BrowserPlatformLocation = (function () {
function BrowserPlatformLocation() { }
BrowserPlatformLocation = __decorate([
__metadata("design:paramtypes", [ElementRef, LibService])
], BrowserPlatformLocation);
return BrowserPlatformLocation;
}());
`;

expect(testScrubFile(input)).toBeTruthy();
expect(tags.oneLine`${transform(input)}`).toEqual(tags.oneLine`${input}`);
});

it('removes only Angular decorators calls in __decorate', () => {
const output = tags.stripIndent`
import { Component } from '@angular/core';
Expand Down Expand Up @@ -628,14 +611,5 @@ describe('scrub-file', () => {

expect(tags.oneLine`${transform(input)}`).toEqual(tags.oneLine`${output}`);
});

it('doesn\'t remove constructor parameters from whitelisted classes', () => {
const input = tags.stripIndent`
${clazz.replace('Clazz', 'PlatformRef_')}
PlatformRef_.ctorParameters = function () { return []; };
`;

expect(tags.oneLine`${transform(input)}`).toEqual(tags.oneLine`${input}`);
});
});
});