Skip to content

Commit

Permalink
fix(angular): remove form control side effects (#28359)
Browse files Browse the repository at this point in the history
Issue number: resolves #28358

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->


28f2ec9
exposed a (possible) `ng-packagr` bug where the form control components
were being re-assigned, which breaks treeshaking. These components were
considered side effects and were always being pulled into the bundle.

This resulted in a higher than expected bundle size. This issue appears
to be caused by using 2 decorators **and** referring to the class in
`useExisting` (for providers). Doing just one of these does not
reproduce the issue.

The compiled output looks something like this:

```typescript
let IonToggle = IonToggle_1 = /*@__PURE__*/ class IonToggle extends ValueAccessor {
    constructor(c, r, z, injector) {
        super(injector, r);
        this.z = z;
        c.detach();
        this.el = r.nativeElement;
        proxyOutputs(this, this.el, ['ionChange', 'ionFocus', 'ionBlur']);
    }
    writeValue(value) {
        this.elementRef.nativeElement.checked = this.lastValue = value;
        setIonicClasses(this.elementRef);
    }
    handleIonChange(el) {
        this.handleValueChange(el, el.checked);
    }
};
/** @nocollapse */ IonToggle.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "14.2.12", ngImport: i0, type: IonToggle, deps: [{ token: i0.ChangeDetectorRef }, { token: i0.ElementRef }, { token: i0.NgZone }, { token: i0.Injector }], target: i0.ɵɵFactoryTarget.Component });
/** @nocollapse */ IonToggle.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "14.2.12", type: IonToggle, isStandalone: true, selector: "ion-toggle", inputs: { checked: "checked", color: "color", disabled: "disabled", enableOnOffLabels: "enableOnOffLabels", justify: "justify", labelPlacement: "labelPlacement", legacy: "legacy", mode: "mode", name: "name", value: "value" }, host: { listeners: { "ionChange": "handleIonChange($event.target)" } }, providers: [
        {
            provide: NG_VALUE_ACCESSOR,
            useExisting: IonToggle_1,
            multi: true,
        },
    ], usesInheritance: true, ngImport: i0, template: '<ng-content></ng-content>', isInline: true, changeDetection: i0.ChangeDetectionStrategy.OnPush });
IonToggle = IonToggle_1 = __decorate([
    ProxyCmp({
        defineCustomElementFn: defineCustomElement$1i,
        inputs: TOGGLE_INPUTS,
    })
], IonToggle);
```

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Removed the `ProxyCmp` usage in favor of manually calling proxyInputs
and proxyMethods.
-  Also saw that select was missing a form control test, so I added one

The compiled code now looks something like this:

```typescript
class IonToggle extends ValueAccessor {
    constructor(c, r, z, injector) {
        super(injector, r);
        this.z = z;
        defineCustomElement$1i();
        proxyInputs(IonToggle, TOGGLE_INPUTS);
        c.detach();
        this.el = r.nativeElement;
        proxyOutputs(this, this.el, ['ionChange', 'ionFocus', 'ionBlur']);
    }
    writeValue(value) {
        this.elementRef.nativeElement.checked = this.lastValue = value;
        setIonicClasses(this.elementRef);
    }
    handleIonChange(el) {
        this.handleValueChange(el, el.checked);
    }
}
/** @nocollapse */ IonToggle.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "14.2.12", ngImport: i0, type: IonToggle, deps: [{ token: i0.ChangeDetectorRef }, { token: i0.ElementRef }, { token: i0.NgZone }, { token: i0.Injector }], target: i0.ɵɵFactoryTarget.Component });
/** @nocollapse */ IonToggle.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "14.2.12", type: IonToggle, isStandalone: true, selector: "ion-toggle", inputs: { checked: "checked", color: "color", disabled: "disabled", enableOnOffLabels: "enableOnOffLabels", justify: "justify", labelPlacement: "labelPlacement", legacy: "legacy", mode: "mode", name: "name", value: "value" }, host: { listeners: { "ionChange": "handleIonChange($event.target)" } }, providers: [
        {
            provide: NG_VALUE_ACCESSOR,
            useExisting: IonToggle,
            multi: true,
        },
    ], usesInheritance: true, ngImport: i0, template: '<ng-content></ng-content>', isInline: true, changeDetection: i0.ChangeDetectionStrategy.OnPush });
```

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Ryan provided some context on a related Stencil bug where doing
reassignments broke treeshaking in Webpack. While the source of this bug
is not Stencil, understanding the Stencil bug helped me better
understand this issue:

ionic-team/stencil#3191
ionic-team/stencil#3248
ionic-team/stencil#4188 (fixes an issue
introduced in the above stencil PR)

