cargdev/angular
1
0
Fork 0
Code Issues 6 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(common): cleanup the StylingDiffer and related code (#34307)

Browse Source 
Since I was learning the codebase and had a hard time understanding what was going on I've done a
bunch of changes in one commit that under normal circumstances should have been split into several
commits. Because this code is likely going to be overwritten with Misko's changes I'm not going to
spend the time with trying to split this up.

Overall I've done the following:
- I processed review feedback from #34307
- I did a bunch of renaming to make the code easier to understand
- I refactored some internal functions that were either inefficient or hard to read
- I also updated lots of type signatures to correct them and to remove many casts in the code

PR Close #34307
This commit is contained in:
Igor Minar
2020-01-11 11:23:53 -08:00
committed by Matias Niemelä
parent 93a035f7f6
commit 21a9a41c5b
8 changed files with 309 additions and 236 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
packages/common/test/directives/ng_class_spec.ts
View File

@ -196,7 +196,7 @@ import {ComponentFixture, TestBed, async} from '@angular/core/testing';
fixture = createTestComponent(`<div [ngClass]="['foo', {}]"></div>`);
expect(() => fixture !.detectChanges())
.toThrowError(
/NgClass can only toggle CSS classes expressed as strings, got \[object Object\]/);
/NgClass can only toggle CSS classes expressed as strings, got: \[object Object\]/);
});
});
@ -353,17 +353,23 @@ import {ComponentFixture, TestBed, async} from '@angular/core/testing';
}));
});
describe('non-regression', () => {
describe('prevent regressions', () => {
// https://github.com/angular/angular/issues/34336
it('should not write to native node when a bound expression doesnt change', () => {
fixture = createTestComponent(`<div [ngClass]="{'color-red': true}"></div>`);
it('should not write to the native node unless the bound expression has changed', () => {
fixture = createTestComponent(`<div [ngClass]="{'color-red': condition}"></div>`);
detectChangesAndExpectClassName('color-red');
// Overwrite CSS classes to make sure that ngClass is not doing any DOM manipulation (as
// there was no change to the expression bound to [ngClass]).
// Overwrite CSS classes so that we can check if ngClass performed DOM manipulation to
// update it
fixture.debugElement.children[0].nativeElement.className = '';
// Assert that the DOM node still has the same value after change detection
detectChangesAndExpectClassName('');
fixture.componentInstance.condition = false;
fixture.detectChanges();
fixture.componentInstance.condition = true;
detectChangesAndExpectClassName('color-red');
});
});

25
packages/common/test/directives/ng_style_spec.ts
View File

@ -12,7 +12,7 @@ import {ComponentFixture, TestBed, async} from '@angular/core/testing';
{
describe('NgStyle', () => {
let fixture: ComponentFixture<any>;
let fixture: ComponentFixture<TestComponent>;
function getComponent(): TestComponent { return fixture.componentInstance; }
@ -158,21 +158,36 @@ import {ComponentFixture, TestBed, async} from '@angular/core/testing';
expectNativeEl(fixture).toHaveCssStyle({'font-size': '12px'});
}));
it('should not write to native node when a bound expression doesnt change', () => {
it('should not write to the native node unless the bound expression has changed', () => {
const template = `<div [ngStyle]="{'color': 'red'}"></div>`;
const template = `<div [ngStyle]="{'color': expr}"></div>`;
fixture = createTestComponent(template);
fixture.componentInstance.expr = 'red';
fixture.detectChanges();
expectNativeEl(fixture).toHaveCssStyle({'color': 'red'});
// Overwrite native styles to make sure that ngClass is not doing any DOM manipulation (as
// there was no change to the expression bound to [ngStyle]).
// Overwrite native styles so that we can check if ngStyle has performed DOM manupulation to
// update it.
fixture.debugElement.children[0].nativeElement.style.color = 'blue';
fixture.detectChanges();
// Assert that the style hasn't been updated
expectNativeEl(fixture).toHaveCssStyle({'color': 'blue'});
fixture.componentInstance.expr = 'yellow';
fixture.detectChanges();
// Assert that the style has changed now that the model has changed
expectNativeEl(fixture).toHaveCssStyle({'color': 'yellow'});
});
it('should correctly update style with units (.px) when the model is set to number', () => {
const template = `<div [ngStyle]="{'width.px': expr}"></div>`;
fixture = createTestComponent(template);
fixture.componentInstance.expr = 400;
fixture.detectChanges();
expectNativeEl(fixture).toHaveCssStyle({'width': '400px'});
});
});

154
packages/common/test/directives/styling_differ_spec.ts
View File

@ -13,101 +13,111 @@ describe('StylingDiffer', () => {
'ngClass', StylingDifferOptions.ForceAsMap | StylingDifferOptions.AllowStringValue);
expect(d.value).toEqual(null);
d.setValue('one two');
d.setInput('one two');
expect(d.value).toEqual({one: true, two: true});
d.setValue('three');
d.setInput('three');
expect(d.value).toEqual({three: true});
});
it('should not emit that a value has changed if a new non-collection value was not set', () => {
const d = new StylingDiffer(
'ngClass', StylingDifferOptions.ForceAsMap | StylingDifferOptions.AllowStringValue);
expect(d.value).toEqual(null);
d.setValue('one two');
expect(d.hasValueChanged()).toBeTruthy();
expect(d.value).toEqual({one: true, two: true});
expect(d.hasValueChanged()).toBeFalsy();
expect(d.value).toEqual({one: true, two: true});
describe('setInput', () => {
d.setValue('three');
expect(d.hasValueChanged()).toBeTruthy();
expect(d.value).toEqual({three: true});
expect(d.hasValueChanged()).toBeFalsy();
expect(d.value).toEqual({three: true});
it('should not emit that a value has changed if a new non-collection value was not set', () => {
const d = new StylingDiffer(
'ngClass', StylingDifferOptions.ForceAsMap | StylingDifferOptions.AllowStringValue);
expect(d.value).toEqual(null);
d.setValue(null);
expect(d.hasValueChanged()).toBeTruthy();
expect(d.value).toEqual(null);
expect(d.hasValueChanged()).toBeFalsy();
expect(d.value).toEqual(null);
d.setInput('one two');
expect(d.updateValue()).toBeTruthy();
expect(d.value).toEqual({one: true, two: true});
expect(d.updateValue()).toBeFalsy();
expect(d.value).toEqual({one: true, two: true});
d.setInput('three');
expect(d.updateValue()).toBeTruthy();
expect(d.value).toEqual({three: true});
expect(d.updateValue()).toBeFalsy();
expect(d.value).toEqual({three: true});
d.setInput(null);
expect(d.updateValue()).toBeTruthy();
expect(d.value).toEqual(null);
expect(d.updateValue()).toBeFalsy();
expect(d.value).toEqual(null);
});
});
it('should watch the contents of a StringMap value and emit new values if they change', () => {
const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap);
const myMap: {[key: string]: any} = {};
myMap['abc'] = true;
describe('updateValue', () => {
d.setValue(myMap);
expect(d.hasValueChanged()).toBeTruthy();
expect(d.value).toEqual({abc: true});
expect(d.hasValueChanged()).toBeFalsy();
it('should update the differ value if the contents of a input StringMap change', () => {
const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap);
myMap['def'] = true;
expect(d.hasValueChanged()).toBeTruthy();
expect(d.value).toEqual({abc: true, def: true});
expect(d.hasValueChanged()).toBeFalsy();
const myMap: {[key: string]: true} = {};
myMap['abc'] = true;
delete myMap['abc'];
delete myMap['def'];
expect(d.hasValueChanged()).toBeTruthy();
expect(d.value).toEqual({});
expect(d.hasValueChanged()).toBeFalsy();
});
d.setInput(myMap);
expect(d.updateValue()).toBeTruthy();
expect(d.value).toEqual({abc: true});
expect(d.updateValue()).toBeFalsy();
it('should watch the contents of an Array value and emit new values if they change', () => {
const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap);
myMap['def'] = true;
expect(d.updateValue()).toBeTruthy();
expect(d.value).toEqual({abc: true, def: true});
expect(d.updateValue()).toBeFalsy();
const myArray: string[] = [];
myArray.push('abc');
delete myMap['abc'];
delete myMap['def'];
expect(d.updateValue()).toBeTruthy();
expect(d.value).toEqual({});
expect(d.updateValue()).toBeFalsy();
});
d.setValue(myArray);
expect(d.hasValueChanged()).toBeTruthy();
expect(d.value).toEqual({abc: true});
expect(d.hasValueChanged()).toBeFalsy();
myArray.push('def');
expect(d.hasValueChanged()).toBeTruthy();
expect(d.value).toEqual({abc: true, def: true});
expect(d.hasValueChanged()).toBeFalsy();
it('should update the differ value if the contents of an input Array change', () => {
const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap);
myArray.length = 0;
expect(d.hasValueChanged()).toBeTruthy();
expect(d.value).toEqual({});
expect(d.hasValueChanged()).toBeFalsy();
});
const myArray: string[] = [];
myArray.push('abc');
it('should watch the contents of a Set value and emit new values if they change', () => {
const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap);
d.setInput(myArray);
expect(d.updateValue()).toBeTruthy();
expect(d.value).toEqual({abc: true});
expect(d.updateValue()).toBeFalsy();
const mySet = new Set<string>();
mySet.add('abc');
myArray.push('def');
expect(d.updateValue()).toBeTruthy();
expect(d.value).toEqual({abc: true, def: true});
expect(d.updateValue()).toBeFalsy();
d.setValue(mySet);
expect(d.hasValueChanged()).toBeTruthy();
expect(d.value).toEqual({abc: true});
expect(d.hasValueChanged()).toBeFalsy();
myArray.length = 0;
expect(d.updateValue()).toBeTruthy();
expect(d.value).toEqual({});
expect(d.updateValue()).toBeFalsy();
});
mySet.add('def');
expect(d.hasValueChanged()).toBeTruthy();
expect(d.value).toEqual({abc: true, def: true});
expect(d.hasValueChanged()).toBeFalsy();
mySet.clear();
expect(d.hasValueChanged()).toBeTruthy();
expect(d.value).toEqual({});
expect(d.hasValueChanged()).toBeFalsy();
it('should update the differ value if the contents of an input Set change', () => {
const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap);
const mySet = new Set<string>();
mySet.add('abc');
d.setInput(mySet);
expect(d.updateValue()).toBeTruthy();
expect(d.value).toEqual({abc: true});
expect(d.updateValue()).toBeFalsy();
mySet.add('def');
expect(d.updateValue()).toBeTruthy();
expect(d.value).toEqual({abc: true, def: true});
expect(d.updateValue()).toBeFalsy();
mySet.clear();
expect(d.updateValue()).toBeTruthy();
expect(d.value).toEqual({});
expect(d.updateValue()).toBeFalsy();
});
});
});