From 42231f571947fbd2db5ea5692d3990470c019990 Mon Sep 17 00:00:00 2001 From: vsavkin Date: Tue, 5 Jan 2016 13:42:21 -0800 Subject: [PATCH] feat(change_detection): allow all legal programs in the dev mode BEFORE: The following would throw in the dev mode because `f` would return a new array when called by checkNoChanges. @Component({ template: ` {{f()}} ` }) class A { f() { return [1]; } } AFTER: The checkNoChanges function compares only primitives types for equality, and deeply compares iterables. Other objects cannot cause checkNoChanges to throw. This means that the dev mode would never fail given a legal program, but may allow some illegal programs. --- .../change_detection_jit_generator.ts | 7 +-- .../change_detection/change_detection_util.ts | 24 +++++++- .../dynamic_change_detector.ts | 10 +++- modules/angular2/src/facade/collection.dart | 13 ++++ modules/angular2/src/facade/collection.ts | 13 ++++ modules/angular2/src/facade/lang.dart | 2 + modules/angular2/src/facade/lang.ts | 4 ++ .../change_detection/change_detector_spec.ts | 13 ++++ .../change_detector_util_spec.ts | 60 +++++++++++++++++++ .../change_detector_codegen.dart | 7 +-- 10 files changed, 141 insertions(+), 12 deletions(-) create mode 100644 modules/angular2/test/core/change_detection/change_detector_util_spec.ts diff --git a/modules/angular2/src/core/change_detection/change_detection_jit_generator.ts b/modules/angular2/src/core/change_detection/change_detection_jit_generator.ts index 98615d94ae..80d1573c34 100644 --- a/modules/angular2/src/core/change_detection/change_detection_jit_generator.ts +++ b/modules/angular2/src/core/change_detection/change_detection_jit_generator.ts @@ -350,6 +350,7 @@ export class ChangeDetectorJITGenerator { var condition = `!${pipe}.pure || (${contexOrArgCheck.join(" || ")})`; var check = ` + ${this._genThrowOnChangeCheck(oldValue, newValue)} if (${this.changeDetectionUtilVarName}.looseNotIdentical(${oldValue}, ${newValue})) { ${newValue} = ${this.changeDetectionUtilVarName}.unwrapValue(${newValue}) ${this._genChangeMarker(r)} @@ -377,6 +378,7 @@ export class ChangeDetectorJITGenerator { `; var check = ` + ${this._genThrowOnChangeCheck(oldValue, newValue)} if (${this.changeDetectionUtilVarName}.looseNotIdentical(${oldValue}, ${newValue})) { ${this._genChangeMarker(r)} ${this._genUpdateDirectiveOrElement(r)} @@ -409,7 +411,6 @@ export class ChangeDetectorJITGenerator { if (!r.lastInBinding) return ""; var newValue = this._names.getLocalName(r.selfIndex); - var oldValue = this._names.getFieldName(r.selfIndex); var notifyDebug = this.genConfig.logBindingUpdate ? `this.logBindingUpdate(${newValue});` : ""; var br = r.bindingRecord; @@ -417,14 +418,12 @@ export class ChangeDetectorJITGenerator { var directiveProperty = `${this._names.getDirectiveName(br.directiveRecord.directiveIndex)}.${br.target.name}`; return ` - ${this._genThrowOnChangeCheck(oldValue, newValue)} ${directiveProperty} = ${newValue}; ${notifyDebug} ${IS_CHANGED_LOCAL} = true; `; } else { return ` - ${this._genThrowOnChangeCheck(oldValue, newValue)} this.notifyDispatcher(${newValue}); ${notifyDebug} `; @@ -435,7 +434,7 @@ export class ChangeDetectorJITGenerator { _genThrowOnChangeCheck(oldValue: string, newValue: string): string { if (assertionsEnabled()) { return ` - if(throwOnChange) { + if (throwOnChange && !${this.changeDetectionUtilVarName}.devModeEqual(${oldValue}, ${newValue})) { this.throwOnChangeError(${oldValue}, ${newValue}); } `; diff --git a/modules/angular2/src/core/change_detection/change_detection_util.ts b/modules/angular2/src/core/change_detection/change_detection_util.ts index fd99a44fa8..67c4fc175c 100644 --- a/modules/angular2/src/core/change_detection/change_detection_util.ts +++ b/modules/angular2/src/core/change_detection/change_detection_util.ts @@ -4,10 +4,17 @@ import { isBlank, Type, StringWrapper, - looseIdentical + looseIdentical, + isPrimitive } from 'angular2/src/facade/lang'; import {BaseException} from 'angular2/src/facade/exceptions'; -import {ListWrapper, MapWrapper, StringMapWrapper} from 'angular2/src/facade/collection'; +import { + ListWrapper, + MapWrapper, + StringMapWrapper, + isListLikeIterable, + areIterablesEqual +} from 'angular2/src/facade/collection'; import {ProtoRecord} from './proto_record'; import {ChangeDetectionStrategy, isDefaultChangeDetectionStrategy} from './constants'; import {implementsOnDestroy} from './pipe_lifecycle_reflector'; @@ -214,4 +221,17 @@ export class ChangeDetectionUtil { } static looseNotIdentical(a: any, b: any): boolean { return !looseIdentical(a, b); } + + static devModeEqual(a: any, b: any): boolean { + if (isListLikeIterable(a) && isListLikeIterable(b)) { + return areIterablesEqual(a, b, ChangeDetectionUtil.devModeEqual); + + } else if (!isListLikeIterable(a) && !isPrimitive(a) && !isListLikeIterable(b) && + !isPrimitive(b)) { + return true; + + } else { + return looseIdentical(a, b); + } + } } diff --git a/modules/angular2/src/core/change_detection/dynamic_change_detector.ts b/modules/angular2/src/core/change_detection/dynamic_change_detector.ts index ec6c766f97..078c51f20c 100644 --- a/modules/angular2/src/core/change_detection/dynamic_change_detector.ts +++ b/modules/angular2/src/core/change_detection/dynamic_change_detector.ts @@ -304,7 +304,10 @@ export class DynamicChangeDetector extends AbstractChangeDetector { if (proto.shouldBeChecked()) { var prevValue = this._readSelf(proto, values); - if (ChangeDetectionUtil.looseNotIdentical(prevValue, currValue)) { + var detectedChange = throwOnChange ? + !ChangeDetectionUtil.devModeEqual(prevValue, currValue) : + ChangeDetectionUtil.looseNotIdentical(prevValue, currValue); + if (detectedChange) { if (proto.lastInBinding) { var change = ChangeDetectionUtil.simpleChange(prevValue, currValue); if (throwOnChange) this.throwOnChangeError(prevValue, currValue); @@ -405,7 +408,10 @@ export class DynamicChangeDetector extends AbstractChangeDetector { if (proto.shouldBeChecked()) { var prevValue = this._readSelf(proto, values); - if (ChangeDetectionUtil.looseNotIdentical(prevValue, currValue)) { + var detectedChange = throwOnChange ? + !ChangeDetectionUtil.devModeEqual(prevValue, currValue) : + ChangeDetectionUtil.looseNotIdentical(prevValue, currValue); + if (detectedChange) { currValue = ChangeDetectionUtil.unwrapValue(currValue); if (proto.lastInBinding) { diff --git a/modules/angular2/src/facade/collection.dart b/modules/angular2/src/facade/collection.dart index d01cb8c40d..ecc54a6d17 100644 --- a/modules/angular2/src/facade/collection.dart +++ b/modules/angular2/src/facade/collection.dart @@ -225,6 +225,19 @@ class ListWrapper { bool isListLikeIterable(obj) => obj is Iterable; +bool areIterablesEqual(Iterable a, Iterable b, Function comparator) { + var iterator1 = a.iterator; + var iterator2 = b.iterator; + + while (true) { + var done1 = !iterator1.moveNext(); + var done2 = !iterator2.moveNext(); + if (done1 && done2) return true; + if (done1 || done2) return false; + if (!comparator(iterator2.current, iterator2.current)) return false; + } +} + void iterateListLike(iter, fn(item)) { assert(iter is Iterable); for (var item in iter) { diff --git a/modules/angular2/src/facade/collection.ts b/modules/angular2/src/facade/collection.ts index aabbe66f0e..ebc97f2eb6 100644 --- a/modules/angular2/src/facade/collection.ts +++ b/modules/angular2/src/facade/collection.ts @@ -274,6 +274,19 @@ export function isListLikeIterable(obj: any): boolean { getSymbolIterator() in obj); // JS Iterable have a Symbol.iterator prop } +export function areIterablesEqual(a: any, b: any, comparator: Function): boolean { + var iterator1 = a[getSymbolIterator()](); + var iterator2 = b[getSymbolIterator()](); + + while (true) { + let item1 = iterator1.next(); + let item2 = iterator2.next(); + if (item1.done && item2.done) return true; + if (item1.done || item2.done) return false; + if (!comparator(item1.value, item2.value)) return false; + } +} + export function iterateListLike(obj: any, fn: Function) { if (isArray(obj)) { for (var i = 0; i < obj.length; i++) { diff --git a/modules/angular2/src/facade/lang.dart b/modules/angular2/src/facade/lang.dart index dfe50b7ed6..e09adbd2cd 100644 --- a/modules/angular2/src/facade/lang.dart +++ b/modules/angular2/src/facade/lang.dart @@ -345,6 +345,8 @@ class DateWrapper { } } +bool isPrimitive(Object obj) => obj is num || obj is bool || obj == null || obj is String; + // needed to match the exports from lang.js var global = null; diff --git a/modules/angular2/src/facade/lang.ts b/modules/angular2/src/facade/lang.ts index 7c83209ec0..d19007a553 100644 --- a/modules/angular2/src/facade/lang.ts +++ b/modules/angular2/src/facade/lang.ts @@ -427,3 +427,7 @@ export function evalExpression(sourceUrl: string, expr: string, declarations: st } return new Function(...fnArgNames.concat(fnBody))(...fnArgValues); } + +export function isPrimitive(obj: any): boolean { + return !isJsObject(obj); +} \ No newline at end of file diff --git a/modules/angular2/test/core/change_detection/change_detector_spec.ts b/modules/angular2/test/core/change_detection/change_detector_spec.ts index 9e4dffb57d..a2629ae8ba 100644 --- a/modules/angular2/test/core/change_detection/change_detector_spec.ts +++ b/modules/angular2/test/core/change_detection/change_detector_spec.ts @@ -887,6 +887,13 @@ export function main() { 'Expression [\'"]a in location[\'"] has changed after it was checked')); }); + it('should not throw when two arrays are structurally the same', () => { + var val = _createChangeDetector('a', new TestDataWithGetter(() => ['value'])); + val.changeDetector.detectChanges(); + + expect(() => { val.changeDetector.checkNoChanges(); }).not.toThrow(); + }); + it('should not break the next run', () => { var val = _createChangeDetector('a', new TestData('value')); expect(() => val.changeDetector.checkNoChanges()) @@ -1597,6 +1604,12 @@ class TestData { constructor(public a: any) {} } +class TestDataWithGetter { + constructor(private fn: Function) {} + + get a() { return this.fn(); } +} + class TestDispatcher implements ChangeDispatcher { log: string[]; debugLog: string[]; diff --git a/modules/angular2/test/core/change_detection/change_detector_util_spec.ts b/modules/angular2/test/core/change_detection/change_detector_util_spec.ts new file mode 100644 index 0000000000..88625bb5ea --- /dev/null +++ b/modules/angular2/test/core/change_detection/change_detector_util_spec.ts @@ -0,0 +1,60 @@ +import { + ddescribe, + describe, + it, + iit, + xit, + expect, + beforeEach, + afterEach +} from 'angular2/testing_internal'; + +import {ChangeDetectionUtil} from 'angular2/src/core/change_detection/change_detection_util'; + +export function main() { + describe("ChangeDetectionUtil", () => { + describe("devModeEqual", () => { + it("should do the deep comparison of iterables", () => { + expect(ChangeDetectionUtil.devModeEqual([['one']], [['one']])).toBe(true); + expect(ChangeDetectionUtil.devModeEqual(['one'], ['one', 'two'])).toBe(false); + expect(ChangeDetectionUtil.devModeEqual(['one', 'two'], ['one'])).toBe(false); + expect(ChangeDetectionUtil.devModeEqual(['one'], 'one')).toBe(false); + expect(ChangeDetectionUtil.devModeEqual(['one'], new Object())).toBe(false); + expect(ChangeDetectionUtil.devModeEqual('one', ['one'])).toBe(false); + expect(ChangeDetectionUtil.devModeEqual(new Object(), ['one'])).toBe(false); + }); + + it("should compare primitive numbers", () => { + expect(ChangeDetectionUtil.devModeEqual(1, 1)).toBe(true); + expect(ChangeDetectionUtil.devModeEqual(1, 2)).toBe(false); + expect(ChangeDetectionUtil.devModeEqual(new Object(), 2)).toBe(false); + expect(ChangeDetectionUtil.devModeEqual(1, new Object())).toBe(false); + }); + + it("should compare primitive strings", () => { + expect(ChangeDetectionUtil.devModeEqual('one', 'one')).toBe(true); + expect(ChangeDetectionUtil.devModeEqual('one', 'two')).toBe(false); + expect(ChangeDetectionUtil.devModeEqual(new Object(), 'one')).toBe(false); + expect(ChangeDetectionUtil.devModeEqual('one', new Object())).toBe(false); + }); + + it("should compare primitive booleans", () => { + expect(ChangeDetectionUtil.devModeEqual(true, true)).toBe(true); + expect(ChangeDetectionUtil.devModeEqual(true, false)).toBe(false); + expect(ChangeDetectionUtil.devModeEqual(new Object(), true)).toBe(false); + expect(ChangeDetectionUtil.devModeEqual(true, new Object())).toBe(false); + }); + + it("should compare null", () => { + expect(ChangeDetectionUtil.devModeEqual(null, null)).toBe(true); + expect(ChangeDetectionUtil.devModeEqual(null, 1)).toBe(false); + expect(ChangeDetectionUtil.devModeEqual(new Object(), null)).toBe(false); + expect(ChangeDetectionUtil.devModeEqual(null, new Object())).toBe(false); + }); + + it("should return true for other objects", () => { + expect(ChangeDetectionUtil.devModeEqual(new Object(), new Object())).toBe(true); + }); + }); + }); +} diff --git a/modules_dart/transform/lib/src/transform/template_compiler/change_detector_codegen.dart b/modules_dart/transform/lib/src/transform/template_compiler/change_detector_codegen.dart index a496ddc65e..faf9943d7c 100644 --- a/modules_dart/transform/lib/src/transform/template_compiler/change_detector_codegen.dart +++ b/modules_dart/transform/lib/src/transform/template_compiler/change_detector_codegen.dart @@ -433,6 +433,7 @@ class _CodegenState { var condition = '''!${pipe}.pure || (${contexOrArgCheck.join(" || ")})'''; var check = ''' + ${_genThrowOnChangeCheck(oldValue, newValue)} if (${_genPrefix}$_UTIL.looseNotIdentical($oldValue, $newValue)) { $newValue = ${_genPrefix}$_UTIL.unwrapValue($newValue); ${_genChangeMarker(r)} @@ -459,6 +460,7 @@ class _CodegenState { '''; var check = ''' + ${_genThrowOnChangeCheck(oldValue, newValue)} if (${_genPrefix}$_UTIL.looseNotIdentical($newValue, $oldValue)) { ${_genChangeMarker(r)} ${_genUpdateDirectiveOrElement(r)} @@ -492,7 +494,6 @@ class _CodegenState { if (!r.lastInBinding) return ''; var newValue = _names.getLocalName(r.selfIndex); - var oldValue = _names.getFieldName(r.selfIndex); var notifyDebug = _genConfig.logBindingUpdate ? "this.logBindingUpdate(${newValue});" : ""; @@ -502,14 +503,12 @@ class _CodegenState { var directiveProperty = '${_names.getDirectiveName(br.directiveRecord.directiveIndex)}.${br.target.name}'; return ''' - ${_genThrowOnChangeCheck(oldValue, newValue)} $directiveProperty = $newValue; ${notifyDebug} $_IS_CHANGED_LOCAL = true; '''; } else { return ''' - ${_genThrowOnChangeCheck(oldValue, newValue)} this.notifyDispatcher(${newValue}); ${notifyDebug} '''; @@ -518,7 +517,7 @@ class _CodegenState { String _genThrowOnChangeCheck(String oldValue, String newValue) { return ''' - if(${_genPrefix}assertionsEnabled() && throwOnChange) { + if(${_genPrefix}assertionsEnabled() && throwOnChange && !${_genPrefix}${_UTIL}.devModeEqual(${oldValue}, ${newValue})) { this.throwOnChangeError(${oldValue}, ${newValue}); } ''';