From 0914dc35e8a3b7b820e235fe26eef89ac8855f29 Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Fri, 15 Jul 2016 16:26:19 -0700 Subject: [PATCH] refactor(Differ): cleanup --- .../core/src/animation/view_animation_map.ts | 7 +- .../differs/default_keyvalue_differ.ts | 131 ++++-------- .../differs/default_keyvalue_differ_spec.ts | 194 ++++++++++-------- modules/@angular/facade/src/collection.ts | 26 +-- modules/@angular/forms/src/model.ts | 4 +- 5 files changed, 153 insertions(+), 209 deletions(-) diff --git a/modules/@angular/core/src/animation/view_animation_map.ts b/modules/@angular/core/src/animation/view_animation_map.ts index 2065143dcd..fd3c6e3af9 100644 --- a/modules/@angular/core/src/animation/view_animation_map.ts +++ b/modules/@angular/core/src/animation/view_animation_map.ts @@ -25,10 +25,9 @@ export class ViewAnimationMap { } findAllPlayersByElement(element: any): AnimationPlayer[] { - var players: AnimationPlayer[] = []; - StringMapWrapper.forEach( - this._map.get(element), (player: AnimationPlayer) => players.push(player)); - return players; + const el = this._map.get(element); + + return el ? StringMapWrapper.values(el) : []; } set(element: any, animationName: string, player: AnimationPlayer): void { diff --git a/modules/@angular/core/src/change_detection/differs/default_keyvalue_differ.ts b/modules/@angular/core/src/change_detection/differs/default_keyvalue_differ.ts index 765deddac6..fa5d90540c 100644 --- a/modules/@angular/core/src/change_detection/differs/default_keyvalue_differ.ts +++ b/modules/@angular/core/src/change_detection/differs/default_keyvalue_differ.ts @@ -6,15 +6,14 @@ * found in the LICENSE file at https://angular.io/license */ -import {MapWrapper, StringMapWrapper} from '../../facade/collection'; +import {StringMapWrapper} from '../../facade/collection'; import {BaseException} from '../../facade/exceptions'; -import {isBlank, isJsObject, looseIdentical, stringify} from '../../facade/lang'; +import {isJsObject, looseIdentical, stringify} from '../../facade/lang'; import {ChangeDetectorRef} from '../change_detector_ref'; import {KeyValueDiffer, KeyValueDifferFactory} from './keyvalue_differs'; -/* @ts2dart_const */ export class DefaultKeyValueDifferFactory implements KeyValueDifferFactory { constructor() {} supports(obj: any): boolean { return obj instanceof Map || isJsObject(obj); } @@ -38,67 +37,64 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer { this._removalsHead !== null; } - forEachItem(fn: Function) { + forEachItem(fn: (r: KeyValueChangeRecord) => void) { var record: KeyValueChangeRecord; for (record = this._mapHead; record !== null; record = record._next) { fn(record); } } - forEachPreviousItem(fn: Function) { + forEachPreviousItem(fn: (r: KeyValueChangeRecord) => void) { var record: KeyValueChangeRecord; for (record = this._previousMapHead; record !== null; record = record._nextPrevious) { fn(record); } } - forEachChangedItem(fn: Function) { + forEachChangedItem(fn: (r: KeyValueChangeRecord) => void) { var record: KeyValueChangeRecord; for (record = this._changesHead; record !== null; record = record._nextChanged) { fn(record); } } - forEachAddedItem(fn: Function) { + forEachAddedItem(fn: (r: KeyValueChangeRecord) => void) { var record: KeyValueChangeRecord; for (record = this._additionsHead; record !== null; record = record._nextAdded) { fn(record); } } - forEachRemovedItem(fn: Function) { + forEachRemovedItem(fn: (r: KeyValueChangeRecord) => void) { var record: KeyValueChangeRecord; for (record = this._removalsHead; record !== null; record = record._nextRemoved) { fn(record); } } - diff(map: Map): any { - if (isBlank(map)) map = MapWrapper.createFromPairs([]); - if (!(map instanceof Map || isJsObject(map))) { + diff(map: Map|{[k: string]: any}): any { + if (!map) { + map = new Map(); + } else if (!(map instanceof Map || isJsObject(map))) { throw new BaseException(`Error trying to diff '${map}'`); } - if (this.check(map)) { - return this; - } else { - return null; - } + return this.check(map) ? this : null } onDestroy() {} - check(map: Map): boolean { + check(map: Map|{[k: string]: any}): boolean { this._reset(); - var records = this._records; - var oldSeqRecord: KeyValueChangeRecord = this._mapHead; - var lastOldSeqRecord: KeyValueChangeRecord = null; - var lastNewSeqRecord: KeyValueChangeRecord = null; - var seqChanged: boolean = false; + let records = this._records; + let oldSeqRecord: KeyValueChangeRecord = this._mapHead; + let lastOldSeqRecord: KeyValueChangeRecord = null; + let lastNewSeqRecord: KeyValueChangeRecord = null; + let seqChanged: boolean = false; - this._forEach(map, (value: any /** TODO #9100 */, key: any /** TODO #9100 */) => { - var newSeqRecord: any /** TODO #9100 */; - if (oldSeqRecord !== null && key === oldSeqRecord.key) { + this._forEach(map, (value: any, key: any) => { + let newSeqRecord: any; + if (oldSeqRecord && key === oldSeqRecord.key) { newSeqRecord = oldSeqRecord; if (!looseIdentical(value, oldSeqRecord.currentValue)) { oldSeqRecord.previousValue = oldSeqRecord.currentValue; @@ -108,7 +104,6 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer { } else { seqChanged = true; if (oldSeqRecord !== null) { - oldSeqRecord._next = null; this._removeFromSeq(lastOldSeqRecord, oldSeqRecord); this._addToRemovals(oldSeqRecord); } @@ -134,7 +129,7 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer { } lastOldSeqRecord = oldSeqRecord; lastNewSeqRecord = newSeqRecord; - oldSeqRecord = oldSeqRecord === null ? null : oldSeqRecord._next; + oldSeqRecord = oldSeqRecord && oldSeqRecord._next; }); this._truncate(lastOldSeqRecord, oldSeqRecord); return this.isDirty; @@ -143,7 +138,7 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer { /** @internal */ _reset() { if (this.isDirty) { - var record: KeyValueChangeRecord; + let record: KeyValueChangeRecord; // Record the state of the mapping for (record = this._previousMapHead = this._mapHead; record !== null; record = record._next) { record._nextPrevious = record._next; @@ -157,31 +152,6 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer { record.previousValue = record.currentValue; } - // todo(vicb) once assert is supported - // assert(() { - // var r = _changesHead; - // while (r != null) { - // var nextRecord = r._nextChanged; - // r._nextChanged = null; - // r = nextRecord; - // } - // - // r = _additionsHead; - // while (r != null) { - // var nextRecord = r._nextAdded; - // r._nextAdded = null; - // r = nextRecord; - // } - // - // r = _removalsHead; - // while (r != null) { - // var nextRecord = r._nextRemoved; - // r._nextRemoved = null; - // r = nextRecord; - // } - // - // return true; - //}); this._changesHead = this._changesTail = null; this._additionsHead = this._additionsTail = null; this._removalsHead = this._removalsTail = null; @@ -197,17 +167,12 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer { lastRecord._next = null; } var nextRecord = record._next; - // todo(vicb) assert - // assert((() { - // record._next = null; - // return true; - //})); this._addToRemovals(record); lastRecord = record; record = nextRecord; } - for (var rec: KeyValueChangeRecord = this._removalsHead; rec !== null; rec = rec._nextRemoved) { + for (let rec: KeyValueChangeRecord = this._removalsHead; rec !== null; rec = rec._nextRemoved) { rec.previousValue = rec.currentValue; rec.currentValue = null; this._records.delete(rec.key); @@ -222,12 +187,6 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer { /** @internal */ _addToRemovals(record: KeyValueChangeRecord) { - // todo(vicb) assert - // assert(record._next == null); - // assert(record._nextAdded == null); - // assert(record._nextChanged == null); - // assert(record._nextRemoved == null); - // assert(record._prevRemoved == null); if (this._removalsHead === null) { this._removalsHead = this._removalsTail = record; } else { @@ -245,22 +204,13 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer { } else { prev._next = next; } - // todo(vicb) assert - // assert((() { - // record._next = null; - // return true; - //})()); + record._next = null; } /** @internal */ _removeFromRemovals(record: KeyValueChangeRecord) { - // todo(vicb) assert - // assert(record._next == null); - // assert(record._nextAdded == null); - // assert(record._nextChanged == null); - - var prev = record._prevRemoved; - var next = record._nextRemoved; + const prev = record._prevRemoved; + const next = record._nextRemoved; if (prev === null) { this._removalsHead = next; } else { @@ -276,12 +226,6 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer { /** @internal */ _addToAdditions(record: KeyValueChangeRecord) { - // todo(vicb): assert - // assert(record._next == null); - // assert(record._nextAdded == null); - // assert(record._nextChanged == null); - // assert(record._nextRemoved == null); - // assert(record._prevRemoved == null); if (this._additionsHead === null) { this._additionsHead = this._additionsTail = record; } else { @@ -292,11 +236,6 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer { /** @internal */ _addToChanges(record: KeyValueChangeRecord) { - // todo(vicb) assert - // assert(record._nextAdded == null); - // assert(record._nextChanged == null); - // assert(record._nextRemoved == null); - // assert(record._prevRemoved == null); if (this._changesHead === null) { this._changesHead = this._changesTail = record; } else { @@ -306,12 +245,12 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer { } toString(): string { - var items: any[] /** TODO #9100 */ = []; - var previous: any[] /** TODO #9100 */ = []; - var changes: any[] /** TODO #9100 */ = []; - var additions: any[] /** TODO #9100 */ = []; - var removals: any[] /** TODO #9100 */ = []; - var record: KeyValueChangeRecord; + const items: any[] = []; + const previous: any[] = []; + const changes: any[] = []; + const additions: any[] = []; + const removals: any[] = []; + let record: KeyValueChangeRecord; for (record = this._mapHead; record !== null; record = record._next) { items.push(stringify(record)); @@ -337,9 +276,9 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer { } /** @internal */ - _forEach(obj: any /** TODO #9100 */, fn: Function) { + private _forEach(obj: Map|{[k: string]: V}, fn: (v: V, k: any) => void) { if (obj instanceof Map) { - (>obj).forEach(fn); + obj.forEach(fn); } else { StringMapWrapper.forEach(obj, fn); } diff --git a/modules/@angular/core/test/change_detection/differs/default_keyvalue_differ_spec.ts b/modules/@angular/core/test/change_detection/differs/default_keyvalue_differ_spec.ts index a74d7f72ef..06e78c31af 100644 --- a/modules/@angular/core/test/change_detection/differs/default_keyvalue_differ_spec.ts +++ b/modules/@angular/core/test/change_detection/differs/default_keyvalue_differ_spec.ts @@ -8,16 +8,13 @@ import {DefaultKeyValueDiffer, DefaultKeyValueDifferFactory} from '@angular/core/src/change_detection/differs/default_keyvalue_differ'; import {afterEach, beforeEach, ddescribe, describe, expect, iit, it, xit} from '@angular/core/testing/testing_internal'; - -import {NumberWrapper, isJsObject} from '../../../src/facade/lang'; import {kvChangesAsString} from '../../change_detection/util'; - // todo(vicb): Update the code & tests for object equality export function main() { describe('keyvalue differ', function() { describe('DefaultKeyValueDiffer', function() { - var differ: any /** TODO #9100 */; + var differ: DefaultKeyValueDiffer; var m: Map; beforeEach(() => { @@ -113,108 +110,123 @@ export function main() { })); }); - it('should test string by value rather than by reference (DART)', () => { - m.set('foo', 'bar'); - differ.check(m); - - var f = 'f'; - var oo = 'oo'; - var b = 'b'; - var ar = 'ar'; - - m.set(f + oo, b + ar); - differ.check(m); - - expect(differ.toString()).toEqual(kvChangesAsString({map: ['foo'], previous: ['foo']})); - }); - - it('should not see a NaN value as a change (JS)', () => { - m.set('foo', NumberWrapper.NaN); + it('should not see a NaN value as a change', () => { + m.set('foo', Number.NaN); differ.check(m); differ.check(m); expect(differ.toString()).toEqual(kvChangesAsString({map: ['foo'], previous: ['foo']})); }); - // JS specific tests (JS Objects) - if (isJsObject({})) { - describe('JsObject changes', () => { - it('should support JS Object', () => { - var f = new DefaultKeyValueDifferFactory(); - expect(f.supports({})).toBeTruthy(); - expect(f.supports('not supported')).toBeFalsy(); - expect(f.supports(0)).toBeFalsy(); - expect(f.supports(null)).toBeFalsy(); - }); + it('should work regardless key order', () => { + m.set('a', 0); + m.set('b', 0); + differ.check(m); - it('should do basic object watching', () => { - let m = {}; - differ.check(m); + m = new Map(); + m.set('b', 1); + m.set('a', 1); + differ.check(m); - (m as any /** TODO #9100 */)['a'] = 'A'; - differ.check(m); - expect(differ.toString()) - .toEqual(kvChangesAsString({map: ['a[null->A]'], additions: ['a[null->A]']})); + expect(differ.toString()).toEqual(kvChangesAsString({ + map: ['b[0->1]', 'a[0->1]'], + previous: ['a[0->1]', 'b[0->1]'], + changes: ['b[0->1]', 'a[0->1]'] + })); + }); - (m as any /** TODO #9100 */)['b'] = 'B'; - differ.check(m); - expect(differ.toString()) - .toEqual(kvChangesAsString( - {map: ['a', 'b[null->B]'], previous: ['a'], additions: ['b[null->B]']})); - - (m as any /** TODO #9100 */)['b'] = 'BB'; - (m as any /** TODO #9100 */)['d'] = 'D'; - differ.check(m); - expect(differ.toString()).toEqual(kvChangesAsString({ - map: ['a', 'b[B->BB]', 'd[null->D]'], - previous: ['a', 'b[B->BB]'], - additions: ['d[null->D]'], - changes: ['b[B->BB]'] - })); - - m = {}; - (m as any /** TODO #9100 */)['a'] = 'A'; - (m as any /** TODO #9100 */)['d'] = 'D'; - differ.check(m); - expect(differ.toString()).toEqual(kvChangesAsString({ - map: ['a', 'd'], - previous: ['a', 'b[BB->null]', 'd'], - removals: ['b[BB->null]'] - })); - - m = {}; - differ.check(m); - expect(differ.toString()).toEqual(kvChangesAsString({ - previous: ['a[A->null]', 'd[D->null]'], - removals: ['a[A->null]', 'd[D->null]'] - })); - }); + describe('JsObject changes', () => { + it('should support JS Object', () => { + var f = new DefaultKeyValueDifferFactory(); + expect(f.supports({})).toBeTruthy(); + expect(f.supports('not supported')).toBeFalsy(); + expect(f.supports(0)).toBeFalsy(); + expect(f.supports(null)).toBeFalsy(); }); - describe('diff', () => { - it('should return self when there is a change', () => { - m.set('a', 'A'); - expect(differ.diff(m)).toBe(differ); - }); + it('should do basic object watching', () => { + let m: {[k: string]: string} = {}; + differ.check(m); - it('should return null when there is no change', () => { - m.set('a', 'A'); - differ.diff(m); - expect(differ.diff(m)).toEqual(null); - }); + m['a'] = 'A'; + differ.check(m); + expect(differ.toString()) + .toEqual(kvChangesAsString({map: ['a[null->A]'], additions: ['a[null->A]']})); - it('should treat null as an empty list', () => { - m.set('a', 'A'); - differ.diff(m); - expect(differ.diff(null).toString()) - .toEqual(kvChangesAsString({previous: ['a[A->null]'], removals: ['a[A->null]']})); - }); + m['b'] = 'B'; + differ.check(m); + expect(differ.toString()) + .toEqual(kvChangesAsString( + {map: ['a', 'b[null->B]'], previous: ['a'], additions: ['b[null->B]']})); + + m['b'] = 'BB'; + m['d'] = 'D'; + differ.check(m); + expect(differ.toString()).toEqual(kvChangesAsString({ + map: ['a', 'b[B->BB]', 'd[null->D]'], + previous: ['a', 'b[B->BB]'], + additions: ['d[null->D]'], + changes: ['b[B->BB]'] + })); + + m = {}; + m['a'] = 'A'; + m['d'] = 'D'; + differ.check(m); + expect(differ.toString()).toEqual(kvChangesAsString({ + map: ['a', 'd'], + previous: ['a', 'b[BB->null]', 'd'], + removals: ['b[BB->null]'] + })); + + m = {}; + differ.check(m); + expect(differ.toString()).toEqual(kvChangesAsString({ + previous: ['a[A->null]', 'd[D->null]'], + removals: ['a[A->null]', 'd[D->null]'] + })); - it('should throw when given an invalid collection', () => { - expect(() => differ.diff('invalid')).toThrowError('Error trying to diff \'invalid\''); - }); }); - } + + it('should work regardless key order', () => { + let m: {[k: string]: number} = {a: 0, b: 0}; + differ.check(m); + + m = {b: 1, a: 1}; + differ.check(m); + + expect(differ.toString()).toEqual(kvChangesAsString({ + map: ['b[0->1]', 'a[0->1]'], + previous: ['a[0->1]', 'b[0->1]'], + changes: ['b[0->1]', 'a[0->1]'] + })); + }); + }); + + describe('diff', () => { + it('should return self when there is a change', () => { + m.set('a', 'A'); + expect(differ.diff(m)).toBe(differ); + }); + + it('should return null when there is no change', () => { + m.set('a', 'A'); + differ.diff(m); + expect(differ.diff(m)).toEqual(null); + }); + + it('should treat null as an empty list', () => { + m.set('a', 'A'); + differ.diff(m); + expect(differ.diff(null).toString()) + .toEqual(kvChangesAsString({previous: ['a[A->null]'], removals: ['a[A->null]']})); + }); + + it('should throw when given an invalid collection', () => { + expect(() => differ.diff('invalid')) + .toThrowError('Error trying to diff \'invalid\''); + }); + }); }); }); } diff --git a/modules/@angular/facade/src/collection.ts b/modules/@angular/facade/src/collection.ts index 58a6d05cf1..29176a78c6 100644 --- a/modules/@angular/facade/src/collection.ts +++ b/modules/@angular/facade/src/collection.ts @@ -116,12 +116,10 @@ export class StringMapWrapper { return map.hasOwnProperty(key) ? map[key] : undefined; } static set(map: {[key: string]: V}, key: string, value: V) { map[key] = value; } + static keys(map: {[key: string]: any}): string[] { return Object.keys(map); } static values(map: {[key: string]: T}): T[] { - return Object.keys(map).reduce((r, a) => { - r.push(map[a]); - return r; - }, []); + return Object.keys(map).map((k: string): T => map[k]); } static isEmpty(map: {[key: string]: any}): boolean { for (var prop in map) { @@ -130,27 +128,21 @@ export class StringMapWrapper { return true; } static delete (map: {[key: string]: any}, key: string) { delete map[key]; } - static forEach(map: {[key: string]: V}, callback: /*(V, K) => void*/ Function) { - for (var prop in map) { - if (map.hasOwnProperty(prop)) { - callback(map[prop], prop); - } + static forEach(map: {[key: string]: V}, callback: (v: V, K: string) => void) { + for (let k of Object.keys(map)) { + callback(map[k], k); } } static merge(m1: {[key: string]: V}, m2: {[key: string]: V}): {[key: string]: V} { var m: {[key: string]: V} = {}; - for (var attr in m1) { - if (m1.hasOwnProperty(attr)) { - m[attr] = m1[attr]; - } + for (let k of Object.keys(m1)) { + m[k] = m1[k]; } - for (var attr in m2) { - if (m2.hasOwnProperty(attr)) { - m[attr] = m2[attr]; - } + for (let k of Object.keys(m2)) { + m[k] = m2[k]; } return m; diff --git a/modules/@angular/forms/src/model.ts b/modules/@angular/forms/src/model.ts index cbbe12c222..1edd2c5295 100644 --- a/modules/@angular/forms/src/model.ts +++ b/modules/@angular/forms/src/model.ts @@ -539,7 +539,9 @@ export class FormGroup extends AbstractControl { } /** @internal */ - _forEachChild(cb: Function): void { StringMapWrapper.forEach(this.controls, cb); } + _forEachChild(cb: (v: any, k: string) => void): void { + StringMapWrapper.forEach(this.controls, cb); + } /** @internal */ _setParentForControls() {