Skip to content

Commit 42231f5

Browse files
vsavkinrkirov
vsavkin
authored and
rkirov
committed
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.
1 parent db87bae commit 42231f5

10 files changed

Lines changed: 141 additions & 12 deletions

File tree

‎modules/angular2/src/core/change_detection/change_detection_jit_generator.ts‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,7 @@ export class ChangeDetectorJITGenerator {
350350
var condition = `!${pipe}.pure || (${contexOrArgCheck.join(" || ")})`;
351351

352352
var check = `
353+
${this._genThrowOnChangeCheck(oldValue, newValue)}
353354
if (${this.changeDetectionUtilVarName}.looseNotIdentical(${oldValue}, ${newValue})) {
354355
${newValue} = ${this.changeDetectionUtilVarName}.unwrapValue(${newValue})
355356
${this._genChangeMarker(r)}
@@ -377,6 +378,7 @@ export class ChangeDetectorJITGenerator {
377378
`;
378379

379380
var check = `
381+
${this._genThrowOnChangeCheck(oldValue, newValue)}
380382
if (${this.changeDetectionUtilVarName}.looseNotIdentical(${oldValue}, ${newValue})) {
381383
${this._genChangeMarker(r)}
382384
${this._genUpdateDirectiveOrElement(r)}
@@ -409,22 +411,19 @@ export class ChangeDetectorJITGenerator {
409411
if (!r.lastInBinding) return "";
410412

411413
var newValue = this._names.getLocalName(r.selfIndex);
412-
var oldValue = this._names.getFieldName(r.selfIndex);
413414
var notifyDebug = this.genConfig.logBindingUpdate ? `this.logBindingUpdate(${newValue});` : "";
414415

415416
var br = r.bindingRecord;
416417
if (br.target.isDirective()) {
417418
var directiveProperty =
418419
`${this._names.getDirectiveName(br.directiveRecord.directiveIndex)}.${br.target.name}`;
419420
return `
420-
${this._genThrowOnChangeCheck(oldValue, newValue)}
421421
${directiveProperty} = ${newValue};
422422
${notifyDebug}
423423
${IS_CHANGED_LOCAL} = true;
424424
`;
425425
} else {
426426
return `
427-
${this._genThrowOnChangeCheck(oldValue, newValue)}
428427
this.notifyDispatcher(${newValue});
429428
${notifyDebug}
430429
`;
@@ -435,7 +434,7 @@ export class ChangeDetectorJITGenerator {
435434
_genThrowOnChangeCheck(oldValue: string, newValue: string): string {
436435
if (assertionsEnabled()) {
437436
return `
438-
if(throwOnChange) {
437+
if (throwOnChange && !${this.changeDetectionUtilVarName}.devModeEqual(${oldValue}, ${newValue})) {
439438
this.throwOnChangeError(${oldValue}, ${newValue});
440439
}
441440
`;

‎modules/angular2/src/core/change_detection/change_detection_util.ts‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,17 @@ import {
44
isBlank,
55
Type,
66
StringWrapper,
7-
looseIdentical
7+
looseIdentical,
8+
isPrimitive
89
} from 'angular2/src/facade/lang';
910
import {BaseException} from 'angular2/src/facade/exceptions';
10-
import {ListWrapper, MapWrapper, StringMapWrapper} from 'angular2/src/facade/collection';
11+
import {
12+
ListWrapper,
13+
MapWrapper,
14+
StringMapWrapper,
15+
isListLikeIterable,
16+
areIterablesEqual
17+
} from 'angular2/src/facade/collection';
1118
import {ProtoRecord} from './proto_record';
1219
import {ChangeDetectionStrategy, isDefaultChangeDetectionStrategy} from './constants';
1320
import {implementsOnDestroy} from './pipe_lifecycle_reflector';
@@ -214,4 +221,17 @@ export class ChangeDetectionUtil {
214221
}
215222

216223
static looseNotIdentical(a: any, b: any): boolean { return !looseIdentical(a, b); }
224+
225+
static devModeEqual(a: any, b: any): boolean {
226+
if (isListLikeIterable(a) && isListLikeIterable(b)) {
227+
return areIterablesEqual(a, b, ChangeDetectionUtil.devModeEqual);
228+
229+
} else if (!isListLikeIterable(a) && !isPrimitive(a) && !isListLikeIterable(b) &&
230+
!isPrimitive(b)) {
231+
return true;
232+
233+
} else {
234+
return looseIdentical(a, b);
235+
}
236+
}
217237
}

‎modules/angular2/src/core/change_detection/dynamic_change_detector.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,10 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
304304

305305
if (proto.shouldBeChecked()) {
306306
var prevValue = this._readSelf(proto, values);
307-
if (ChangeDetectionUtil.looseNotIdentical(prevValue, currValue)) {
307+
var detectedChange = throwOnChange ?
308+
!ChangeDetectionUtil.devModeEqual(prevValue, currValue) :
309+
ChangeDetectionUtil.looseNotIdentical(prevValue, currValue);
310+
if (detectedChange) {
308311
if (proto.lastInBinding) {
309312
var change = ChangeDetectionUtil.simpleChange(prevValue, currValue);
310313
if (throwOnChange) this.throwOnChangeError(prevValue, currValue);
@@ -405,7 +408,10 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
405408

406409
if (proto.shouldBeChecked()) {
407410
var prevValue = this._readSelf(proto, values);
408-
if (ChangeDetectionUtil.looseNotIdentical(prevValue, currValue)) {
411+
var detectedChange = throwOnChange ?
412+
!ChangeDetectionUtil.devModeEqual(prevValue, currValue) :
413+
ChangeDetectionUtil.looseNotIdentical(prevValue, currValue);
414+
if (detectedChange) {
409415
currValue = ChangeDetectionUtil.unwrapValue(currValue);
410416

411417
if (proto.lastInBinding) {

‎modules/angular2/src/facade/collection.dart‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,19 @@ class ListWrapper {
225225

226226
bool isListLikeIterable(obj) => obj is Iterable;
227227

228+
bool areIterablesEqual(Iterable a, Iterable b, Function comparator) {
229+
var iterator1 = a.iterator;
230+
var iterator2 = b.iterator;
231+
232+
while (true) {
233+
var done1 = !iterator1.moveNext();
234+
var done2 = !iterator2.moveNext();
235+
if (done1 && done2) return true;
236+
if (done1 || done2) return false;
237+
if (!comparator(iterator2.current, iterator2.current)) return false;
238+
}
239+
}
240+
228241
void iterateListLike(iter, fn(item)) {
229242
assert(iter is Iterable);
230243
for (var item in iter) {

‎modules/angular2/src/facade/collection.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,19 @@ export function isListLikeIterable(obj: any): boolean {
274274
getSymbolIterator() in obj); // JS Iterable have a Symbol.iterator prop
275275
}
276276

277+
export function areIterablesEqual(a: any, b: any, comparator: Function): boolean {
278+
var iterator1 = a[getSymbolIterator()]();
279+
var iterator2 = b[getSymbolIterator()]();
280+
281+
while (true) {
282+
let item1 = iterator1.next();
283+
let item2 = iterator2.next();
284+
if (item1.done && item2.done) return true;
285+
if (item1.done || item2.done) return false;
286+
if (!comparator(item1.value, item2.value)) return false;
287+
}
288+
}
289+
277290
export function iterateListLike(obj: any, fn: Function) {
278291
if (isArray(obj)) {
279292
for (var i = 0; i < obj.length; i++) {

‎modules/angular2/src/facade/lang.dart‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -345,6 +345,8 @@ class DateWrapper {
345345
}
346346
}
347347

348+
bool isPrimitive(Object obj) => obj is num || obj is bool || obj == null || obj is String;
349+
348350
// needed to match the exports from lang.js
349351
var global = null;
350352

‎modules/angular2/src/facade/lang.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,3 +427,7 @@ export function evalExpression(sourceUrl: string, expr: string, declarations: st
427427
}
428428
return new Function(...fnArgNames.concat(fnBody))(...fnArgValues);
429429
}
430+
431+
export function isPrimitive(obj: any): boolean {
432+
return !isJsObject(obj);
433+
}

‎modules/angular2/test/core/change_detection/change_detector_spec.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -887,6 +887,13 @@ export function main() {
887887
'Expression [\'"]a in location[\'"] has changed after it was checked'));
888888
});
889889

890+
it('should not throw when two arrays are structurally the same', () => {
891+
var val = _createChangeDetector('a', new TestDataWithGetter(() => ['value']));
892+
val.changeDetector.detectChanges();
893+
894+
expect(() => { val.changeDetector.checkNoChanges(); }).not.toThrow();
895+
});
896+
890897
it('should not break the next run', () => {
891898
var val = _createChangeDetector('a', new TestData('value'));
892899
expect(() => val.changeDetector.checkNoChanges())
@@ -1597,6 +1604,12 @@ class TestData {
15971604
constructor(public a: any) {}
15981605
}
15991606

1607+
class TestDataWithGetter {
1608+
constructor(private fn: Function) {}
1609+
1610+
get a() { return this.fn(); }
1611+
}
1612+
16001613
class TestDispatcher implements ChangeDispatcher {
16011614
log: string[];
16021615
debugLog: string[];

‎modules/angular2/test/core/change_detection/change_detector_util_spec.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
import {
2+
ddescribe,
3+
describe,
4+
it,
5+
iit,
6+
xit,
7+
expect,
8+
beforeEach,
9+
afterEach
10+
} from 'angular2/testing_internal';
11+
12+
import {ChangeDetectionUtil} from 'angular2/src/core/change_detection/change_detection_util';
13+
14+
export function main() {
15+
describe("ChangeDetectionUtil", () => {
16+
describe("devModeEqual", () => {
17+
it("should do the deep comparison of iterables", () => {
18+
expect(ChangeDetectionUtil.devModeEqual([['one']], [['one']])).toBe(true);
19+
expect(ChangeDetectionUtil.devModeEqual(['one'], ['one', 'two'])).toBe(false);
20+
expect(ChangeDetectionUtil.devModeEqual(['one', 'two'], ['one'])).toBe(false);
21+
expect(ChangeDetectionUtil.devModeEqual(['one'], 'one')).toBe(false);
22+
expect(ChangeDetectionUtil.devModeEqual(['one'], new Object())).toBe(false);
23+
expect(ChangeDetectionUtil.devModeEqual('one', ['one'])).toBe(false);
24+
expect(ChangeDetectionUtil.devModeEqual(new Object(), ['one'])).toBe(false);
25+
});
26+
27+
it("should compare primitive numbers", () => {
28+
expect(ChangeDetectionUtil.devModeEqual(1, 1)).toBe(true);
29+
expect(ChangeDetectionUtil.devModeEqual(1, 2)).toBe(false);
30+
expect(ChangeDetectionUtil.devModeEqual(new Object(), 2)).toBe(false);
31+
expect(ChangeDetectionUtil.devModeEqual(1, new Object())).toBe(false);
32+
});
33+
34+
it("should compare primitive strings", () => {
35+
expect(ChangeDetectionUtil.devModeEqual('one', 'one')).toBe(true);
36+
expect(ChangeDetectionUtil.devModeEqual('one', 'two')).toBe(false);
37+
expect(ChangeDetectionUtil.devModeEqual(new Object(), 'one')).toBe(false);
38+
expect(ChangeDetectionUtil.devModeEqual('one', new Object())).toBe(false);
39+
});
40+
41+
it("should compare primitive booleans", () => {
42+
expect(ChangeDetectionUtil.devModeEqual(true, true)).toBe(true);
43+
expect(ChangeDetectionUtil.devModeEqual(true, false)).toBe(false);
44+
expect(ChangeDetectionUtil.devModeEqual(new Object(), true)).toBe(false);
45+
expect(ChangeDetectionUtil.devModeEqual(true, new Object())).toBe(false);
46+
});
47+
48+
it("should compare null", () => {
49+
expect(ChangeDetectionUtil.devModeEqual(null, null)).toBe(true);
50+
expect(ChangeDetectionUtil.devModeEqual(null, 1)).toBe(false);
51+
expect(ChangeDetectionUtil.devModeEqual(new Object(), null)).toBe(false);
52+
expect(ChangeDetectionUtil.devModeEqual(null, new Object())).toBe(false);
53+
});
54+
55+
it("should return true for other objects", () => {
56+
expect(ChangeDetectionUtil.devModeEqual(new Object(), new Object())).toBe(true);
57+
});
58+
});
59+
});
60+
}

‎modules_dart/transform/lib/src/transform/template_compiler/change_detector_codegen.dart‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,7 @@ class _CodegenState {
433433
var condition = '''!${pipe}.pure || (${contexOrArgCheck.join(" || ")})''';
434434

435435
var check = '''
436+
${_genThrowOnChangeCheck(oldValue, newValue)}
436437
if (${_genPrefix}$_UTIL.looseNotIdentical($oldValue, $newValue)) {
437438
$newValue = ${_genPrefix}$_UTIL.unwrapValue($newValue);
438439
${_genChangeMarker(r)}
@@ -459,6 +460,7 @@ class _CodegenState {
459460
''';
460461

461462
var check = '''
463+
${_genThrowOnChangeCheck(oldValue, newValue)}
462464
if (${_genPrefix}$_UTIL.looseNotIdentical($newValue, $oldValue)) {
463465
${_genChangeMarker(r)}
464466
${_genUpdateDirectiveOrElement(r)}
@@ -492,7 +494,6 @@ class _CodegenState {
492494
if (!r.lastInBinding) return '';
493495

494496
var newValue = _names.getLocalName(r.selfIndex);
495-
var oldValue = _names.getFieldName(r.selfIndex);
496497
var notifyDebug = _genConfig.logBindingUpdate
497498
? "this.logBindingUpdate(${newValue});"
498499
: "";
@@ -502,14 +503,12 @@ class _CodegenState {
502503
var directiveProperty =
503504
'${_names.getDirectiveName(br.directiveRecord.directiveIndex)}.${br.target.name}';
504505
return '''
505-
${_genThrowOnChangeCheck(oldValue, newValue)}
506506
$directiveProperty = $newValue;
507507
${notifyDebug}
508508
$_IS_CHANGED_LOCAL = true;
509509
''';
510510
} else {
511511
return '''
512-
${_genThrowOnChangeCheck(oldValue, newValue)}
513512
this.notifyDispatcher(${newValue});
514513
${notifyDebug}
515514
''';
@@ -518,7 +517,7 @@ class _CodegenState {
518517

519518
String _genThrowOnChangeCheck(String oldValue, String newValue) {
520519
return '''
521-
if(${_genPrefix}assertionsEnabled() && throwOnChange) {
520+
if(${_genPrefix}assertionsEnabled() && throwOnChange && !${_genPrefix}${_UTIL}.devModeEqual(${oldValue}, ${newValue})) {
522521
this.throwOnChangeError(${oldValue}, ${newValue});
523522
}
524523
''';

0 commit comments

Comments
 (0)