bug(build-optimizer): initial Map values are stripped unnecessarily

[petebacondarwin]

This code in the @angular/animations package gets compiled incorrectly when the build optimizer is turned on:

// DO NOT REFACTOR ... keep the follow set instantiations
// with the values intact (closure compiler for some reason
// removes follow-up lines that add the values outside of
// the constructor...
const TRUE_BOOLEAN_VALUES = new Set<string>(['true', '1']);
const FALSE_BOOLEAN_VALUES = new Set<string>(['false', '0']);

WITHOUT optimization:

var TRUE_BOOLEAN_VALUES = new Set();
TRUE_BOOLEAN_VALUES.add('true');
TRUE_BOOLEAN_VALUES.add('1');
var FALSE_BOOLEAN_VALUES = new Set();
FALSE_BOOLEAN_VALUES.add('false');
FALSE_BOOLEAN_VALUES.add('0');

WITH optimization:

var TRUE_BOOLEAN_VALUES = new Set();
var FALSE_BOOLEAN_VALUES = new Set();

You can see that the initialization is being stripped out causing animations that rely upon boolean values not to match, and so not run.

This use case was identified in the AIO application, where we wanted to use boolean based animation states.

 animations: [
    trigger('accordion', [
      state('true', style({height: '*'})),
      state('false', style({height: 0})),
      transition('true => false', animate(250))
    ])

You could see the animations working without the optmizer ng start --prod --build-optimizer=false but as soon as you ran in production mode ng start --prod the animation was not matching and so not being played.

We worked around it by using string based animation states instead:

 animations: [
    trigger('accordion', [
      state('show', style({height: '*'})),
      state('hide', style({height: 0})),
      transition('show => hide', animate(250))
    ])

Bug Report or Feature Request (mark with an x)

- [x] bug report -> please search issues before submitting
- [ ] feature request

Area

- [x] devkit
- [ ] schematics

Versions

• node: v8.9.4
• OS: High Sierra
• @angular-devkit/build-optimizer: 0.0.31

[filipesilva]

Running Build Optimizer over the following code:

var TRUE_BOOLEAN_VALUES = new Set();
TRUE_BOOLEAN_VALUES.add('true');
TRUE_BOOLEAN_VALUES.add('1');
var FALSE_BOOLEAN_VALUES = new Set();
FALSE_BOOLEAN_VALUES.add('false');
FALSE_BOOLEAN_VALUES.add('0');

will return

var TRUE_BOOLEAN_VALUES = /*@__PURE__*/ new Set();
/*@__PURE__*/ TRUE_BOOLEAN_VALUES.add('true');
/*@__PURE__*/ TRUE_BOOLEAN_VALUES.add('1');
var FALSE_BOOLEAN_VALUES = /*@__PURE__*/ new Set();
/*@__PURE__*/ FALSE_BOOLEAN_VALUES.add('false');
/*@__PURE__*/ FALSE_BOOLEAN_VALUES.add('0');

This is expected behaviour since Build Optimizer consider all code in @angular/* packages to be side-effect free. This causes all top level function calls and constructor to get the /*@__PURE__*/ comment, which will later cause those statements to be dropped if their values are not used.

But that code is actually not side effect free. Those function calls cause a side effect. I suppose that's why there's a comment there about the Closure compiler as well.

So I'm not really sure what to do in this case. We can remove @angular/animations from the side-effect free whitelist but that's likely to cause size regressions.

I think the Angular packages are actually meant to be side-effect free so maybe that code should be modified.

@IgorMinar WDYT?

[filipesilva]

What's doing this transpilation though?

const TRUE_BOOLEAN_VALUES = new Set<string>(['true', '1']);
const FALSE_BOOLEAN_VALUES = new Set<string>(['false', '0']);

var TRUE_BOOLEAN_VALUES = new Set();
TRUE_BOOLEAN_VALUES.add('true');
TRUE_BOOLEAN_VALUES.add('1');
var FALSE_BOOLEAN_VALUES = new Set();
FALSE_BOOLEAN_VALUES.add('false');
FALSE_BOOLEAN_VALUES.add('0');

I think Angular itself uses TS 2.6 and according to the TS Playground it's correctly compiled to

var TRUE_BOOLEAN_VALUES = new Set(['true', '1']);
var FALSE_BOOLEAN_VALUES = new Set(['false', '0']);

[clydin]

This was an issue with @angular/animations less then 5.1.1.
This commit fixed the issue: angular/angular@661fdcd

[petebacondarwin]

Ahah. I think we may not yet be on latest

…

On Tue, 9 Jan 2018, 16:03 clydin, ***@***.***> wrote: This was an issue with @angular/animations less then 5.1.1. This commit fixed the issue: ***@***.*** <angular/angular@661fdcd> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#364 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA9JymRlQlmXbtu_mtX2TImBnqQ2AJ7ks5tI43HgaJpZM4RX2S_> .

[IgorMinar]

@petebacondarwin I have a PR pending that updates aio to the latest rc

[IgorMinar]

The interesting thing about this issue is that while the source code has no non-local side effects, the transpiled code no longer has that property because what used to be a single statement, got transpiled into three statements.

[filipesilva]

@IgorMinar I think the transpiled code shown was not transpiled from the current code in master, as per @clydin's comment (angular/angular@661fdcd).

I tried transpiling with TS 2.6 and 2.4 and it yielded the correct JS code with no side effects. The source before that commit really did have side effects.

[petebacondarwin]

I can concur. The code we are using in AIO is currently from 5.1.0-beta.2 so it is before the fix that moves those side-effect calls into constructor parameters.

[petebacondarwin]

I think we can close this.