From b2a0be87e8934e48672d3f97c717fff600b0651b Mon Sep 17 00:00:00 2001 From: Tim Blasi Date: Mon, 20 Jul 2015 13:37:50 -0700 Subject: [PATCH] fix(change_detect): Sort `DirectiveMetadata` properties during processing The Angular 2 render compiler can get out of sync between its transformer execution and its runtime execution, leading to incorrect change detectors with out-of-order property values. Stable sorting solves this problem (temporarily). --- modules/angular2/src/facade/collection.dart | 1 + modules/angular2/src/facade/collection.ts | 1 + modules/angular2/src/facade/lang.dart | 2 ++ modules/angular2/src/facade/lang.ts | 10 ++++++++++ .../src/render/dom/compiler/directive_parser.ts | 16 +++++++++++++--- modules/angular2/test/forms/integration_spec.ts | 12 ++++++------ 6 files changed, 33 insertions(+), 9 deletions(-) diff --git a/modules/angular2/src/facade/collection.dart b/modules/angular2/src/facade/collection.dart index f2c27d4550..43a617b5da 100644 --- a/modules/angular2/src/facade/collection.dart +++ b/modules/angular2/src/facade/collection.dart @@ -42,6 +42,7 @@ class MapWrapper { static forEach(Map m, fn(v, k)) { m.forEach((k, v) => fn(v, k)); } + static get(Map map, key) => map[key]; static int size(Map m) => m.length; static void delete(Map m, k) { m.remove(k); diff --git a/modules/angular2/src/facade/collection.ts b/modules/angular2/src/facade/collection.ts index 82169bc727..d9c5128d00 100644 --- a/modules/angular2/src/facade/collection.ts +++ b/modules/angular2/src/facade/collection.ts @@ -64,6 +64,7 @@ export class MapWrapper { } static createFromPairs(pairs: List): Map { return createMapFromPairs(pairs); } static forEach(m: Map, fn: /*(V, K) => void*/ Function) { m.forEach(fn); } + static get(map: Map, key: K): V { return map.get(key); } static size(m: Map): number { return m.size; } static delete(m: Map, k: K) { m.delete(k); } static clearValues(m: Map) { _clearValues(m); } diff --git a/modules/angular2/src/facade/lang.dart b/modules/angular2/src/facade/lang.dart index 95d0496eb0..9c2ff18820 100644 --- a/modules/angular2/src/facade/lang.dart +++ b/modules/angular2/src/facade/lang.dart @@ -97,6 +97,8 @@ class StringWrapper { static bool contains(String s, String substr) { return s.contains(substr); } + + static int compare(String a, String b) => a.compareTo(b); } class StringJoiner { diff --git a/modules/angular2/src/facade/lang.ts b/modules/angular2/src/facade/lang.ts index d99cd9a206..01b547ca8f 100644 --- a/modules/angular2/src/facade/lang.ts +++ b/modules/angular2/src/facade/lang.ts @@ -158,6 +158,16 @@ export class StringWrapper { } static contains(s: string, substr: string): boolean { return s.indexOf(substr) != -1; } + + static compare(a: string, b: string): int { + if (a < b) { + return -1; + } else if (a > b) { + return 1; + } else { + return 0; + } + } } export class StringJoiner { diff --git a/modules/angular2/src/render/dom/compiler/directive_parser.ts b/modules/angular2/src/render/dom/compiler/directive_parser.ts index 25bea4c6e7..3151cc11c5 100644 --- a/modules/angular2/src/render/dom/compiler/directive_parser.ts +++ b/modules/angular2/src/render/dom/compiler/directive_parser.ts @@ -76,17 +76,17 @@ export class DirectiveParser implements CompileStep { }); } if (isPresent(dirMetadata.hostListeners)) { - MapWrapper.forEach(dirMetadata.hostListeners, (action, eventName) => { + this._sortedKeysForEach(dirMetadata.hostListeners, (action, eventName) => { this._bindDirectiveEvent(eventName, action, current, directiveBinderBuilder); }); } if (isPresent(dirMetadata.hostProperties)) { - MapWrapper.forEach(dirMetadata.hostProperties, (expression, hostPropertyName) => { + this._sortedKeysForEach(dirMetadata.hostProperties, (expression, hostPropertyName) => { this._bindHostProperty(hostPropertyName, expression, current, directiveBinderBuilder); }); } if (isPresent(dirMetadata.hostAttributes)) { - MapWrapper.forEach(dirMetadata.hostAttributes, (hostAttrValue, hostAttrName) => { + this._sortedKeysForEach(dirMetadata.hostAttributes, (hostAttrValue, hostAttrName) => { this._addHostAttribute(hostAttrName, hostAttrValue, current); }); } @@ -97,6 +97,16 @@ export class DirectiveParser implements CompileStep { }); } + _sortedKeysForEach(map: Map, fn: (value: string, key: string) => void): void { + var keys = MapWrapper.keys(map); + ListWrapper.sort(keys, (a, b) => { + // Ensure a stable sort. + var compareVal = StringWrapper.compare(a, b); + return compareVal == 0 ? -1 : compareVal; + }); + ListWrapper.forEach(keys, (key) => { fn(MapWrapper.get(map, key), key); }); + } + _ensureHasOnlyOneComponent(elementBinder: ElementBinderBuilder, elDescription: string): void { if (isPresent(elementBinder.componentId)) { throw new BaseException( diff --git a/modules/angular2/test/forms/integration_spec.ts b/modules/angular2/test/forms/integration_spec.ts index 06d5a823a8..ddf1b5f9cf 100644 --- a/modules/angular2/test/forms/integration_spec.ts +++ b/modules/angular2/test/forms/integration_spec.ts @@ -645,13 +645,13 @@ export function main() { var input = rootTC.query(By.css("input")).nativeElement; expect(DOM.classList(input)) - .toEqual(['ng-binding', 'ng-untouched', 'ng-pristine', 'ng-invalid']); + .toEqual(['ng-binding', 'ng-invalid', 'ng-pristine', 'ng-untouched']); dispatchEvent(input, "blur"); rootTC.detectChanges(); expect(DOM.classList(input)) - .toEqual(["ng-binding", "ng-pristine", "ng-invalid", "ng-touched"]); + .toEqual(["ng-binding", "ng-invalid", "ng-pristine", "ng-touched"]); input.value = "updatedValue"; dispatchEvent(input, "change"); @@ -675,13 +675,13 @@ export function main() { var input = rootTC.query(By.css("input")).nativeElement; expect(DOM.classList(input)) - .toEqual(["ng-binding", "ng-untouched", "ng-pristine", "ng-invalid"]); + .toEqual(["ng-binding", "ng-invalid", "ng-pristine", "ng-untouched"]); dispatchEvent(input, "blur"); rootTC.detectChanges(); expect(DOM.classList(input)) - .toEqual(["ng-binding", "ng-pristine", "ng-invalid", "ng-touched"]); + .toEqual(["ng-binding", "ng-invalid", "ng-pristine", "ng-touched"]); input.value = "updatedValue"; dispatchEvent(input, "change"); @@ -703,13 +703,13 @@ export function main() { var input = rootTC.query(By.css("input")).nativeElement; expect(DOM.classList(input)) - .toEqual(["ng-binding", "ng-untouched", "ng-pristine", "ng-invalid"]); + .toEqual(["ng-binding", "ng-invalid", "ng-pristine", "ng-untouched"]); dispatchEvent(input, "blur"); rootTC.detectChanges(); expect(DOM.classList(input)) - .toEqual(["ng-binding", "ng-pristine", "ng-invalid", "ng-touched"]); + .toEqual(["ng-binding", "ng-invalid", "ng-pristine", "ng-touched"]); input.value = "updatedValue"; dispatchEvent(input, "change");