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.

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

fix(@angular-devkit/build-optimizer): wrap es2015 class expressions for better tree-shaking #14585

Merged
merged 3 commits into from
Jun 6, 2019
Merged

Conversation

alan-agius4
Copy link
Collaborator

@alan-agius4 alan-agius4 commented May 30, 2019

fix(@angular-devkit/build-optimizer): wrap es2015 class expressions for better tree-shaking

ClassExpressions such as the below are not treeshakable unless we wrap them in an IIFE

let AggregateColumnDirective = class AggregateColumnDirective {
	constructor(viewContainerRef) { }
};
AggregateColumnDirective = __decorate([
	Directive({}),
	__metadata("design:paramtypes", [ViewContainerRef])
], AggregateColumnDirective);

With this change we wrap the above in an IIFE and mark it as a PURE function.

const AggregateColumnDirective = /*@__PURE__*/ (() => {
	let AggregateColumnDirective = class AggregateColumnDirective {
		constructor(viewContainerRef) { }
	};
	AggregateColumnDirective = __decorate([
		Directive({}),
		__metadata("design:paramtypes", [ViewContainerRef])
	], AggregateColumnDirective);

	return AggregateColumnDirective;
})();

With this pattern if the class is unused it will be dropped.

Note: In future we should rename wrap-enums to something more generic, and combine class-fold with this transformer especially considering the future fix that needs to be done for #14610

Fixes #14577

fix(@angular-devkit/build-optimizer) wrap ClassDeclarations in an IIFE for better treeshaking

With this change we wrap ClassDeclarations inside an IIFE, also we move some code from the class fold into the wrap-enums.

This changes the below code:

export class Foo {
	method() {
	}
}
Foo.bar = 'barValue';
__decorate([
	methodDecorator
], Foo.prototype, "method", null);

to

export const Foo = /*@__PURE__*/ (() => {
  class Foo {
    method() {
    }
  }
  Foo.bar = 'barValue';
  __decorate([
    methodDecorator
  ], Foo.prototype, "method", null);

  return Foo;
})();

Fixes #14610

test: add test to verify that the new es2015 class wrapping logic handles wrapping of tslib and tsickle classes

Related to ngrx/platform#1905 and ng-packagr/ng-packagr#1307

Fixes: #14613

@alan-agius4 alan-agius4 changed the title wip! class expressions fix(@angular-devkit/build-optimizer): wrap es2015 class expressions for better tree-shaking May 31, 2019
@alan-agius4 alan-agius4 added target: patch This PR is targeted for the next patch release and removed state: WIP labels May 31, 2019
@alan-agius4 alan-agius4 marked this pull request as ready for review May 31, 2019 11:37
@alan-agius4 alan-agius4 requested review from filipesilva and clydin May 31, 2019 11:37
…or better tree-shaking

ClassExpressions such as the below are not treeshakable unless we wrap them in an IIFE

```js
let AggregateColumnDirective = class AggregateColumnDirective {
	constructor(viewContainerRef) { }
};
AggregateColumnDirective = __decorate([
	Directive({}),
	__metadata("design:paramtypes", [ViewContainerRef])
], AggregateColumnDirective);
```

With this change we wrap the above in an IIFE and mark it as a PURE function.
```js
const AggregateColumnDirective = /*@__PURE__*/ (() => {
	let AggregateColumnDirective = class AggregateColumnDirective {
		constructor(viewContainerRef) { }
	};
	AggregateColumnDirective = __decorate([
		Directive({}),
		__metadata("design:paramtypes", [ViewContainerRef])
	], AggregateColumnDirective);

	return AggregateColumnDirective;
})();
```

With this pattern if the class is unused it will be dropped.

Note: In future we should rename `wrap-enums` to something more generic, and combine class-fold with this transformer especially considering the future fix that needs to be done for #14610

Fixes #14577
@bgotink
Copy link
Contributor

bgotink commented Jun 3, 2019

It appears classes that self-reference using forwardRef lead to slightly different output:

var TestUnshakeableDirective_1;
export const SOME_TOKEN = new InjectionToken('some token');
let TestUnshakeableDirective = TestUnshakeableDirective_1 = class TestUnshakeableDirective {
    constructor() { }
};
TestUnshakeableDirective = TestUnshakeableDirective_1 = __decorate([
    Directive({
        selector: '[libUnshakeable]',
        providers: [
            {
                provide: SOME_TOKEN,
                useExisting: forwardRef(() => TestUnshakeableDirective_1)
            },
        ]
    })
], TestUnshakeableDirective);
export { TestUnshakeableDirective };

