Skip to content

Commit

Permalink
fix(@angular-devkit/build-optimizer): increase safety of code removal
Browse files Browse the repository at this point in the history 
This change lowers the potential for code to be errantly removed by the prefix functions and scrub file transformers.  Only known safe modules are used with the prefix functions transformer as it can easily remove required module level side effects (as opposed to global level side effects) such as `__decorate` calls.
The scrub file transformer will now keep metadata if non-Angular decorators are present. This allows libraries that use that information to continue to function.

Closes #14033
Closes #18621
  • Loading branch information
@clydin @filipesilva
clydin authored and filipesilva committed Oct 12, 2020
1 parent 3a9b512 commit 4b4822f
Show file tree
Hide file tree
Showing 5 changed files with 142 additions and 61 deletions.
39 changes: 16 additions & 23 deletions packages/angular_devkit/build_optimizer/src/build-optimizer/build-optimizer.ts
View file
Expand Up @@ -15,8 +15,7 @@ import {
import { getPrefixClassesTransformer, testPrefixClasses } from '../transforms/prefix-classes';
import { getPrefixFunctionsTransformer } from '../transforms/prefix-functions';
import {
getScrubFileTransformer,
getScrubFileTransformerForCore,
createScrubFileTransformerFactory,
testScrubFile,
} from '../transforms/scrub-file';
import { getWrapEnumsTransformer } from '../transforms/wrap-enums';
Expand Down Expand Up @@ -70,9 +69,8 @@ export interface BuildOptimizerOptions {
}

export function buildOptimizer(options: BuildOptimizerOptions): TransformJavascriptOutput {

const { inputFilePath, isAngularCoreFile } = options;
let { originalFilePath, content } = options;
const { inputFilePath } = options;
let { originalFilePath, content, isAngularCoreFile } = options;

if (!originalFilePath && inputFilePath) {
originalFilePath = inputFilePath;
Expand All @@ -94,39 +92,34 @@ export function buildOptimizer(options: BuildOptimizerOptions): TransformJavascr
};
}

let selectedGetScrubFileTransformer = getScrubFileTransformer;

if (
isAngularCoreFile === true ||
(isAngularCoreFile === undefined && originalFilePath && isKnownCoreFile(originalFilePath))
) {
selectedGetScrubFileTransformer = getScrubFileTransformerForCore;
if (isAngularCoreFile === undefined) {
isAngularCoreFile = !!originalFilePath && isKnownCoreFile(originalFilePath);
}

const hasSafeSideEffects = originalFilePath && isKnownSideEffectFree(originalFilePath);

// Determine which transforms to apply.
const getTransforms: TransformerFactoryCreator[] = [];

let typeCheck = false;
if (options.isSideEffectFree || originalFilePath && isKnownSideEffectFree(originalFilePath)) {
if (hasSafeSideEffects) {
// Angular modules have known safe side effects
getTransforms.push(
// getPrefixFunctionsTransformer is rather dangerous, apply only to known pure es5 modules.
// It will mark both `require()` calls and `console.log(stuff)` as pure.
// We only apply it to modules known to be side effect free, since we know they are safe.
// getPrefixFunctionsTransformer needs to be before getFoldFileTransformer.
getPrefixFunctionsTransformer,
selectedGetScrubFileTransformer,
);
typeCheck = true;
} else if (testScrubFile(content)) {
// Always test as these require the type checker
getTransforms.push(
selectedGetScrubFileTransformer,
);
typeCheck = true;
} else if (testPrefixClasses(content)) {
// This is only relevant if prefix functions is not used since prefix functions will prefix IIFE wrapped classes.
getTransforms.unshift(getPrefixClassesTransformer);
}

if (testPrefixClasses(content)) {
getTransforms.unshift(getPrefixClassesTransformer);
if (testScrubFile(content)) {
// Always test as these require the type checker
getTransforms.push(createScrubFileTransformerFactory(isAngularCoreFile));
typeCheck = true;
}

getTransforms.push(getWrapEnumsTransformer);
Expand Down
13 changes: 0 additions & 13 deletions packages/angular_devkit/build_optimizer/src/build-optimizer/build-optimizer_spec.ts
View file
Expand Up @@ -84,19 +84,6 @@ describe('build-optimizer', () => {
});
});

it('supports flagging module as side-effect free', () => {
const output = tags.oneLine`
var RenderType_MdOption = /*@__PURE__*/ ɵcrt({ encapsulation: 2, styles: styles_MdOption });
`;
const input = tags.stripIndent`
var RenderType_MdOption = ɵcrt({ encapsulation: 2, styles: styles_MdOption});
`;

const boOutput = buildOptimizer({ content: input, isSideEffectFree: true });
expect(tags.oneLine`${boOutput.content}`).toEqual(output);
expect(boOutput.emitSkipped).toEqual(false);
});

it('should not add pure comments to tslib helpers', () => {
const input = tags.stripIndent`
class LanguageState {
Expand Down
19 changes: 15 additions & 4 deletions packages/angular_devkit/build_optimizer/src/index.ts
View file
Expand Up @@ -5,6 +5,9 @@
* 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 * as ts from 'typescript';
import { createScrubFileTransformerFactory } from './transforms/scrub-file';

export {
default as buildOptimizerLoader,
buildOptimizerLoaderPath,
Expand All @@ -16,8 +19,16 @@ export { transformJavascript } from './helpers/transform-javascript';

export { getPrefixClassesTransformer } from './transforms/prefix-classes';
export { getPrefixFunctionsTransformer } from './transforms/prefix-functions';
export {
getScrubFileTransformer,
getScrubFileTransformerForCore,
} from './transforms/scrub-file';
export { getWrapEnumsTransformer } from './transforms/wrap-enums';

export function getScrubFileTransformer(
program?: ts.Program,
): ts.TransformerFactory<ts.SourceFile> {
return createScrubFileTransformerFactory(false)(program);
}

export function getScrubFileTransformerForCore(
program?: ts.Program,
): ts.TransformerFactory<ts.SourceFile> {
return createScrubFileTransformerFactory(true)(program);
}
31 changes: 17 additions & 14 deletions packages/angular_devkit/build_optimizer/src/transforms/scrub-file.ts
View file
Expand Up @@ -20,26 +20,23 @@ export function testScrubFile(content: string) {
return markers.some((marker) => content.includes(marker));
}

export function getScrubFileTransformer(program?: ts.Program): ts.TransformerFactory<ts.SourceFile> {
return scrubFileTransformer(program, false);
export function createScrubFileTransformerFactory(
isAngularCoreFile: boolean,
): (program?: ts.Program) => ts.TransformerFactory<ts.SourceFile> {
return (program) => scrubFileTransformer(program, isAngularCoreFile);
}

export function getScrubFileTransformerForCore(
program?: ts.Program,
): ts.TransformerFactory<ts.SourceFile> {
return scrubFileTransformer(program, true);
}

function scrubFileTransformer(program: ts.Program | undefined, isAngularCoreFile: boolean) {
function scrubFileTransformer(
program: ts.Program | undefined,
isAngularCoreFile: boolean,
) {
if (!program) {
throw new Error('scrubFileTransformer requires a TypeScript Program.');
}
const checker = program.getTypeChecker();

return (context: ts.TransformationContext): ts.Transformer<ts.SourceFile> => {

const transformer: ts.Transformer<ts.SourceFile> = (sf: ts.SourceFile) => {

const ngMetadata = findAngularMetadata(sf, isAngularCoreFile);
const tslibImports = findTslibImports(sf);

Expand Down Expand Up @@ -431,11 +428,17 @@ function pickDecorateNodesToRemove(
return true;
});

ngDecoratorCalls.push(...metadataCalls, ...paramCalls);
if (ngDecoratorCalls.length === 0) {
return [];
}

const callCount = ngDecoratorCalls.length + metadataCalls.length + paramCalls.length;

// If all decorators are metadata decorators then return the whole `Class = __decorate([...])'`
// statement so that it is removed in entirety
return (elements.length === ngDecoratorCalls.length) ? [exprStmt] : ngDecoratorCalls;
// statement so that it is removed in entirety.
// If not then only remove the Angular decorators.
// The metadata and param calls may be used by the non-Angular decorators.
return (elements.length === callCount) ? [exprStmt] : ngDecoratorCalls;
}

// Remove Angular decorators from`Clazz.propDecorators = [...];`, or expression itself if all
Expand Down
101 changes: 94 additions & 7 deletions packages/angular_devkit/build_optimizer/src/transforms/scrub-file_spec.ts
View file
Expand Up @@ -10,16 +10,15 @@
import { tags } from '@angular-devkit/core';
import { transformJavascript } from '../helpers/transform-javascript';
import {
getScrubFileTransformer,
getScrubFileTransformerForCore,
createScrubFileTransformerFactory,
testScrubFile,
} from './scrub-file';


const transform = (content: string) => transformJavascript(
{ content, getTransforms: [getScrubFileTransformer], typeCheck: true }).content;
{ content, getTransforms: [createScrubFileTransformerFactory(false)], typeCheck: true }).content;
const transformCore = (content: string) => transformJavascript(
{ content, getTransforms: [getScrubFileTransformerForCore], typeCheck: true }).content;
{ content, getTransforms: [createScrubFileTransformerFactory(true)], typeCheck: true }).content;

describe('scrub-file', () => {
const clazz = 'var Clazz = (function () { function Clazz() { } return Clazz; }());';
Expand Down Expand Up @@ -605,19 +604,61 @@ describe('scrub-file', () => {
});

describe('__param', () => {
it('removes all constructor parameters and their type metadata', () => {
it('removes all constructor parameters and their type metadata with only Angular decorators', () => {
const output = tags.stripIndent`
import { __decorate, __param, __metadata } from "tslib";
import { Component } from '@angular/core';
import { __decorate, __param, __metadata } from "tslib";
var MyClass = /** @class */ (function () {
function MyClass(myParam) {
this.myProp = 'foo';
}
return MyClass;
}());
`;
const input = tags.stripIndent`
import { Component } from '@angular/core';
import { __decorate, __param, __metadata } from "tslib";
var MyClass = /** @class */ (function () {
function MyClass(myParam) {
this.myProp = 'foo';
}
MyClass = __decorate([
myDecorator()
Component(),
__param(0, Component()),
__metadata("design:paramtypes", [Number])
], MyClass);
return MyClass;
}());
`;

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

it('keeps all constructor parameters and their type metadata with only custom decorators', () => {
const output = tags.stripIndent`
import { __decorate, __param, __metadata } from "tslib";
var MyClass = /** @class */ (function () {
function MyClass(myParam) {
this.myProp = 'foo';
}
MyClass = __decorate([
myDecorator(),
__param(0, myDecorator()),
__metadata("design:paramtypes", [Number])
], MyClass);
return MyClass;
}());
var MyOtherClass = /** @class */ (function () {
function MyOtherClass(myParam) {
this.myProp = 'bar';
}
MyOtherClass = __decorate([
__metadata("design:paramtypes", [Number])
], MyOtherClass);
return MyOtherClass;
}());
`;
const input = tags.stripIndent`
import { __decorate, __param, __metadata } from "tslib";
var MyClass = /** @class */ (function () {
Expand All @@ -631,6 +672,52 @@ describe('scrub-file', () => {
], MyClass);
return MyClass;
}());
var MyOtherClass = /** @class */ (function () {
function MyOtherClass(myParam) {
this.myProp = 'bar';
}
MyOtherClass = __decorate([
__metadata("design:paramtypes", [Number])
], MyOtherClass);
return MyOtherClass;
}());
`;

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

it('keeps all constructor parameters and their type metadata with custom & Angular decorators', () => {
const output = tags.stripIndent`
import { Component } from '@angular/core';
import { __decorate, __param, __metadata } from "tslib";
var MyClass = /** @class */ (function () {
function MyClass(myParam) {
this.myProp = 'foo';
}
MyClass = __decorate([
myDecorator(),
__param(0, myDecorator()),
__metadata("design:paramtypes", [Number])
], MyClass);
return MyClass;
}());
`;
const input = tags.stripIndent`
import { Component } from '@angular/core';
import { __decorate, __param, __metadata } from "tslib";
var MyClass = /** @class */ (function () {
function MyClass(myParam) {
this.myProp = 'foo';
}
MyClass = __decorate([
Component(),
myDecorator(),
__param(0, myDecorator()),
__metadata("design:paramtypes", [Number])
], MyClass);
return MyClass;
}());
`;

expect(testScrubFile(input)).toBeTruthy();
Expand Down

0 comments on commit 4b4822f

Please sign in to comment.