From d38159304d748aa6da18002bec883cbb6bb77a29 Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Sat, 10 Oct 2020 18:59:14 -0400 Subject: [PATCH] fix(@angular-devkit/build-optimizer): increase safety of code removal 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 --- .../src/build-optimizer/build-optimizer.ts | 39 +++---- .../build-optimizer/build-optimizer_spec.ts | 13 --- .../build_optimizer/src/index.ts | 19 +++- .../src/transforms/scrub-file.ts | 31 +++--- .../src/transforms/scrub-file_spec.ts | 101 ++++++++++++++++-- 5 files changed, 142 insertions(+), 61 deletions(-) diff --git a/packages/angular_devkit/build_optimizer/src/build-optimizer/build-optimizer.ts b/packages/angular_devkit/build_optimizer/src/build-optimizer/build-optimizer.ts index f59ad53df96a..209f409c345e 100644 --- a/packages/angular_devkit/build_optimizer/src/build-optimizer/build-optimizer.ts +++ b/packages/angular_devkit/build_optimizer/src/build-optimizer/build-optimizer.ts @@ -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'; @@ -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; @@ -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); diff --git a/packages/angular_devkit/build_optimizer/src/build-optimizer/build-optimizer_spec.ts b/packages/angular_devkit/build_optimizer/src/build-optimizer/build-optimizer_spec.ts index 101c74c3e903..a7b094d5589f 100644 --- a/packages/angular_devkit/build_optimizer/src/build-optimizer/build-optimizer_spec.ts +++ b/packages/angular_devkit/build_optimizer/src/build-optimizer/build-optimizer_spec.ts @@ -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 { diff --git a/packages/angular_devkit/build_optimizer/src/index.ts b/packages/angular_devkit/build_optimizer/src/index.ts index 52b5daf35ce4..99b72e09d294 100644 --- a/packages/angular_devkit/build_optimizer/src/index.ts +++ b/packages/angular_devkit/build_optimizer/src/index.ts @@ -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 type * as ts from 'typescript'; +import { createScrubFileTransformerFactory } from './transforms/scrub-file'; + export { default as buildOptimizerLoader, buildOptimizerLoaderPath, @@ -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 { + return createScrubFileTransformerFactory(false)(program); +} + +export function getScrubFileTransformerCore( + program?: ts.Program, +): ts.TransformerFactory { + return createScrubFileTransformerFactory(true)(program); +} diff --git a/packages/angular_devkit/build_optimizer/src/transforms/scrub-file.ts b/packages/angular_devkit/build_optimizer/src/transforms/scrub-file.ts index 2f1c68342fb4..4c5e70663525 100644 --- a/packages/angular_devkit/build_optimizer/src/transforms/scrub-file.ts +++ b/packages/angular_devkit/build_optimizer/src/transforms/scrub-file.ts @@ -20,26 +20,23 @@ export function testScrubFile(content: string) { return markers.some((marker) => content.includes(marker)); } -export function getScrubFileTransformer(program?: ts.Program): ts.TransformerFactory { - return scrubFileTransformer(program, false); +export function createScrubFileTransformerFactory( + isAngularCoreFile: boolean, +): (program?: ts.Program) => ts.TransformerFactory { + return (program) => scrubFileTransformer(program, isAngularCoreFile); } -export function getScrubFileTransformerForCore( - program?: ts.Program, -): ts.TransformerFactory { - 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 => { - const transformer: ts.Transformer = (sf: ts.SourceFile) => { - const ngMetadata = findAngularMetadata(sf, isAngularCoreFile); const tslibImports = findTslibImports(sf); @@ -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 diff --git a/packages/angular_devkit/build_optimizer/src/transforms/scrub-file_spec.ts b/packages/angular_devkit/build_optimizer/src/transforms/scrub-file_spec.ts index e7e8e46c507f..145e9c979b90 100644 --- a/packages/angular_devkit/build_optimizer/src/transforms/scrub-file_spec.ts +++ b/packages/angular_devkit/build_optimizer/src/transforms/scrub-file_spec.ts @@ -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; }());'; @@ -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 () { @@ -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();