Dev build: `7.5.1-dev.11697480817.10fa2601`
  • Loading branch information
liamdebeasi committed Oct 18, 2023
1 parent 416bb73 commit 82d6309
Show file tree
Hide file tree
Showing 24 changed files with 407 additions and 80 deletions.
31 changes: 25 additions & 6 deletions packages/angular/standalone/src/directives/checkbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,25 @@ import {
Injector,
NgZone,
} from '@angular/core';
import type { OnInit } from '@angular/core';
import { NG_VALUE_ACCESSOR } from '@angular/forms';
import { ValueAccessor, setIonicClasses } from '@ionic/angular/common';
import type { CheckboxChangeEventDetail, Components } from '@ionic/core/components';
import { defineCustomElement } from '@ionic/core/components/ion-checkbox.js';

import { ProxyCmp, proxyOutputs } from './angular-component-lib/utils';
/**
* Value accessor components should not use ProxyCmp
* and should call defineCustomElement and proxyInputs
* manually instead. Using both the @ProxyCmp and @Component
* decorators and useExisting (where useExisting refers to the
* class) causes ng-packagr to output multiple component variables
* which breaks treeshaking.
* For example, the following would be generated:
* let IonCheckbox = IonCheckbox_1 = class IonCheckbox extends ValueAccessor {
* Instead, we want only want the class generated:
* class IonCheckbox extends ValueAccessor {
*/
import { proxyInputs, proxyOutputs } from './angular-component-lib/utils';

