Skip to content

Commit

Permalink
fix(angular): inputs on standalone form controls are reactive (#28434)
Browse files Browse the repository at this point in the history
Issue number: resolves #28431

---------

<!-- 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. -->

My previous attempt at fixing
#28358 caused inputs
to no longer be correctly proxied to the underlying components. This was
an attempt to work around an underlying ng-packagr bug (see linked
thread for more info).

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

I decided it would be best to continue using `ProxyCmp` (since we know
that works) and find an alternative to working around the ng-packagr
bug. I spoke with the Angular team, and they recommended pulling the
provider into its own object. `forwardRef` is now required since we are
referencing the component before it is declared.

- Revert
82d6309
- Moves provider to an object to avoid ng-packagr issue
- I reverted the proxy e2e tests. These are no longer needed since we
are not ejecting from the typical `ProxyCmp` usage anymore.

## 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. -->
Dev build: `7.5.3-dev.11698699090.1151d73f`
Verified that the issue is fixed with the repro provided in
#28431

Also verified that this does not regress the issue described in
#28358.
  • Loading branch information
liamdebeasi authored Oct 31, 2023
1 parent 89698b3 commit 3b6e631
Show file tree
Hide file tree
Showing 21 changed files with 252 additions and 420 deletions.
54 changes: 22 additions & 32 deletions packages/angular/standalone/src/directives/checkbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,26 +7,14 @@ import {
HostListener,
Injector,
NgZone,
forwardRef,
} 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';

/**
* 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';
import { ProxyCmp, proxyOutputs } from './angular-component-lib/utils';

const CHECKBOX_INPUTS = [
'checked',
Expand All @@ -41,40 +29,42 @@ const CHECKBOX_INPUTS = [
'value',
];

/**
* Pulling the provider into an object and using PURE works
* around an ng-packagr issue that causes
* components with multiple decorators and
* a provider to be re-assigned. This re-assignment
* is not supported by Webpack and causes treeshaking
* to not work on these kinds of components.
*/
const accessorProvider = {
provide: NG_VALUE_ACCESSOR,
useExisting: /*@__PURE__*/ forwardRef(() => IonCheckbox),
multi: true,
};

@ProxyCmp({
defineCustomElementFn: defineCustomElement,
inputs: CHECKBOX_INPUTS,
})
@Component({
selector: 'ion-checkbox',
changeDetection: ChangeDetectionStrategy.OnPush,
template: '<ng-content></ng-content>',
// eslint-disable-next-line @angular-eslint/no-inputs-metadata-property
inputs: CHECKBOX_INPUTS,
providers: [
{
provide: NG_VALUE_ACCESSOR,
useExisting: IonCheckbox,
multi: true,
},
],
providers: [accessorProvider],
standalone: true,
})
export class IonCheckbox extends ValueAccessor implements OnInit {
export class IonCheckbox extends ValueAccessor {
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
57 changes: 23 additions & 34 deletions packages/angular/standalone/src/directives/datetime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,26 +7,14 @@ import {
HostListener,
Injector,
NgZone,
forwardRef,
} 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';

/**
* 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';
import { ProxyCmp, proxyOutputs } from './angular-component-lib/utils';

const DATETIME_INPUTS = [
'cancelText',
Expand Down Expand Up @@ -61,43 +49,44 @@ const DATETIME_INPUTS = [
'yearValues',
];

const DATETIME_METHODS = ['confirm', 'reset', 'cancel'];
/**
* Pulling the provider into an object and using PURE works
* around an ng-packagr issue that causes
* components with multiple decorators and
* a provider to be re-assigned. This re-assignment
* is not supported by Webpack and causes treeshaking
* to not work on these kinds of components.
*/
const accessorProvider = {
provide: NG_VALUE_ACCESSOR,
useExisting: /*@__PURE__*/ forwardRef(() => IonDatetime),
multi: true,
};

@ProxyCmp({
defineCustomElementFn: defineCustomElement,
inputs: DATETIME_INPUTS,
methods: ['confirm', 'reset', 'cancel'],
})
@Component({
selector: 'ion-datetime',
changeDetection: ChangeDetectionStrategy.OnPush,
template: '<ng-content></ng-content>',
// eslint-disable-next-line @angular-eslint/no-inputs-metadata-property
inputs: DATETIME_INPUTS,
providers: [
{
provide: NG_VALUE_ACCESSOR,
useExisting: IonDatetime,
multi: true,
},
],
providers: [accessorProvider],
standalone: true,
})
export class IonDatetime extends ValueAccessor implements OnInit {
export class IonDatetime extends ValueAccessor {
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
56 changes: 22 additions & 34 deletions packages/angular/standalone/src/directives/input.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import {
HostListener,
Injector,
NgZone,
forwardRef,
} 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,19 +18,7 @@ import type {
} from '@ionic/core/components';
import { defineCustomElement } from '@ionic/core/components/ion-input.js';

/**
* 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';
import { ProxyCmp, proxyOutputs } from './angular-component-lib/utils';

const INPUT_INPUTS = [
'accept',
Expand Down Expand Up @@ -72,43 +60,43 @@ const INPUT_INPUTS = [
'value',
];

const INPUT_METHODS = ['setFocus', 'getInputElement'];
/**
* Pulling the provider into an object and using PURE works
* around an ng-packagr issue that causes
* components with multiple decorators and
* a provider to be re-assigned. This re-assignment
* is not supported by Webpack and causes treeshaking
* to not work on these kinds of components.
*/
const accessorProvider = {
provide: NG_VALUE_ACCESSOR,
useExisting: /*@__PURE__*/ forwardRef(() => IonInput),
multi: true,
};

@ProxyCmp({
defineCustomElementFn: defineCustomElement,
inputs: INPUT_INPUTS,
methods: ['setFocus', 'getInputElement'],
})
@Component({
selector: 'ion-input',
changeDetection: ChangeDetectionStrategy.OnPush,
template: '<ng-content></ng-content>',
// eslint-disable-next-line @angular-eslint/no-inputs-metadata-property
inputs: INPUT_INPUTS,
providers: [
{
provide: NG_VALUE_ACCESSOR,
useExisting: IonInput,
multi: true,
},
],
providers: [accessorProvider],
standalone: true,
})
export class IonInput extends ValueAccessor implements OnInit {
export class IonInput extends ValueAccessor {
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
54 changes: 22 additions & 32 deletions packages/angular/standalone/src/directives/radio-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,63 +7,53 @@ import {
HostListener,
Injector,
NgZone,
forwardRef,
} 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';

/**
* 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';
import { ProxyCmp, proxyOutputs } from './angular-component-lib/utils';

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

/**
* Pulling the provider into an object and using PURE works
* around an ng-packagr issue that causes
* components with multiple decorators and
* a provider to be re-assigned. This re-assignment
* is not supported by Webpack and causes treeshaking
* to not work on these kinds of components.
*/
const accessorProvider = {
provide: NG_VALUE_ACCESSOR,
useExisting: /*@__PURE__*/ forwardRef(() => IonRadioGroup),
multi: true,
};

@ProxyCmp({
defineCustomElementFn: defineCustomElement,
inputs: RADIO_GROUP_INPUTS,
})
@Component({
selector: 'ion-radio-group',
changeDetection: ChangeDetectionStrategy.OnPush,
template: '<ng-content></ng-content>',
// eslint-disable-next-line @angular-eslint/no-inputs-metadata-property
inputs: RADIO_GROUP_INPUTS,
providers: [
{
provide: NG_VALUE_ACCESSOR,
useExisting: IonRadioGroup,
multi: true,
},
],
providers: [accessorProvider],
standalone: true,
})
export class IonRadioGroup extends ValueAccessor implements OnInit {
export class IonRadioGroup extends ValueAccessor {
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
Loading

0 comments on commit 3b6e631

Please sign in to comment.