Skip to content
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

Jump to bottom

fix(@angular-devkit/build-optimizer): increase safety of code removal #19049

Merged
merged 2 commits into from Oct 12, 2020
Merged

fix(@angular-devkit/build-optimizer): increase safety of code removal #19049

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
46ef96e
fix(@angular-devkit/build-optimizer): increase safety of code removal
clydin Oct 10, 2020
6bfa224
test(@angular-devkit/build-angular): disable experimental Rollup size…
clydin Oct 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/angular_devkit/build_angular/src/browser/specs/rollup_spec.ts
View file
Open in desktop
Expand Up @@ -82,15 +82,15 @@ describe('Browser Builder Rollup Concatenation test', () => {
await browserBuild(architect, host, target, overrides);
});

it('creates smaller or same size bundles for app without lazy bundles', async () => {
xit('creates smaller or same size bundles for app without lazy bundles', async () => {
const prodOutput = await browserBuild(architect, host, target, prodOverrides);
const prodSize = await getOutputSize(prodOutput);
const rollupProdOutput = await browserBuild(architect, host, target, rollupProdOverrides);
const rollupProd = await getOutputSize(rollupProdOutput);
expect(prodSize).toBeGreaterThan(rollupProd);
});

it('creates smaller bundles for apps with lazy bundles', async () => {
xit('creates smaller bundles for apps with lazy bundles', async () => {
host.writeMultipleFiles(lazyModuleFiles);
host.writeMultipleFiles(lazyModuleFnImport);
const prodOutput = await browserBuild(architect, host, target, prodOverrides);
Expand Down
39 changes: 16 additions & 23 deletions packages/angular_devkit/build_optimizer/src/build-optimizer/build-optimizer.ts
View file
Open in desktop
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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know for which cases the prefix function transformer is needed in Angular?

Copy link
Member Author

@clydin clydin Oct 12, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

View Engine factory files need it and were already included in the safe side effect check.
Additionally, there are a variety call/new expressions in @angular/* involving the following:

[
  "ComponentFactoryResolver$1",
  "InjectionToken",
  "KeyRegistry",
  "Optional",
  "ObservableStrategy",
  "PromiseStrategy",
  "ReflectionCapabilities",
  "Reflector",
  "Version",
  "core_merge",
  "getClosureSafeProperty",
  "makeDecorator",
  "makeParamDecorator",
  "makePropDecorator",
  "tagSet",
  "tokenKey",
]

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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