const CHECKBOX_INPUTS = [
'checked',
Expand All @@ -28,10 +41,6 @@ const CHECKBOX_INPUTS = [
'value',
];

@ProxyCmp({
defineCustomElementFn: defineCustomElement,
inputs: CHECKBOX_INPUTS,
})
@Component({
selector: 'ion-checkbox',
changeDetection: ChangeDetectionStrategy.OnPush,
Expand All @@ -47,15 +56,25 @@ const CHECKBOX_INPUTS = [
],
standalone: true,
})
export class IonCheckbox extends ValueAccessor {
export class IonCheckbox extends ValueAccessor implements OnInit {
protected el: HTMLElement;
constructor(c: ChangeDetectorRef, r: ElementRef, protected z: NgZone, injector: Injector) {
super(injector, r);
defineCustomElement();
c.detach();
this.el = r.nativeElement;
proxyOutputs(this, this.el, ['ionChange', 'ionFocus', 'ionBlur']);
}

ngOnInit(): void {
/**
* Data-bound input properties are set
* by Angular after the constructor, so
* we need to run the proxy in ngOnInit.
*/
proxyInputs(IonCheckbox, CHECKBOX_INPUTS);
}

writeValue(value: boolean): void {
this.elementRef.nativeElement.checked = this.lastValue = value;
setIonicClasses(this.elementRef);
Expand Down
35 changes: 28 additions & 7 deletions packages/angular/standalone/src/directives/datetime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,25 @@ import {
Injector,
NgZone,
} from '@angular/core';
import type { OnInit } from '@angular/core';
import { NG_VALUE_ACCESSOR } from '@angular/forms';
import { ValueAccessor } from '@ionic/angular/common';
import type { DatetimeChangeEventDetail, Components } from '@ionic/core/components';
import { defineCustomElement } from '@ionic/core/components/ion-datetime.js';

import { ProxyCmp, proxyOutputs } from './angular-component-lib/utils';
/**
* Value accessor components should not use ProxyCmp
* and should call defineCustomElement and proxyInputs
* manually instead. Using both the @ProxyCmp and @Component
* decorators and useExisting (where useExisting refers to the
* class) causes ng-packagr to output multiple component variables
* which breaks treeshaking.
* For example, the following would be generated:
* let IonDatetime = IonDatetime_1 = class IonDatetime extends ValueAccessor {
* Instead, we want only want the class generated:
* class IonDatetime extends ValueAccessor {
*/
import { proxyInputs, proxyMethods, proxyOutputs } from './angular-component-lib/utils';

const DATETIME_INPUTS = [
'cancelText',
Expand Down Expand Up @@ -48,11 +61,8 @@ const DATETIME_INPUTS = [
'yearValues',
];

@ProxyCmp({
defineCustomElementFn: defineCustomElement,
inputs: DATETIME_INPUTS,
methods: ['confirm', 'reset', 'cancel'],
})
const DATETIME_METHODS = ['confirm', 'reset', 'cancel'];

@Component({
selector: 'ion-datetime',
changeDetection: ChangeDetectionStrategy.OnPush,
Expand All @@ -68,15 +78,26 @@ const DATETIME_INPUTS = [
],
standalone: true,
})
export class IonDatetime extends ValueAccessor {
export class IonDatetime extends ValueAccessor implements OnInit {
protected el: HTMLElement;
constructor(c: ChangeDetectorRef, r: ElementRef, protected z: NgZone, injector: Injector) {
super(injector, r);
defineCustomElement();
c.detach();
this.el = r.nativeElement;
proxyOutputs(this, this.el, ['ionCancel', 'ionChange', 'ionFocus', 'ionBlur']);
}

ngOnInit(): void {
/**
* Data-bound input properties are set
* by Angular after the constructor, so
* we need to run the proxy in ngOnInit.
*/
proxyInputs(IonDatetime, DATETIME_INPUTS);
proxyMethods(IonDatetime, DATETIME_METHODS);
}

@HostListener('ionChange', ['$event.target'])
handleIonChange(el: HTMLIonDatetimeElement): void {
this.handleValueChange(el, el.value);
Expand Down
35 changes: 28 additions & 7 deletions packages/angular/standalone/src/directives/input.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
Injector,
NgZone,
} from '@angular/core';
import type { OnInit } from '@angular/core';
import { NG_VALUE_ACCESSOR } from '@angular/forms';
import { ValueAccessor } from '@ionic/angular/common';
import type {
Expand All @@ -17,7 +18,19 @@ import type {
} from '@ionic/core/components';
import { defineCustomElement } from '@ionic/core/components/ion-input.js';

import { ProxyCmp, proxyOutputs } from './angular-component-lib/utils';
/**
* Value accessor components should not use ProxyCmp
* and should call defineCustomElement and proxyInputs
* manually instead. Using both the @ProxyCmp and @Component
* decorators and useExisting (where useExisting refers to the
* class) causes ng-packagr to output multiple component variables
* which breaks treeshaking.
* For example, the following would be generated:
* let IonInput = IonInput_1 = class IonInput extends ValueAccessor {
* Instead, we want only want the class generated:
* class IonInput extends ValueAccessor {
*/
import { proxyInputs, proxyMethods, proxyOutputs } from './angular-component-lib/utils';

const INPUT_INPUTS = [
'accept',
Expand Down Expand Up @@ -59,11 +72,8 @@ const INPUT_INPUTS = [
'value',
];

@ProxyCmp({
defineCustomElementFn: defineCustomElement,
inputs: INPUT_INPUTS,
methods: ['setFocus', 'getInputElement'],
})
const INPUT_METHODS = ['setFocus', 'getInputElement'];

@Component({
selector: 'ion-input',
changeDetection: ChangeDetectionStrategy.OnPush,
Expand All @@ -79,15 +89,26 @@ const INPUT_INPUTS = [
],
standalone: true,
})
export class IonInput extends ValueAccessor {
export class IonInput extends ValueAccessor implements OnInit {
protected el: HTMLElement;
constructor(c: ChangeDetectorRef, r: ElementRef, protected z: NgZone, injector: Injector) {
super(injector, r);
defineCustomElement();
c.detach();
this.el = r.nativeElement;
proxyOutputs(this, this.el, ['ionInput', 'ionChange', 'ionBlur', 'ionFocus']);
}

ngOnInit(): void {
/**
* Data-bound input properties are set
* by Angular after the constructor, so
* we need to run the proxy in ngOnInit.
*/
proxyInputs(IonInput, INPUT_INPUTS);
proxyMethods(IonInput, INPUT_METHODS);
}

@HostListener('ionInput', ['$event.target'])
handleIonInput(el: HTMLIonInputElement): void {
this.handleValueChange(el, el.value);
Expand Down
31 changes: 25 additions & 6 deletions packages/angular/standalone/src/directives/radio-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,28 @@ import {
Injector,
NgZone,
} from '@angular/core';
import type { OnInit } from '@angular/core';
import { NG_VALUE_ACCESSOR } from '@angular/forms';
import { ValueAccessor } from '@ionic/angular/common';
import type { RadioGroupChangeEventDetail, Components } from '@ionic/core/components';
import { defineCustomElement } from '@ionic/core/components/ion-radio-group.js';

import { ProxyCmp, proxyOutputs } from './angular-component-lib/utils';
/**
* Value accessor components should not use ProxyCmp
* and should call defineCustomElement and proxyInputs
* manually instead. Using both the @ProxyCmp and @Component
* decorators and useExisting (where useExisting refers to the
* class) causes ng-packagr to output multiple component variables
* which breaks treeshaking.
* For example, the following would be generated:
* let IonRadioGroup = IonRadioGroup_1 = class IonRadioGroup extends ValueAccessor {
* Instead, we want only want the class generated:
* class IonRadioGroup extends ValueAccessor {
*/
import { proxyInputs, proxyOutputs } from './angular-component-lib/utils';

const RADIO_GROUP_INPUTS = ['allowEmptySelection', 'name', 'value'];

@ProxyCmp({
defineCustomElementFn: defineCustomElement,
inputs: RADIO_GROUP_INPUTS,
})
@Component({
selector: 'ion-radio-group',
changeDetection: ChangeDetectionStrategy.OnPush,
Expand All @@ -36,15 +45,25 @@ const RADIO_GROUP_INPUTS = ['allowEmptySelection', 'name', 'value'];
],
standalone: true,
})
export class IonRadioGroup extends ValueAccessor {
export class IonRadioGroup extends ValueAccessor implements OnInit {
protected el: HTMLElement;
constructor(c: ChangeDetectorRef, r: ElementRef, protected z: NgZone, injector: Injector) {
super(injector, r);
defineCustomElement();
c.detach();
this.el = r.nativeElement;
proxyOutputs(this, this.el, ['ionChange']);
}

ngOnInit(): void {
/**
* Data-bound input properties are set
* by Angular after the constructor, so
* we need to run the proxy in ngOnInit.
*/
proxyInputs(IonRadioGroup, RADIO_GROUP_INPUTS);
}

@HostListener('ionChange', ['$event.target'])
handleIonChange(el: HTMLIonRadioGroupElement): void {
this.handleValueChange(el, el.value);
Expand Down
31 changes: 25 additions & 6 deletions packages/angular/standalone/src/directives/radio.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,28 @@ import {
Injector,
NgZone,
} from '@angular/core';
import type { OnInit } from '@angular/core';
import { NG_VALUE_ACCESSOR } from '@angular/forms';
import { ValueAccessor } from '@ionic/angular/common';
import type { Components } from '@ionic/core/components';
import { defineCustomElement } from '@ionic/core/components/ion-radio.js';

import { ProxyCmp, proxyOutputs } from './angular-component-lib/utils';
/**
* Value accessor components should not use ProxyCmp
* and should call defineCustomElement and proxyInputs
* manually instead. Using both the @ProxyCmp and @Component
* decorators and useExisting (where useExisting refers to the
* class) causes ng-packagr to output multiple component variables
* which breaks treeshaking.
* For example, the following would be generated:
* let IonRadio = IonRadio_1 = class IonRadio extends ValueAccessor {
* Instead, we want only want the class generated:
* class IonRadio extends ValueAccessor {
*/
import { proxyInputs, proxyOutputs } from './angular-component-lib/utils';

const RADIO_INPUTS = ['color', 'disabled', 'justify', 'labelPlacement', 'legacy', 'mode', 'name', 'value'];

@ProxyCmp({
defineCustomElementFn: defineCustomElement,
inputs: RADIO_INPUTS,
})
@Component({
selector: 'ion-radio',
changeDetection: ChangeDetectionStrategy.OnPush,
Expand All @@ -36,15 +45,25 @@ const RADIO_INPUTS = ['color', 'disabled', 'justify', 'labelPlacement', 'legacy'
],
standalone: true,
})
export class IonRadio extends ValueAccessor {
export class IonRadio extends ValueAccessor implements OnInit {
protected el: HTMLElement;
constructor(c: ChangeDetectorRef, r: ElementRef, protected z: NgZone, injector: Injector) {
super(injector, r);
defineCustomElement();
c.detach();
this.el = r.nativeElement;
proxyOutputs(this, this.el, ['ionFocus', 'ionBlur']);
}

ngOnInit(): void {
/**
* Data-bound input properties are set
* by Angular after the constructor, so
* we need to run the proxy in ngOnInit.
*/
proxyInputs(IonRadio, RADIO_INPUTS);
}

@HostListener('ionSelect', ['$event.target'])
handleIonSelect(el: any): void {
/**
Expand Down
31 changes: 25 additions & 6 deletions packages/angular/standalone/src/directives/range.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
Injector,
NgZone,
} from '@angular/core';
import type { OnInit } from '@angular/core';
import { NG_VALUE_ACCESSOR } from '@angular/forms';
import { ValueAccessor } from '@ionic/angular/common';
import type {
Expand All @@ -18,7 +19,19 @@ import type {
} from '@ionic/core/components';
import { defineCustomElement } from '@ionic/core/components/ion-range.js';

import { ProxyCmp, proxyOutputs } from './angular-component-lib/utils';
/**
* Value accessor components should not use ProxyCmp
* and should call defineCustomElement and proxyInputs
* manually instead. Using both the @ProxyCmp and @Component
* decorators and useExisting (where useExisting refers to the
* class) causes ng-packagr to output multiple component variables
* which breaks treeshaking.
* For example, the following would be generated:
* let IonRange = IonRange_1 = class IonRange extends ValueAccessor {
* Instead, we want only want the class generated:
* class IonRange extends ValueAccessor {
*/
import { proxyInputs, proxyOutputs } from './angular-component-lib/utils';

const RANGE_INPUTS = [
'activeBarStart',
Expand All @@ -41,10 +54,6 @@ const RANGE_INPUTS = [
'value',
];

@ProxyCmp({
defineCustomElementFn: defineCustomElement,
inputs: RANGE_INPUTS,
})
@Component({
selector: 'ion-range',
changeDetection: ChangeDetectionStrategy.OnPush,
Expand All @@ -60,15 +69,25 @@ const RANGE_INPUTS = [
],
standalone: true,
})
export class IonRange extends ValueAccessor {
export class IonRange extends ValueAccessor implements OnInit {
protected el: HTMLElement;
constructor(c: ChangeDetectorRef, r: ElementRef, protected z: NgZone, injector: Injector) {
super(injector, r);
defineCustomElement();
c.detach();
this.el = r.nativeElement;
proxyOutputs(this, this.el, ['ionChange', 'ionInput', 'ionFocus', 'ionBlur', 'ionKnobMoveStart', 'ionKnobMoveEnd']);
}

ngOnInit(): void {
/**
* Data-bound input properties are set
* by Angular after the constructor, so
* we need to run the proxy in ngOnInit.
*/
proxyInputs(IonRange, RANGE_INPUTS);
}

@HostListener('ionChange', ['$event.target'])
handleIonChange(el: HTMLIonRangeElement): void {
this.handleValueChange(el, el.value);
Expand Down
Loading

0 comments on commit 82d6309

Please sign in to comment.