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.
This commit is contained in:
parent
db87baeb98
commit
42231f5719
|
@ -350,6 +350,7 @@ export class ChangeDetectorJITGenerator {
|
||||||
var condition = `!${pipe}.pure || (${contexOrArgCheck.join(" || ")})`;
|
var condition = `!${pipe}.pure || (${contexOrArgCheck.join(" || ")})`;
|
||||||
|
|
||||||
var check = `
|
var check = `
|
||||||
|
${this._genThrowOnChangeCheck(oldValue, newValue)}
|
||||||
if (${this.changeDetectionUtilVarName}.looseNotIdentical(${oldValue}, ${newValue})) {
|
if (${this.changeDetectionUtilVarName}.looseNotIdentical(${oldValue}, ${newValue})) {
|
||||||
${newValue} = ${this.changeDetectionUtilVarName}.unwrapValue(${newValue})
|
${newValue} = ${this.changeDetectionUtilVarName}.unwrapValue(${newValue})
|
||||||
${this._genChangeMarker(r)}
|
${this._genChangeMarker(r)}
|
||||||
|
@ -377,6 +378,7 @@ export class ChangeDetectorJITGenerator {
|
||||||
`;
|
`;
|
||||||
|
|
||||||
var check = `
|
var check = `
|
||||||
|
${this._genThrowOnChangeCheck(oldValue, newValue)}
|
||||||
if (${this.changeDetectionUtilVarName}.looseNotIdentical(${oldValue}, ${newValue})) {
|
if (${this.changeDetectionUtilVarName}.looseNotIdentical(${oldValue}, ${newValue})) {
|
||||||
${this._genChangeMarker(r)}
|
${this._genChangeMarker(r)}
|
||||||
${this._genUpdateDirectiveOrElement(r)}
|
${this._genUpdateDirectiveOrElement(r)}
|
||||||
|
@ -409,7 +411,6 @@ export class ChangeDetectorJITGenerator {
|
||||||
if (!r.lastInBinding) return "";
|
if (!r.lastInBinding) return "";
|
||||||
|
|
||||||
var newValue = this._names.getLocalName(r.selfIndex);
|
var newValue = this._names.getLocalName(r.selfIndex);
|
||||||
var oldValue = this._names.getFieldName(r.selfIndex);
|
|
||||||
var notifyDebug = this.genConfig.logBindingUpdate ? `this.logBindingUpdate(${newValue});` : "";
|
var notifyDebug = this.genConfig.logBindingUpdate ? `this.logBindingUpdate(${newValue});` : "";
|
||||||
|
|
||||||
var br = r.bindingRecord;
|
var br = r.bindingRecord;
|
||||||
|
@ -417,14 +418,12 @@ export class ChangeDetectorJITGenerator {
|
||||||
var directiveProperty =
|
var directiveProperty =
|
||||||
`${this._names.getDirectiveName(br.directiveRecord.directiveIndex)}.${br.target.name}`;
|
`${this._names.getDirectiveName(br.directiveRecord.directiveIndex)}.${br.target.name}`;
|
||||||
return `
|
return `
|
||||||
${this._genThrowOnChangeCheck(oldValue, newValue)}
|
|
||||||
${directiveProperty} = ${newValue};
|
${directiveProperty} = ${newValue};
|
||||||
${notifyDebug}
|
${notifyDebug}
|
||||||
${IS_CHANGED_LOCAL} = true;
|
${IS_CHANGED_LOCAL} = true;
|
||||||
`;
|
`;
|
||||||
} else {
|
} else {
|
||||||
return `
|
return `
|
||||||
${this._genThrowOnChangeCheck(oldValue, newValue)}
|
|
||||||
this.notifyDispatcher(${newValue});
|
this.notifyDispatcher(${newValue});
|
||||||
${notifyDebug}
|
${notifyDebug}
|
||||||
`;
|
`;
|
||||||
|
@ -435,7 +434,7 @@ export class ChangeDetectorJITGenerator {
|
||||||
_genThrowOnChangeCheck(oldValue: string, newValue: string): string {
|
_genThrowOnChangeCheck(oldValue: string, newValue: string): string {
|
||||||
if (assertionsEnabled()) {
|
if (assertionsEnabled()) {
|
||||||
return `
|
return `
|
||||||
if(throwOnChange) {
|
if (throwOnChange && !${this.changeDetectionUtilVarName}.devModeEqual(${oldValue}, ${newValue})) {
|
||||||
this.throwOnChangeError(${oldValue}, ${newValue});
|
this.throwOnChangeError(${oldValue}, ${newValue});
|
||||||
}
|
}
|
||||||
`;
|
`;
|
||||||
|
|
|
@ -4,10 +4,17 @@ import {
|
||||||
isBlank,
|
isBlank,
|
||||||
Type,
|
Type,
|
||||||
StringWrapper,
|
StringWrapper,
|
||||||
looseIdentical
|
looseIdentical,
|
||||||
|
isPrimitive
|
||||||
} from 'angular2/src/facade/lang';
|
} from 'angular2/src/facade/lang';
|
||||||
import {BaseException} from 'angular2/src/facade/exceptions';
|
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 {ProtoRecord} from './proto_record';
|
||||||
import {ChangeDetectionStrategy, isDefaultChangeDetectionStrategy} from './constants';
|
import {ChangeDetectionStrategy, isDefaultChangeDetectionStrategy} from './constants';
|
||||||
import {implementsOnDestroy} from './pipe_lifecycle_reflector';
|
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 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);
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
@ -304,7 +304,10 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
|
||||||
|
|
||||||
if (proto.shouldBeChecked()) {
|
if (proto.shouldBeChecked()) {
|
||||||
var prevValue = this._readSelf(proto, values);
|
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) {
|
if (proto.lastInBinding) {
|
||||||
var change = ChangeDetectionUtil.simpleChange(prevValue, currValue);
|
var change = ChangeDetectionUtil.simpleChange(prevValue, currValue);
|
||||||
if (throwOnChange) this.throwOnChangeError(prevValue, currValue);
|
if (throwOnChange) this.throwOnChangeError(prevValue, currValue);
|
||||||
|
@ -405,7 +408,10 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
|
||||||
|
|
||||||
if (proto.shouldBeChecked()) {
|
if (proto.shouldBeChecked()) {
|
||||||
var prevValue = this._readSelf(proto, values);
|
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);
|
currValue = ChangeDetectionUtil.unwrapValue(currValue);
|
||||||
|
|
||||||
if (proto.lastInBinding) {
|
if (proto.lastInBinding) {
|
||||||
|
|
|
@ -225,6 +225,19 @@ class ListWrapper {
|
||||||
|
|
||||||
bool isListLikeIterable(obj) => obj is Iterable;
|
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)) {
|
void iterateListLike(iter, fn(item)) {
|
||||||
assert(iter is Iterable);
|
assert(iter is Iterable);
|
||||||
for (var item in iter) {
|
for (var item in iter) {
|
||||||
|
|
|
@ -274,6 +274,19 @@ export function isListLikeIterable(obj: any): boolean {
|
||||||
getSymbolIterator() in obj); // JS Iterable have a Symbol.iterator prop
|
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) {
|
export function iterateListLike(obj: any, fn: Function) {
|
||||||
if (isArray(obj)) {
|
if (isArray(obj)) {
|
||||||
for (var i = 0; i < obj.length; i++) {
|
for (var i = 0; i < obj.length; i++) {
|
||||||
|
|
|
@ -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
|
// needed to match the exports from lang.js
|
||||||
var global = null;
|
var global = null;
|
||||||
|
|
||||||
|
|
|
@ -427,3 +427,7 @@ export function evalExpression(sourceUrl: string, expr: string, declarations: st
|
||||||
}
|
}
|
||||||
return new Function(...fnArgNames.concat(fnBody))(...fnArgValues);
|
return new Function(...fnArgNames.concat(fnBody))(...fnArgValues);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
export function isPrimitive(obj: any): boolean {
|
||||||
|
return !isJsObject(obj);
|
||||||
|
}
|
|
@ -887,6 +887,13 @@ export function main() {
|
||||||
'Expression [\'"]a in location[\'"] has changed after it was checked'));
|
'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', () => {
|
it('should not break the next run', () => {
|
||||||
var val = _createChangeDetector('a', new TestData('value'));
|
var val = _createChangeDetector('a', new TestData('value'));
|
||||||
expect(() => val.changeDetector.checkNoChanges())
|
expect(() => val.changeDetector.checkNoChanges())
|
||||||
|
@ -1597,6 +1604,12 @@ class TestData {
|
||||||
constructor(public a: any) {}
|
constructor(public a: any) {}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
class TestDataWithGetter {
|
||||||
|
constructor(private fn: Function) {}
|
||||||
|
|
||||||
|
get a() { return this.fn(); }
|
||||||
|
}
|
||||||
|
|
||||||
class TestDispatcher implements ChangeDispatcher {
|
class TestDispatcher implements ChangeDispatcher {
|
||||||
log: string[];
|
log: string[];
|
||||||
debugLog: string[];
|
debugLog: string[];
|
||||||
|
|
|
@ -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);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
||||||
|
}
|
|
@ -433,6 +433,7 @@ class _CodegenState {
|
||||||
var condition = '''!${pipe}.pure || (${contexOrArgCheck.join(" || ")})''';
|
var condition = '''!${pipe}.pure || (${contexOrArgCheck.join(" || ")})''';
|
||||||
|
|
||||||
var check = '''
|
var check = '''
|
||||||
|
${_genThrowOnChangeCheck(oldValue, newValue)}
|
||||||
if (${_genPrefix}$_UTIL.looseNotIdentical($oldValue, $newValue)) {
|
if (${_genPrefix}$_UTIL.looseNotIdentical($oldValue, $newValue)) {
|
||||||
$newValue = ${_genPrefix}$_UTIL.unwrapValue($newValue);
|
$newValue = ${_genPrefix}$_UTIL.unwrapValue($newValue);
|
||||||
${_genChangeMarker(r)}
|
${_genChangeMarker(r)}
|
||||||
|
@ -459,6 +460,7 @@ class _CodegenState {
|
||||||
''';
|
''';
|
||||||
|
|
||||||
var check = '''
|
var check = '''
|
||||||
|
${_genThrowOnChangeCheck(oldValue, newValue)}
|
||||||
if (${_genPrefix}$_UTIL.looseNotIdentical($newValue, $oldValue)) {
|
if (${_genPrefix}$_UTIL.looseNotIdentical($newValue, $oldValue)) {
|
||||||
${_genChangeMarker(r)}
|
${_genChangeMarker(r)}
|
||||||
${_genUpdateDirectiveOrElement(r)}
|
${_genUpdateDirectiveOrElement(r)}
|
||||||
|
@ -492,7 +494,6 @@ class _CodegenState {
|
||||||
if (!r.lastInBinding) return '';
|
if (!r.lastInBinding) return '';
|
||||||
|
|
||||||
var newValue = _names.getLocalName(r.selfIndex);
|
var newValue = _names.getLocalName(r.selfIndex);
|
||||||
var oldValue = _names.getFieldName(r.selfIndex);
|
|
||||||
var notifyDebug = _genConfig.logBindingUpdate
|
var notifyDebug = _genConfig.logBindingUpdate
|
||||||
? "this.logBindingUpdate(${newValue});"
|
? "this.logBindingUpdate(${newValue});"
|
||||||
: "";
|
: "";
|
||||||
|
@ -502,14 +503,12 @@ class _CodegenState {
|
||||||
var directiveProperty =
|
var directiveProperty =
|
||||||
'${_names.getDirectiveName(br.directiveRecord.directiveIndex)}.${br.target.name}';
|
'${_names.getDirectiveName(br.directiveRecord.directiveIndex)}.${br.target.name}';
|
||||||
return '''
|
return '''
|
||||||
${_genThrowOnChangeCheck(oldValue, newValue)}
|
|
||||||
$directiveProperty = $newValue;
|
$directiveProperty = $newValue;
|
||||||
${notifyDebug}
|
${notifyDebug}
|
||||||
$_IS_CHANGED_LOCAL = true;
|
$_IS_CHANGED_LOCAL = true;
|
||||||
''';
|
''';
|
||||||
} else {
|
} else {
|
||||||
return '''
|
return '''
|
||||||
${_genThrowOnChangeCheck(oldValue, newValue)}
|
|
||||||
this.notifyDispatcher(${newValue});
|
this.notifyDispatcher(${newValue});
|
||||||
${notifyDebug}
|
${notifyDebug}
|
||||||
''';
|
''';
|
||||||
|
@ -518,7 +517,7 @@ class _CodegenState {
|
||||||
|
|
||||||
String _genThrowOnChangeCheck(String oldValue, String newValue) {
|
String _genThrowOnChangeCheck(String oldValue, String newValue) {
|
||||||
return '''
|
return '''
|
||||||
if(${_genPrefix}assertionsEnabled() && throwOnChange) {
|
if(${_genPrefix}assertionsEnabled() && throwOnChange && !${_genPrefix}${_UTIL}.devModeEqual(${oldValue}, ${newValue})) {
|
||||||
this.throwOnChangeError(${oldValue}, ${newValue});
|
this.throwOnChangeError(${oldValue}, ${newValue});
|
||||||
}
|
}
|
||||||
''';
|
''';
|
||||||
|
|
Loading…
Reference in New Issue