and

var TestUnshakeable2Directive_1;
let TestUnshakeable2Directive = TestUnshakeable2Directive_1 = class TestUnshakeable2Directive {
    constructor(parent) { }
};
TestUnshakeable2Directive = TestUnshakeable2Directive_1 = __decorate([
    Directive({
        selector: '[libUnshakeable2]',
    }),
    __param(0, SkipSelf()), __param(0, Inject(forwardRef(() => TestUnshakeable2Directive_1))),
    __metadata("design:paramtypes", [TestUnshakeable2Directive])
], TestUnshakeable2Directive);
export { TestUnshakeable2Directive };

The output doesn't change for forwardRefs outside of the class though

code
const SOME_TOKEN = new InjectionToken('some token');
const PROVIDE_SOME_TOKEN = {
    provide: SOME_TOKEN,
    useExisting: forwardRef(() => TestShakeableDirective)
};
let TestShakeableDirective = class TestShakeableDirective {
    constructor() { }
};
TestShakeableDirective = __decorate([
    Directive({
        selector: '[libShakeable]',
        providers: [
            PROVIDE_SOME_TOKEN,
        ]
    })
], TestShakeableDirective);

and

let DoorDirective = class DoorDirective {
    constructor(lock) {
        this.lock = lock;
    }
};
DoorDirective = __decorate([
    Directive({ selector: '[libDoor]' }),
    __param(0, Inject(forwardRef(() => LockDirective))),
    __metadata("design:paramtypes", [LockDirective])
], DoorDirective);
let LockDirective = class LockDirective {
};
LockDirective = __decorate([
    Directive({ selector: '[libLock]' })
], LockDirective);

@alan-agius4
Copy link
Collaborator Author

@bgotink, thanks for the above, it will surly help to improve the logic to handle the above case.

Copy link
Contributor

@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

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

The CI failure looks legit:

Running "tests/build/prod-build" (15 of 51 [1:4] (43/189))...
==========================================================================================
Running `ng "build" "--prod"` [silent]...
CWD: /tmp/angular-cli-e2e-201953-110-nkn7c4.kb2o/test-project
ENV: undefined
Last step took 21.09s...

Error: Expected main-es2015.647cfbad8918a52f10b1.js size to be less than 139.81279296875Kb but it was 163.63671875Kb.
    at verifySize (/home/circleci/ng/tests/legacy-cli/e2e/tests/build/prod-build.ts:15:15)
    at default_1 (/home/circleci/ng/tests/legacy-cli/e2e/tests/build/prod-build.ts:46:9)


Test "tests/build/prod-build" failed...
Expected main-es2015.647cfbad8918a52f10b1.js size to be less than 139.81279296875Kb but it was 163.63671875Kb.
Error: Expected main-es2015.647cfbad8918a52f10b1.js size to be less than 139.81279296875Kb but it was 163.63671875Kb.

Unsure why this happens but it's a bit severe. Some of these changes must be causing more to be retained than before.

@alan-agius4
Copy link
Collaborator Author

@filipesilva comments fixed. PTAL

@alan-agius4
Copy link
Collaborator Author

Should we do the class wrapping only when it's sideEffects free?

…FE for better treeshaking

With this change we wrap ClassDeclarations inside an IIFE, also we move some code from the class fold into the wrap-enums.

This changes the below code:
```js
export class Foo {
	method() {
	}
}
Foo.bar = 'barValue';
__decorate([
	methodDecorator
], Foo.prototype, "method", null);
```

to

```js
export const Foo = /*@__PURE__*/ (() => {
  class Foo {
    method() {
    }
  }
  Foo.bar = 'barValue';
  __decorate([
    methodDecorator
  ], Foo.prototype, "method", null);

  return Foo;
})();
```

Fixes #14610
@clydin
Copy link
Member

clydin commented Jun 3, 2019

It should always happen to be consistent with TS ES5 class behavior and hopefully standard TS behavior in general moving forward.

@alan-agius4 alan-agius4 requested a review from filipesilva June 4, 2019 10:59
@filipesilva filipesilva removed their request for review June 4, 2019 12:42
@filipesilva
Copy link
Contributor

I've removed myself as reviewer because I think @clydin understand this particular transformer much better than me.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
target: patch This PR is targeted for the next patch release
Projects
None yet
6 participants