Skip to content

buildOptimizer deleted some code from a property setter in a component that came from an ng-packagr library #14084

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

Closed
fr0 opened this issue Apr 4, 2019 · 3 comments · Fixed by #14263
Labels
freq1: low Only reported by a handful of users who observe it rarely severity3: broken type: bug/fix
Milestone

Comments

@fr0
Copy link

fr0 commented Apr 4, 2019

🐞 Bug report

Command (mark with an x)

- [ ] new
- [x] build
- [x] serve
- [ ] test
- [ ] e2e
- [ ] generate
- [ ] add
- [ ] update
- [ ] lint
- [ ] xi18n
- [ ] run
- [ ] config
- [ ] help
- [ ] version
- [ ] doc

Is this a regression?

Unknown.

Description

The build optimizer seems to have stripped out some important code. Here's my original TypeScript (template and styles ommitted for brevity):

export interface IExpandable {
  isExpanded: boolean;
  isExpandedChange: EventEmitter<boolean>;
  canExpand: boolean;
  toggleExpand(): void;
  expand();
  collapse();
}

@Component({
  selector: 'atlas-expandable-if',
  template: `
...
  `,
  styles: [`
...
  `]
})
export class ExpandableIfComponent implements IExpandable {
  @Input() preserveSpaceForIcon: boolean = true;
  @Input() canExpand: boolean = true;
  @Input() expandIcon: string;
  @Input() collapseIcon: string;
  @Input() loadingIcon: string;
  @Input() delayedExpand = false;
  @Input() overrideIcon: string|false = false;
  @Output() isExpandedChange: EventEmitter<boolean> = new EventEmitter<boolean>();
  isExpanding = false;

  constructor(public icons: AtlasIconPaths,
              private changeDetector: ChangeDetectorRef) { }

  private _isExpanded = false;
  get isExpanded(): boolean { return this._isExpanded; }
  @Input() set isExpanded(value: boolean) {
    if (this._isExpanded !== value) {
      this._isExpanded = value;
      this.isExpandedChange.emit(value);
      this.changeDetector.markForCheck();
    }
  }

  clickHeader(event: MouseEvent) {
    if (!event.shiftKey) {
      event.preventDefault();
      event.stopPropagation();
      this.toggleExpand();
    }
  }

  toggleExpand() {
    if (this.isExpanding) { return; }
    if (this.isExpanded || this.canExpand) {
      if (this.isExpanded) {
        this.collapse();
      }
      else {
        this.expand();
      }
    }
  }
  expand() {
    if (this.delayedExpand) {
      this.isExpanding = true;
      setTimeout(() => {
        this.isExpanding = false;
        this.isExpanded = true;
      });
      this.changeDetector.markForCheck();
    }
    else {
      this.isExpanded = true;
    }
  }
  collapse() {
    this.isExpanded = false;
  }
}

And here's the code that gets generated with buildOptimizer: true (un-minified, but otherwise presented as-is):

Notice the set isExpanded property setter

                    sa = class {
                        constructor(e, t) {
                            this.icons = e, this.changeDetector = t, this.preserveSpaceForIcon = !0, this.canExpand = !0, this.delayedExpand = !1, this.overrideIcon = !1, this.isExpandedChange = new it, this.isExpanding = !1, this._isExpanded = !1
                        }
                        get isExpanded() {
                            return this._isExpanded
                        }
                        set isExpanded(e) {
                            this._isExpanded !== e && (this._isExpanded = e)
                        }
                        clickHeader(e) {
                            e.shiftKey || (e.preventDefault(), e.stopPropagation(), this.toggleExpand())
                        }
                        toggleExpand() {
                            this.isExpanding || (this.isExpanded || this.canExpand) && (this.isExpanded ? this.collapse() : this.expand())
                        }
                        expand() {
                            this.delayedExpand ? (this.isExpanding = !0, setTimeout(() => {
                                this.isExpanding = !1, this.isExpanded = !0
                            }), this.changeDetector.markForCheck()) : this.isExpanded = !0
                        }
                        collapse() {
                            this.isExpanded = !1
                        }
                    },

This code is in a library that gets packaged via ng-packagr version 5.0.1. However, here is the code that ng-packagr generates:

