From 8acf9fb6091303f50cc083f6e1a0b0243e26239a Mon Sep 17 00:00:00 2001 From: vsavkin Date: Thu, 4 Dec 2014 18:30:54 -0800 Subject: [PATCH] feat(change_detection): ensure that expression do not change after they have been checked --- .../change_detection/src/change_detector.js | 26 ++++++++++++++++++- modules/change_detection/src/parser/ast.js | 26 ++++++++++++++++++- modules/change_detection/src/record.js | 9 ++++++- modules/change_detection/src/record_range.js | 9 ++++--- .../test/change_detector_spec.js | 25 ++++++++++++++++-- .../test/record_range_spec.js | 2 +- modules/core/src/application.js | 4 +-- .../pipeline/element_binder_builder.js | 8 +++--- modules/core/test/compiler/view_spec.js | 23 ++++++++-------- modules/facade/src/lang.dart | 9 +++++++ modules/facade/src/lang.es6 | 9 +++++++ 11 files changed, 123 insertions(+), 27 deletions(-) diff --git a/modules/change_detection/src/change_detector.js b/modules/change_detection/src/change_detector.js index 79a3baa392..2522245490 100644 --- a/modules/change_detection/src/change_detector.js +++ b/modules/change_detection/src/change_detector.js @@ -6,14 +6,37 @@ import {ListWrapper, List} from 'facade/collection'; export * from './record'; export * from './record_range' +class ExpressionChangedAfterItHasBeenChecked extends Error { + message:string; + + constructor(record:Record) { + this.message = `Expression '${record.expressionAsString()}' has changed after it was checked. ` + + `Previous value: '${record.previousValue}'. Current value: '${record.currentValue}'`; + } + + toString():string { + return this.message; + } +} + export class ChangeDetector { _rootRecordRange:RecordRange; + _enforceNoNewChanges:boolean; - constructor(recordRange:RecordRange) { + constructor(recordRange:RecordRange, enforceNoNewChanges:boolean = false) { this._rootRecordRange = recordRange; + this._enforceNoNewChanges = enforceNoNewChanges; } detectChanges():int { + var count = this._detectChanges(false); + if (this._enforceNoNewChanges) { + this._detectChanges(true) + } + return count; + } + + _detectChanges(throwOnChange:boolean):int { var count = 0; var updatedRecords = null; var record = this._rootRecordRange.findFirstEnabledRecord(); @@ -23,6 +46,7 @@ export class ChangeDetector { if (record.check()) { count++; if (record.terminatesExpression()) { + if (throwOnChange) throw new ExpressionChangedAfterItHasBeenChecked(record); currentRange = record.recordRange; currentGroup = record.groupMemento(); updatedRecords = this._addRecord(updatedRecords, record); diff --git a/modules/change_detection/src/parser/ast.js b/modules/change_detection/src/parser/ast.js index 0133276ef3..cc5302c9ea 100644 --- a/modules/change_detection/src/parser/ast.js +++ b/modules/change_detection/src/parser/ast.js @@ -17,6 +17,10 @@ export class AST { visit(visitor, args) { } + + toString():string { + return "AST"; + } } export class EmptyExpr extends AST { @@ -378,13 +382,33 @@ export class FunctionCall extends AST { } } -export class ASTWithSource { +export class ASTWithSource extends AST { ast:AST; source:string; constructor(ast:AST, source:string) { this.source = source; this.ast = ast; } + + eval(context) { + return this.ast.eval(context); + } + + get isAssignable() { + return this.ast.isAssignable; + } + + assign(context, value) { + return this.ast.assign(context, value); + } + + visit(visitor, args) { + return this.ast.visit(visitor, args); + } + + toString():string { + return this.source; + } } export class TemplateBinding { diff --git a/modules/change_detection/src/record.js b/modules/change_detection/src/record.js index e617c0e1e7..94e1b55df1 100644 --- a/modules/change_detection/src/record.js +++ b/modules/change_detection/src/record.js @@ -35,6 +35,7 @@ export class ProtoRecord { name:string; dest:any; groupMemento:any; + expressionAsString:string; next:ProtoRecord; @@ -46,7 +47,8 @@ export class ProtoRecord { arity:int, name:string, dest, - groupMemento) { + groupMemento, + expressionAsString:string) { this.recordRange = recordRange; this._mode = mode; @@ -55,6 +57,7 @@ export class ProtoRecord { this.name = name; this.dest = dest; this.groupMemento = groupMemento; + this.expressionAsString = expressionAsString; this.next = null; // The concrete Record instantiated from this ProtoRecord @@ -341,6 +344,10 @@ export class Record { return this.protoRecord.dest; } + expressionAsString() { + return this.protoRecord.expressionAsString; + } + groupMemento() { return isPresent(this.protoRecord) ? this.protoRecord.groupMemento : null; } diff --git a/modules/change_detection/src/record_range.js b/modules/change_detection/src/record_range.js index 51e2739d9a..6448313bcb 100644 --- a/modules/change_detection/src/record_range.js +++ b/modules/change_detection/src/record_range.js @@ -44,7 +44,7 @@ export class ProtoRecordRange { /** * Parses [ast] into [ProtoRecord]s and adds them to [ProtoRecordRange]. * - * @param ast The expression to watch + * @param astWithSource The expression to watch * @param expressionMemento an opaque object which will be passed to ChangeDispatcher on * detecting a change. * @param groupMemento @@ -62,7 +62,6 @@ export class ProtoRecordRange { if (content) { ast = new Collection(ast); } - this.recordCreator.createRecordsFromAST(ast, expressionMemento, groupMemento); } @@ -318,11 +317,13 @@ class ProtoRecordCreator { headRecord:ProtoRecord; tailRecord:ProtoRecord; groupMemento:any; + expressionAsString:string; constructor(protoRecordRange) { this.protoRecordRange = protoRecordRange; this.headRecord = null; this.tailRecord = null; + this.expressionAsString = null; } visitImplicitReceiver(ast:ImplicitReceiver, args) { @@ -434,11 +435,13 @@ class ProtoRecordCreator { createRecordsFromAST(ast:AST, expressionMemento:any, groupMemento:any){ this.groupMemento = groupMemento; + this.expressionAsString = ast.toString(); ast.visit(this, expressionMemento); } construct(recordType, funcOrValue, arity, name, dest) { - return new ProtoRecord(this.protoRecordRange, recordType, funcOrValue, arity, name, dest, this.groupMemento); + return new ProtoRecord(this.protoRecordRange, recordType, funcOrValue, arity, + name, dest, this.groupMemento, this.expressionAsString); } add(protoRecord:ProtoRecord) { diff --git a/modules/change_detection/test/change_detector_spec.js b/modules/change_detection/test/change_detector_spec.js index 623a76b571..b22b50434d 100644 --- a/modules/change_detection/test/change_detector_spec.js +++ b/modules/change_detection/test/change_detector_spec.js @@ -20,7 +20,7 @@ import {Record} from 'change_detection/record'; export function main() { function ast(exp:string) { var parser = new Parser(new Lexer()); - return parser.parseBinding(exp).ast; + return parser.parseBinding(exp); } function createChangeDetector(memo:string, exp:string, context = null, formatters = null, @@ -433,6 +433,27 @@ export function main() { expect(dispatcher.loggedValues).toEqual(['InvokeA', ['a'], 'InvokeB', 'InvokeC', ['b', 'c']]); }); }); + + describe("enforce no new changes", () => { + it("should throw when a record gets changed after it has been checked", () => { + var prr = new ProtoRecordRange(); + prr.addRecordsFromAST(ast("a"), "a", 1); + prr.addRecordsFromAST(ast("b()"), "b", 2); + + var tr = new TestRecord(); + tr.a = "a"; + tr.b = () => {tr.a = "newA";}; + + var dispatcher = new TestDispatcher(); + var rr = prr.instantiate(dispatcher, null); + rr.setContext(tr); + + expect(() => { + var cd = new ChangeDetector(rr, true); + cd.detectChanges(); + }).toThrowError(new RegExp("Expression 'a' has changed after it was checked")); + }); + }); }); }); } @@ -513,6 +534,6 @@ class TestDispatcher extends ChangeDispatcher { } _asString(value) { - return (value === null ? 'null' : value.toString()); + return (isBlank(value) ? 'null' : value.toString()); } } diff --git a/modules/change_detection/test/record_range_spec.js b/modules/change_detection/test/record_range_spec.js index e1232e5917..ddb0cd7c6d 100644 --- a/modules/change_detection/test/record_range_spec.js +++ b/modules/change_detection/test/record_range_spec.js @@ -45,7 +45,7 @@ export function main() { } function createRecord(rr) { - return new Record(rr, new ProtoRecord(null, 0, null, null, null, null, null), null); + return new Record(rr, new ProtoRecord(null, 0, null, null, null, null, null, null), null); } describe('record range', () => { diff --git a/modules/core/src/application.js b/modules/core/src/application.js index a9905670f2..9eba479b38 100644 --- a/modules/core/src/application.js +++ b/modules/core/src/application.js @@ -1,5 +1,5 @@ import {Injector, bind, OpaqueToken} from 'di/di'; -import {Type, FIELD, isBlank, isPresent, BaseException} from 'facade/lang'; +import {Type, FIELD, isBlank, isPresent, BaseException, assertionsEnabled} from 'facade/lang'; import {DOM, Element} from 'facade/dom'; import {Compiler, CompilerCache} from './compiler/compiler'; import {ProtoView} from './compiler/view'; @@ -63,7 +63,7 @@ export function documentDependentBindings(appComponentType) { bind(appRecordRangeToken).toFactory((rootView) => rootView.recordRange, [appViewToken]), bind(ChangeDetector).toFactory((appRecordRange) => - new ChangeDetector(appRecordRange), [appRecordRangeToken]), + new ChangeDetector(appRecordRange, assertionsEnabled()), [appRecordRangeToken]), bind(appComponentType).toFactory((rootView) => rootView.elementInjectors[0].getComponent(), [appViewToken]) ]; diff --git a/modules/core/src/compiler/pipeline/element_binder_builder.js b/modules/core/src/compiler/pipeline/element_binder_builder.js index 5f90aae25c..829f6aa6ac 100644 --- a/modules/core/src/compiler/pipeline/element_binder_builder.js +++ b/modules/core/src/compiler/pipeline/element_binder_builder.js @@ -68,19 +68,19 @@ export class ElementBinderBuilder extends CompileStep { _bindTextNodes(protoView, compileElement) { MapWrapper.forEach(compileElement.textNodeBindings, (expression, indexInParent) => { - protoView.bindTextNode(indexInParent, expression.ast); + protoView.bindTextNode(indexInParent, expression); }); } _bindElementProperties(protoView, compileElement) { MapWrapper.forEach(compileElement.propertyBindings, (expression, property) => { - protoView.bindElementProperty(property, expression.ast); + protoView.bindElementProperty(property, expression); }); } _bindEvents(protoView, compileElement) { MapWrapper.forEach(compileElement.eventBindings, (expression, eventName) => { - protoView.bindEvent(eventName, expression.ast); + protoView.bindEvent(eventName, expression); }); } @@ -118,7 +118,7 @@ export class ElementBinderBuilder extends CompileStep { } protoView.bindDirectiveProperty( directiveIndex++, - expression.ast, + expression, dirProp, reflector.setter(dirProp) ); diff --git a/modules/core/test/compiler/view_spec.js b/modules/core/test/compiler/view_spec.js index ecb1711eb9..a33383923a 100644 --- a/modules/core/test/compiler/view_spec.js +++ b/modules/core/test/compiler/view_spec.js @@ -105,7 +105,7 @@ export function main() { it('should collect property bindings on the root element if it has the ng-binding class', () => { var pv = new ProtoView(templateAwareCreateElement('
'), new ProtoRecordRange()); pv.bindElement(null); - pv.bindElementProperty('prop', parser.parseBinding('a').ast); + pv.bindElementProperty('prop', parser.parseBinding('a')); var view = pv.instantiate(null); view.hydrate(null, null, null); @@ -117,7 +117,7 @@ export function main() { var pv = new ProtoView(templateAwareCreateElement('
'), new ProtoRecordRange()); pv.bindElement(null); - pv.bindElementProperty('a', parser.parseBinding('b').ast); + pv.bindElementProperty('a', parser.parseBinding('b')); var view = pv.instantiate(null); view.hydrate(null, null, null); @@ -132,8 +132,8 @@ export function main() { it('should collect text nodes under the root element', () => { var pv = new ProtoView(templateAwareCreateElement('
{{}}{{}}
'), new ProtoRecordRange()); pv.bindElement(null); - pv.bindTextNode(0, parser.parseBinding('a').ast); - pv.bindTextNode(2, parser.parseBinding('b').ast); + pv.bindTextNode(0, parser.parseBinding('a')); + pv.bindTextNode(2, parser.parseBinding('b')); var view = pv.instantiate(null); view.hydrate(null, null, null); @@ -146,7 +146,7 @@ export function main() { var pv = new ProtoView(templateAwareCreateElement('
{{}}
'), new ProtoRecordRange()); pv.bindElement(null); - pv.bindTextNode(0, parser.parseBinding('b').ast); + pv.bindTextNode(0, parser.parseBinding('b')); var view = pv.instantiate(null); view.hydrate(null, null, null); @@ -357,7 +357,7 @@ export function main() { var pv = new ProtoView(createElement('
{{}}
'), new ProtoRecordRange()); pv.bindElement(null); - pv.bindTextNode(0, parser.parseBinding('foo').ast); + pv.bindTextNode(0, parser.parseBinding('foo')); createViewAndChangeDetector(pv); ctx.foo = 'buz'; @@ -369,7 +369,7 @@ export function main() { var pv = new ProtoView(createElement('
'), new ProtoRecordRange()); pv.bindElement(null); - pv.bindElementProperty('id', parser.parseBinding('foo').ast); + pv.bindElementProperty('id', parser.parseBinding('foo')); createViewAndChangeDetector(pv); ctx.foo = 'buz'; @@ -381,7 +381,7 @@ export function main() { var pv = new ProtoView(createElement('
'), new ProtoRecordRange()); pv.bindElement(new ProtoElementInjector(null, 0, [SomeDirective])); - pv.bindDirectiveProperty(0, parser.parseBinding('foo').ast, 'prop', reflector.setter('prop')); + pv.bindDirectiveProperty(0, parser.parseBinding('foo'), 'prop', reflector.setter('prop')); createViewAndChangeDetector(pv); ctx.foo = 'buz'; @@ -394,8 +394,8 @@ export function main() { new ProtoRecordRange()); pv.bindElement(new ProtoElementInjector(null, 0, [DirectiveImplementingOnChange])); - pv.bindDirectiveProperty( 0, parser.parseBinding('a').ast, 'a', reflector.setter('a')); - pv.bindDirectiveProperty( 0, parser.parseBinding('b').ast, 'b', reflector.setter('b')); + pv.bindDirectiveProperty( 0, parser.parseBinding('a'), 'a', reflector.setter('a')); + pv.bindDirectiveProperty( 0, parser.parseBinding('b'), 'b', reflector.setter('b')); createViewAndChangeDetector(pv); ctx.a = 100; @@ -413,8 +413,7 @@ export function main() { pv.bindElement(new ProtoElementInjector(null, 0, [DirectiveImplementingOnChange])); pv.bindDirectiveProperty( 0, parser.parseBinding('a').ast, 'a', reflector.setter('a')); pv.bindDirectiveProperty( 0, parser.parseBinding('b').ast, 'b', reflector.setter('b')); - createViewAndChangeDetector(pv); - + createView(pv); ctx.a = 0; ctx.b = 0; cd.detectChanges(); diff --git a/modules/facade/src/lang.dart b/modules/facade/src/lang.dart index a0f9982d21..bc9e857cd7 100644 --- a/modules/facade/src/lang.dart +++ b/modules/facade/src/lang.dart @@ -169,3 +169,12 @@ bool isJsObject(o) { return false; } + +assertionsEnabled() { + try { + assert(false); + return false; + } catch (e) { + return true; + } +} \ No newline at end of file diff --git a/modules/facade/src/lang.es6 b/modules/facade/src/lang.es6 index e7143b14b2..76b796ef24 100644 --- a/modules/facade/src/lang.es6 +++ b/modules/facade/src/lang.es6 @@ -206,3 +206,12 @@ export function normalizeBlank(obj) { export function isJsObject(o):boolean { return o !== null && (typeof o === "function" || typeof o === "object"); } + +export function assertionsEnabled() { + try { + var x:int = "string"; + return false; + } catch (e) { + return true; + } +} \ No newline at end of file