From f89d004c519fd1645b9f94ff40cd740ad3658360 Mon Sep 17 00:00:00 2001 From: Dzmitry Shylovich Date: Sat, 10 Dec 2016 00:10:40 +0300 Subject: [PATCH] fix(forms): provide a method to compare options (#13349) Closes #13268 PR Close #13349 --- .../select_control_value_accessor.ts | 37 +++- .../select_multiple_control_value_accessor.ts | 37 +++- .../forms/test/reactive_integration_spec.ts | 184 +++++++++++++++-- .../forms/test/template_integration_spec.ts | 188 +++++++++++++----- tools/public_api_guard/forms/index.d.ts | 2 + 5 files changed, 372 insertions(+), 76 deletions(-) diff --git a/modules/@angular/forms/src/directives/select_control_value_accessor.ts b/modules/@angular/forms/src/directives/select_control_value_accessor.ts index dd07779d1c..f8423ff8d5 100644 --- a/modules/@angular/forms/src/directives/select_control_value_accessor.ts +++ b/modules/@angular/forms/src/directives/select_control_value_accessor.ts @@ -57,6 +57,31 @@ function _extractId(valueString: string): string { * * {@example forms/ts/reactiveSelectControl/reactive_select_control_example.ts region='Component'} * + * ### Caveat: Option selection + * + * Angular uses object identity to select option. It's possible for the identities of items + * to change while the data does not. This can happen, for example, if the items are produced + * from an RPC to the server, and that RPC is re-run. Even if the data hasn't changed, the + * second response will produce objects with different identities. + * + * To customize the default option comparison algorithm, ` + * + * + * + * compareFn(c1: Country, c2: Country): boolean { + * return c1 && c2 ? c1.id === c2.id : c1 === c2; + * } + * ``` + * * Note: We listen to the 'change' event because 'input' events aren't fired * for selects in Firefox and IE: * https://bugzilla.mozilla.org/show_bug.cgi?id=1024350 @@ -82,6 +107,16 @@ export class SelectControlValueAccessor implements ControlValueAccessor { onChange = (_: any) => {}; onTouched = () => {}; + @Input() + set compareWith(fn: (o1: any, o2: any) => boolean) { + if (typeof fn !== 'function') { + throw new Error(`compareWith must be a function, but received ${JSON.stringify(fn)}`); + } + this._compareWith = fn; + } + + private _compareWith: (o1: any, o2: any) => boolean = looseIdentical; + constructor(private _renderer: Renderer, private _elementRef: ElementRef) {} writeValue(value: any): void { @@ -112,7 +147,7 @@ export class SelectControlValueAccessor implements ControlValueAccessor { /** @internal */ _getOptionId(value: any): string { for (const id of Array.from(this._optionMap.keys())) { - if (looseIdentical(this._optionMap.get(id), value)) return id; + if (this._compareWith(this._optionMap.get(id), value)) return id; } return null; } diff --git a/modules/@angular/forms/src/directives/select_multiple_control_value_accessor.ts b/modules/@angular/forms/src/directives/select_multiple_control_value_accessor.ts index 105ead76c1..72639a39b9 100644 --- a/modules/@angular/forms/src/directives/select_multiple_control_value_accessor.ts +++ b/modules/@angular/forms/src/directives/select_multiple_control_value_accessor.ts @@ -44,6 +44,31 @@ abstract class HTMLCollection { /** * The accessor for writing a value and listening to changes on a select element. * + * ### Caveat: Options selection + * + * Angular uses object identity to select options. It's possible for the identities of items + * to change while the data does not. This can happen, for example, if the items are produced + * from an RPC to the server, and that RPC is re-run. Even if the data hasn't changed, the + * second response will produce objects with different identities. + * + * To customize the default option comparison algorithm, ` + * + * + * + * compareFn(c1: Country, c2: Country): boolean { + * return c1 && c2 ? c1.id === c2.id : c1 === c2; + * } + * ``` + * * @stable */ @Directive({ @@ -62,6 +87,16 @@ export class SelectMultipleControlValueAccessor implements ControlValueAccessor onChange = (_: any) => {}; onTouched = () => {}; + @Input() + set compareWith(fn: (o1: any, o2: any) => boolean) { + if (typeof fn !== 'function') { + throw new Error(`compareWith must be a function, but received ${JSON.stringify(fn)}`); + } + this._compareWith = fn; + } + + private _compareWith: (o1: any, o2: any) => boolean = looseIdentical; + constructor(private _renderer: Renderer, private _elementRef: ElementRef) {} writeValue(value: any): void { @@ -119,7 +154,7 @@ export class SelectMultipleControlValueAccessor implements ControlValueAccessor /** @internal */ _getOptionId(value: any): string { for (const id of Array.from(this._optionMap.keys())) { - if (looseIdentical(this._optionMap.get(id)._value, value)) return id; + if (this._compareWith(this._optionMap.get(id)._value, value)) return id; } return null; } diff --git a/modules/@angular/forms/test/reactive_integration_spec.ts b/modules/@angular/forms/test/reactive_integration_spec.ts index 1a6c0f9fa2..42cdf28f64 100644 --- a/modules/@angular/forms/test/reactive_integration_spec.ts +++ b/modules/@angular/forms/test/reactive_integration_spec.ts @@ -338,6 +338,95 @@ export function main() { }); + describe('select controls', () => { + it(`should support primitive values`, () => { + const fixture = initTest(FormControlNameSelect); + fixture.detectChanges(); + + // model -> view + const select = fixture.debugElement.query(By.css('select')); + const sfOption = fixture.debugElement.query(By.css('option')); + expect(select.nativeElement.value).toEqual('SF'); + expect(sfOption.nativeElement.selected).toBe(true); + + select.nativeElement.value = 'NY'; + dispatchEvent(select.nativeElement, 'change'); + fixture.detectChanges(); + + // view -> model + expect(sfOption.nativeElement.selected).toBe(false); + expect(fixture.componentInstance.form.value).toEqual({'city': 'NY'}); + }); + + it(`should support objects`, () => { + const fixture = initTest(FormControlSelectNgValue); + fixture.detectChanges(); + + // model -> view + const select = fixture.debugElement.query(By.css('select')); + const sfOption = fixture.debugElement.query(By.css('option')); + expect(select.nativeElement.value).toEqual('0: Object'); + expect(sfOption.nativeElement.selected).toBe(true); + }); + + it('should throw an error if compareWith is not a function', () => { + const fixture = initTest(FormControlSelectWithCompareFn); + fixture.componentInstance.compareFn = null; + expect(() => fixture.detectChanges()) + .toThrowError(/compareWith must be a function, but received null/); + }); + + it('should compare options using provided compareWith function', () => { + const fixture = initTest(FormControlSelectWithCompareFn); + fixture.detectChanges(); + + const select = fixture.debugElement.query(By.css('select')); + const sfOption = fixture.debugElement.query(By.css('option')); + expect(select.nativeElement.value).toEqual('0: Object'); + expect(sfOption.nativeElement.selected).toBe(true); + }); + }); + + describe('select multiple controls', () => { + it('should support primitive values', () => { + const fixture = initTest(FormControlSelectMultiple); + fixture.detectChanges(); + + const select = fixture.debugElement.query(By.css('select')); + const sfOption = fixture.debugElement.query(By.css('option')); + expect(select.nativeElement.value).toEqual(`0: 'SF'`); + expect(sfOption.nativeElement.selected).toBe(true); + }); + + it('should support objects', () => { + const fixture = initTest(FormControlSelectMultipleNgValue); + fixture.detectChanges(); + + const select = fixture.debugElement.query(By.css('select')); + const sfOption = fixture.debugElement.query(By.css('option')); + expect(select.nativeElement.value).toEqual('0: Object'); + expect(sfOption.nativeElement.selected).toBe(true); + }); + + it('should throw an error when compareWith is not a function', () => { + const fixture = initTest(FormControlSelectMultipleWithCompareFn); + fixture.componentInstance.compareFn = null; + expect(() => fixture.detectChanges()) + .toThrowError(/compareWith must be a function, but received null/); + }); + + it('should compare options using provided compareWith function', fakeAsync(() => { + const fixture = initTest(FormControlSelectMultipleWithCompareFn); + fixture.detectChanges(); + tick(); + + const select = fixture.debugElement.query(By.css('select')); + const sfOption = fixture.debugElement.query(By.css('option')); + expect(select.nativeElement.value).toEqual('0: Object'); + expect(sfOption.nativeElement.selected).toBe(true); + })); + }); + describe('form arrays', () => { it('should support form arrays', () => { const fixture = initTest(FormArrayComp); @@ -835,25 +924,6 @@ export function main() { expect(control.value).toBe(false); }); - it('should support + + + ` +}) +class FormControlSelectNgValue { + cities = [{id: 1, name: 'SF'}, {id: 2, name: 'NY'}]; + form = new FormGroup({city: new FormControl(this.cities[0])}); +} + +@Component({ + selector: 'form-control-select-compare-with', + template: ` +
+ +
` +}) +class FormControlSelectWithCompareFn { + compareFn: (o1: any, o2: any) => boolean = (o1: any, o2: any) => { + return o1 && o2 ? o1.id === o2.id : o1 === o2; + }; + cities = [{id: 1, name: 'SF'}, {id: 2, name: 'NY'}]; + form = new FormGroup({city: new FormControl({id: 1, name: 'SF'})}); +} + +@Component({ + selector: 'form-control-select-multiple', + template: ` +
+ +
` +}) +class FormControlSelectMultiple { + cities = ['SF', 'NY']; + form = new FormGroup({city: new FormControl(['SF'])}); +} + +@Component({ + selector: 'form-control-select-multiple', + template: ` +
+ +
` +}) +class FormControlSelectMultipleNgValue { + cities = [{id: 1, name: 'SF'}, {id: 2, name: 'NY'}]; + form = new FormGroup({city: new FormControl([this.cities[0]])}); +} + +@Component({ + selector: 'form-control-select-multiple-compare-with', + template: ` +
+ +
` +}) +class FormControlSelectMultipleWithCompareFn { + compareFn: (o1: any, o2: any) => boolean = (o1: any, o2: any) => { + return o1 && o2 ? o1.id === o2.id : o1 === o2; + }; + cities = [{id: 1, name: 'SF'}, {id: 2, name: 'NY'}]; + form = new FormGroup({city: new FormControl([{id: 1, name: 'SF'}])}); +} + @Component({ selector: 'wrapped-value-form', template: ` diff --git a/modules/@angular/forms/test/template_integration_spec.ts b/modules/@angular/forms/test/template_integration_spec.ts index 02ff099a82..79f10f9aee 100644 --- a/modules/@angular/forms/test/template_integration_spec.ts +++ b/modules/@angular/forms/test/template_integration_spec.ts @@ -764,69 +764,115 @@ export function main() { tick(); expect(comp.selectedCity).toEqual(null); })); + + it('should throw an error when compareWith is not a function', () => { + const fixture = initTest(NgModelSelectWithCustomCompareFnForm); + const comp = fixture.componentInstance; + comp.compareFn = null; + expect(() => fixture.detectChanges()) + .toThrowError(/compareWith must be a function, but received null/); + }); + + it('should compare options using provided compareWith function', fakeAsync(() => { + const fixture = initTest(NgModelSelectWithCustomCompareFnForm); + const comp = fixture.componentInstance; + comp.selectedCity = {id: 1, name: 'SF'}; + comp.cities = [{id: 1, name: 'SF'}, {id: 2, name: 'LA'}]; + fixture.detectChanges(); + tick(); + + const select = fixture.debugElement.query(By.css('select')); + const sfOption = fixture.debugElement.query(By.css('option')); + expect(select.nativeElement.value).toEqual('0: Object'); + expect(sfOption.nativeElement.selected).toBe(true); + })); }); describe('select multiple controls', () => { - let fixture: ComponentFixture; - let comp: NgModelSelectMultipleForm; + describe('select options', () => { + let fixture: ComponentFixture; + let comp: NgModelSelectMultipleForm; - beforeEach(() => { - fixture = initTest(NgModelSelectMultipleForm); - comp = fixture.componentInstance; - comp.cities = [{'name': 'SF'}, {'name': 'NYC'}, {'name': 'Buffalo'}]; + beforeEach(() => { + fixture = initTest(NgModelSelectMultipleForm); + comp = fixture.componentInstance; + comp.cities = [{'name': 'SF'}, {'name': 'NYC'}, {'name': 'Buffalo'}]; + }); + + const detectChangesAndTick = (): void => { + fixture.detectChanges(); + tick(); + }; + + const setSelectedCities = (selectedCities: any): void => { + comp.selectedCities = selectedCities; + detectChangesAndTick(); + }; + + const selectOptionViaUI = (valueString: string): void => { + const select = fixture.debugElement.query(By.css('select')); + select.nativeElement.value = valueString; + dispatchEvent(select.nativeElement, 'change'); + detectChangesAndTick(); + }; + + const assertOptionElementSelectedState = (selectedStates: boolean[]): void => { + const options = fixture.debugElement.queryAll(By.css('option')); + if (options.length !== selectedStates.length) { + throw 'the selected state values to assert does not match the number of options'; + } + for (let i = 0; i < selectedStates.length; i++) { + expect(options[i].nativeElement.selected).toBe(selectedStates[i]); + } + }; + + it('should reflect state of model after option selected and new options subsequently added', + fakeAsync(() => { + setSelectedCities([]); + + selectOptionViaUI('1: Object'); + assertOptionElementSelectedState([false, true, false]); + + comp.cities.push({'name': 'Chicago'}); + detectChangesAndTick(); + + assertOptionElementSelectedState([false, true, false, false]); + })); + + it('should reflect state of model after option selected and then other options removed', + fakeAsync(() => { + setSelectedCities([]); + + selectOptionViaUI('1: Object'); + assertOptionElementSelectedState([false, true, false]); + + comp.cities.pop(); + detectChangesAndTick(); + + assertOptionElementSelectedState([false, true]); + })); }); - const detectChangesAndTick = (): void => { - fixture.detectChanges(); - tick(); - }; + it('should throw an error when compareWith is not a function', () => { + const fixture = initTest(NgModelSelectMultipleWithCustomCompareFnForm); + const comp = fixture.componentInstance; + comp.compareFn = null; + expect(() => fixture.detectChanges()) + .toThrowError(/compareWith must be a function, but received null/); + }); - const setSelectedCities = (selectedCities: any): void => { - comp.selectedCities = selectedCities; - detectChangesAndTick(); - }; + it('should compare options using provided compareWith function', fakeAsync(() => { + const fixture = initTest(NgModelSelectMultipleWithCustomCompareFnForm); + const comp = fixture.componentInstance; + comp.cities = [{id: 1, name: 'SF'}, {id: 2, name: 'LA'}]; + comp.selectedCities = [comp.cities[0]]; + fixture.detectChanges(); + tick(); - const selectOptionViaUI = (valueString: string): void => { - const select = fixture.debugElement.query(By.css('select')); - select.nativeElement.value = valueString; - dispatchEvent(select.nativeElement, 'change'); - detectChangesAndTick(); - }; - - const assertOptionElementSelectedState = (selectedStates: boolean[]): void => { - const options = fixture.debugElement.queryAll(By.css('option')); - if (options.length !== selectedStates.length) { - throw 'the selected state values to assert does not match the number of options'; - } - for (let i = 0; i < selectedStates.length; i++) { - expect(options[i].nativeElement.selected).toBe(selectedStates[i]); - } - }; - - it('should reflect state of model after option selected and new options subsequently added', - fakeAsync(() => { - setSelectedCities([]); - - selectOptionViaUI('1: Object'); - assertOptionElementSelectedState([false, true, false]); - - comp.cities.push({'name': 'Chicago'}); - detectChangesAndTick(); - - assertOptionElementSelectedState([false, true, false, false]); - })); - - it('should reflect state of model after option selected and then other options removed', - fakeAsync(() => { - setSelectedCities([]); - - selectOptionViaUI('1: Object'); - assertOptionElementSelectedState([false, true, false]); - - comp.cities.pop(); - detectChangesAndTick(); - - assertOptionElementSelectedState([false, true]); + const select = fixture.debugElement.query(By.css('select')); + const sfOption = fixture.debugElement.query(By.css('option')); + expect(select.nativeElement.value).toEqual('0: Object'); + expect(sfOption.nativeElement.selected).toBe(true); })); }); @@ -1314,6 +1360,38 @@ class NgModelSelectWithNullForm { cities: any[] = []; } +@Component({ + selector: 'ng-model-select-compare-with', + template: ` + + ` +}) +class NgModelSelectWithCustomCompareFnForm { + compareFn: (o1: any, o2: any) => boolean = (o1: any, o2: any) => { + return o1 && o2 ? o1.id === o2.id : o1 === o2; + }; + selectedCity: any = {}; + cities: any[] = []; +} + +@Component({ + selector: 'ng-model-select-multiple-compare-with', + template: ` + + ` +}) +class NgModelSelectMultipleWithCustomCompareFnForm { + compareFn: (o1: any, o2: any) => boolean = (o1: any, o2: any) => { + return o1 && o2 ? o1.id === o2.id : o1 === o2; + }; + selectedCities: any[] = []; + cities: any[] = []; +} + @Component({ selector: 'ng-model-select-multiple-form', template: ` diff --git a/tools/public_api_guard/forms/index.d.ts b/tools/public_api_guard/forms/index.d.ts index ad38179806..20893f1696 100644 --- a/tools/public_api_guard/forms/index.d.ts +++ b/tools/public_api_guard/forms/index.d.ts @@ -499,6 +499,7 @@ export declare class RequiredValidator implements Validator { /** @stable */ export declare class SelectControlValueAccessor implements ControlValueAccessor { + compareWith: (o1: any, o2: any) => boolean; onChange: (_: any) => void; onTouched: () => void; value: any; @@ -511,6 +512,7 @@ export declare class SelectControlValueAccessor implements ControlValueAccessor /** @stable */ export declare class SelectMultipleControlValueAccessor implements ControlValueAccessor { + compareWith: (o1: any, o2: any) => boolean; onChange: (_: any) => void; onTouched: () => void; value: any;