let ExpandableIfComponent = class ExpandableIfComponent {
    constructor(icons, changeDetector) {
        this.icons = icons;
        this.changeDetector = changeDetector;
        this.preserveSpaceForIcon = true;
        this.canExpand = true;
        this.delayedExpand = false;
        this.overrideIcon = false;
        this.isExpandedChange = new EventEmitter();
        this.isExpanding = false;
        this._isExpanded = false;
    }
    get isExpanded() { return this._isExpanded; }
    set isExpanded(value) {
        if (this._isExpanded !== value) {
            this._isExpanded = value;
            this.isExpandedChange.emit(value);
            this.changeDetector.markForCheck();
        }
    }
    clickHeader(event) {
        if (!event.shiftKey) {
            event.preventDefault();
            event.stopPropagation();
            this.toggleExpand();
        }
    }
    toggleExpand() {
        if (this.isExpanding) {
            return;
        }
        if (this.isExpanded || this.canExpand) {
            if (this.isExpanded) {
                this.collapse();
            }
            else {
                this.expand();
            }
        }
    }
    expand() {
        if (this.delayedExpand) {
            this.isExpanding = true;
            setTimeout(() => {
                this.isExpanding = false;
                this.isExpanded = true;
            });
            this.changeDetector.markForCheck();
        }
        else {
            this.isExpanded = true;
        }
    }
    collapse() {
        this.isExpanded = false;
    }
};
__decorate([
    Input(),
    __metadata("design:type", Boolean)
], ExpandableIfComponent.prototype, "preserveSpaceForIcon", void 0);
__decorate([
    Input(),
    __metadata("design:type", Boolean)
], ExpandableIfComponent.prototype, "canExpand", void 0);
__decorate([
    Input(),
    __metadata("design:type", String)
], ExpandableIfComponent.prototype, "expandIcon", void 0);
__decorate([
    Input(),
    __metadata("design:type", String)
], ExpandableIfComponent.prototype, "collapseIcon", void 0);
__decorate([
    Input(),
    __metadata("design:type", String)
], ExpandableIfComponent.prototype, "loadingIcon", void 0);
__decorate([
    Input(),
    __metadata("design:type", Object)
], ExpandableIfComponent.prototype, "delayedExpand", void 0);
__decorate([
    Input(),
    __metadata("design:type", Object)
], ExpandableIfComponent.prototype, "overrideIcon", void 0);
__decorate([
    Output(),
    __metadata("design:type", EventEmitter)
], ExpandableIfComponent.prototype, "isExpandedChange", void 0);
__decorate([
    Input(),
    __metadata("design:type", Boolean),
    __metadata("design:paramtypes", [Boolean])
], ExpandableIfComponent.prototype, "isExpanded", null);
ExpandableIfComponent = __decorate([
    Component({
        selector: 'atlas-expandable-if',
        template: `
...
        styles: [`
...
  `]
    }),
    __metadata("design:paramtypes", [AtlasIconPaths,
        ChangeDetectorRef])
], ExpandableIfComponent);

The correct code is generated if buildOptimizer: false is used in my angular.json.

🌍 Your Environment




Angular CLI: 7.3.7
Node: 11.12.0
OS: darwin x64
Angular: 7.2.11
... animations, common, compiler, compiler-cli, core, elements
... forms, http, language-service, platform-browser
... platform-browser-dynamic, router

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.13.7
@angular-devkit/build-angular     0.13.7
@angular-devkit/build-optimizer   0.13.7
@angular-devkit/core              7.3.7
@angular-devkit/schematics        7.3.7
@angular/cli                      7.3.7
@ngtools/json-schema              1.1.0
@schematics/angular               7.3.7
@schematics/update                0.13.7
ng-packagr                        5.0.1
rxjs                              6.4.0
typescript                        3.2.4
webpack                           4.29.6

@fr0 fr0 changed the title buildOptimizer: true broke my app buildOptimizer: true deleted some code from a property setter in a component that came from an ng-packagr library Apr 4, 2019
@fr0 fr0 changed the title buildOptimizer: true deleted some code from a property setter in a component that came from an ng-packagr library buildOptimizer deleted some code from a property setter in a component that came from an ng-packagr library Apr 4, 2019
@fr0
Copy link
Author

fr0 commented Apr 4, 2019

For those looking for the TL;DR...

Here's the property setter before buildOptimizer:

set isExpanded(value) {
  if (this._isExpanded !== value) {
    this._isExpanded = value;
    this.isExpandedChange.emit(value);
    this.changeDetector.markForCheck();
  }
}

And here's the after:

set isExpanded(e) {
  this._isExpanded !== e && (this._isExpanded = e)
}

The two lines it dropped are very important, and not having them breaks my app.

@fr0 fr0 closed this as completed Apr 4, 2019
@fr0 fr0 reopened this Apr 4, 2019
@ngbot ngbot bot added this to the needsTriage milestone Apr 5, 2019
@alan-agius4
Copy link
Collaborator

alan-agius4 commented Apr 9, 2019

Hi, I just had a quick look at this, and it seems that BuildOptimizer/Terser are dropping the code mentioned above, when the ES2015 JS code is not emitted by Tsickle but rather NGC directly.

Note: Tsickle is not enabled by default in CLI libraries, nor is in ng-packagr version 5.

NGC + Tsickle

class LibComponent {
    /**
     * @param {?} changeDetector
     */
    constructor(changeDetector) {
        this.changeDetector = changeDetector;
        this.isExpandedChange = new EventEmitter();
        this.isExpanding = false;
        this._isExpanded = false;
    }
    /**
     * @return {?}
     */
    get isExpanded() { return this._isExpanded; }
    /**
     * @param {?} value
     * @return {?}
     */
    set isExpanded(value) {
        if (this._isExpanded !== value) {
            this._isExpanded = value;
            this.isExpandedChange.emit(value);
            this.changeDetector.markForCheck();
        }
    }
}
LibComponent.decorators = [
    { type: Component, args: [{
                selector: 'lib',
                template: `hello workd`
            }] }
];
/** @nocollapse */
LibComponent.ctorParameters = () => [
    { type: ChangeDetectorRef }
];
LibComponent.propDecorators = {
    isExpandedChange: [{ type: Output }],
    isExpanded: [{ type: Input }]
};

NGC - Tsickle

let LibComponent = class LibComponent {
    constructor(changeDetector) {
        this.changeDetector = changeDetector;
        this.isExpandedChange = new EventEmitter();
        this.isExpanding = false;
        this._isExpanded = false;
    }
    get isExpanded() { return this._isExpanded; }
    set isExpanded(value) {
        if (this._isExpanded !== value) {
            this._isExpanded = value;
            this.isExpandedChange.emit(value);
            this.changeDetector.markForCheck();
        }
    }
};
__decorate([
    Output(),
    __metadata("design:type", EventEmitter)
], LibComponent.prototype, "isExpandedChange", void 0);
__decorate([
    Input(),
    __metadata("design:type", Boolean),
    __metadata("design:paramtypes", [Boolean])
], LibComponent.prototype, "isExpanded", null);
LibComponent = __decorate([
    Component({
        selector: 'lib',
        template: `hello workd`
    }),
    __metadata("design:paramtypes", [ChangeDetectorRef])
], LibComponent);

Following a convo with @filipesilva, he thinks that the below might be the cause of such issue:

let LibComponent = class LibComponent {

@alan-agius4 alan-agius4 added freq1: low Only reported by a handful of users who observe it rarely severity3: broken type: bug/fix labels Apr 9, 2019
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Apr 9, 2019
@filipesilva filipesilva modified the milestones: Backlog, 8.0 Apr 18, 2019
alexeagle pushed a commit that referenced this issue Apr 24, 2019
… expressions

When we removed tsickle from the library compilation pipeline the emitted JS changes for Classes.

With tsc a class can be of kind CallExpression because of this syntax
```
let Foo = class Foo {
	constructor() {
		this.isExpandedChange = new EventEmitter();
	}

	set isExpanded(value) {
		this.isExpandedChange.emit(value);
	}
};
```

In such case we shall not add `/*@__PURE__*/` inside this class

Fixes #14084
@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 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
freq1: low Only reported by a handful of users who observe it rarely severity3: broken type: bug/fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants