This repository was archived by the owner on Apr 4, 2025. It is now read-only.
Repository navigation
This repository was archived by the owner on Apr 4, 2025. It is now read-only.
bug(build-optimizer): initial Map values are stripped unnecessarily #364
Description
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
Activity
filipesilva commentedon Jan 9, 2018
Running Build Optimizer over the following code:
will return
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/animationsfrom 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 commentedon Jan 9, 2018
What's doing this transpilation though?
I think Angular itself uses TS 2.6 and according to the TS Playground it's correctly compiled to
clydin commentedon Jan 9, 2018
This was an issue with
@angular/animationsless then5.1.1.This commit fixed the issue: angular/angular@661fdcd
petebacondarwin commentedon Jan 9, 2018
IgorMinar commentedon Jan 9, 2018
@petebacondarwin I have a PR pending that updates aio to the latest rc
IgorMinar commentedon Jan 9, 2018
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 commentedon Jan 9, 2018
@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 commentedon Jan 9, 2018
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 commentedon Jan 9, 2018
I think we can close this.