diff --git a/lib/change_detection/change_detection.dart b/lib/change_detection/change_detection.dart index 71f1b97dc..5919f1cc2 100644 --- a/lib/change_detection/change_detection.dart +++ b/lib/change_detection/change_detection.dart @@ -174,9 +174,6 @@ typedef dynamic FieldGetter(object); typedef void FieldSetter(object, value); abstract class FieldGetterFactory { - get isMethodInvoke; - bool isMethod(Object object, String name); - Function method(Object object, String name); FieldGetter getter(Object object, String name); } diff --git a/lib/change_detection/dirty_checking_change_detector.dart b/lib/change_detection/dirty_checking_change_detector.dart index ab940ac07..359c017bb 100644 --- a/lib/change_detection/dirty_checking_change_detector.dart +++ b/lib/change_detection/dirty_checking_change_detector.dart @@ -392,6 +392,8 @@ class DirtyCheckingRecord implements Record, WatchRecord { DirtyCheckingRecord _prevRecord; Record _nextChange; var _object; + var _identityValue; + bool _checkForVaryingClosure = false; FieldGetter _getter; DirtyCheckingRecord(this._group, this._fieldGetterFactory, this.handler, @@ -418,6 +420,7 @@ class DirtyCheckingRecord implements Record, WatchRecord { _object = obj; if (obj == null) { _mode = _MODE_IDENTITY_; + _identityValue = null; _getter = null; return; } @@ -451,6 +454,7 @@ class DirtyCheckingRecord implements Record, WatchRecord { } } else { _mode = _MODE_IDENTITY_; + _identityValue = obj; } return; @@ -460,14 +464,9 @@ class DirtyCheckingRecord implements Record, WatchRecord { _mode = _MODE_MAP_FIELD_; _getter = null; } else { - if (_fieldGetterFactory.isMethod(obj, field)) { - _mode = _MODE_IDENTITY_; - previousValue = currentValue = _fieldGetterFactory.method(obj, field)(obj); - assert(previousValue is Function); - } else { - _mode = _MODE_GETTER_; - _getter = _fieldGetterFactory.getter(obj, field); - } + _mode = _MODE_GETTER_; + _checkForVaryingClosure = true; + _getter = _fieldGetterFactory.getter(obj, field); } } @@ -479,12 +478,24 @@ class DirtyCheckingRecord implements Record, WatchRecord { return false; case _MODE_GETTER_: current = _getter(object); + if (_checkForVaryingClosure) { + _checkForVaryingClosure = false; + // NOTE: Method as handled as _MODE_IDENTITY_. When Dart looks up a + // method "foo" on object "x", it returns a new closure for each lookup. + // They compare equal via "==" but are no identical(). There's no point + // getting a new value each time and decide it's the same so we'll skip + // further checking after the first time. + if (current is Function && !identical(current, _getter(object))) { + _mode = _MODE_IDENTITY_; + _identityValue = current; + } + } break; case _MODE_MAP_FIELD_: current = object[field]; break; case _MODE_IDENTITY_: - current = object; + current = _identityValue; break; case _MODE_MAP_: return (currentValue as _MapChangeRecord)._check(object); diff --git a/lib/change_detection/dirty_checking_change_detector_dynamic.dart b/lib/change_detection/dirty_checking_change_detector_dynamic.dart index bc03d013d..f5f8bb973 100644 --- a/lib/change_detection/dirty_checking_change_detector_dynamic.dart +++ b/lib/change_detection/dirty_checking_change_detector_dynamic.dart @@ -11,25 +11,9 @@ export 'package:angular/change_detection/change_detection.dart' show import 'dart:mirrors'; class DynamicFieldGetterFactory implements FieldGetterFactory { - final isMethodInvoke = true; - - isMethod(Object object, String name) { - var declaration = reflectClass(object.runtimeType).declarations[new Symbol(name)]; - return declaration != null && - declaration is MethodMirror && - (declaration as MethodMirror).isRegularMethod; - } - - Function method(Object object, String name) { - Symbol symbol = new Symbol(name); - InstanceMirror instanceMirror = reflect(object); - return (List args, Map namedArgs) => - instanceMirror.invoke(symbol, args, namedArgs).reflectee; - } - FieldGetter getter(Object object, String name) { Symbol symbol = new Symbol(name); InstanceMirror instanceMirror = reflect(object); - return (Object object) => instanceMirror.getField(symbol).reflectee; + return (Object object) => instanceMirror.getField(symbol).reflectee; } } diff --git a/lib/change_detection/dirty_checking_change_detector_static.dart b/lib/change_detection/dirty_checking_change_detector_static.dart index 66bd245f7..1c7764dfd 100644 --- a/lib/change_detection/dirty_checking_change_detector_static.dart +++ b/lib/change_detection/dirty_checking_change_detector_static.dart @@ -3,30 +3,13 @@ library dirty_checking_change_detector_static; import 'package:angular/change_detection/change_detection.dart'; class StaticFieldGetterFactory implements FieldGetterFactory { - final isMethodInvoke = false; Map getters; StaticFieldGetterFactory(this.getters); - bool isMethod(Object object, String name) { - // We need to know if we are referring to method or field which is a - // function. We can find out by calling it twice and seeing if we get - // the same value. Methods create a new closure each time. - FieldGetter getterFn = getter(object, name); - dynamic property = getterFn(object); - return (property is Function) && - (!identical(property, getterFn(object))); - } - FieldGetter getter(Object object, String name) { var getter = getters[name]; if (getter == null) throw "Missing getter: (o) => o.$name"; return getter; } - - Function method(Object object, String name) { - var method = getters[name]; - if (method == null) throw "Missing method: $name"; - return method; - } } diff --git a/lib/change_detection/watch_group.dart b/lib/change_detection/watch_group.dart index b0dbd05f0..9690b19c5 100644 --- a/lib/change_detection/watch_group.dart +++ b/lib/change_detection/watch_group.dart @@ -26,8 +26,7 @@ typedef void ChangeLog(String expression, current, previous); * number of arguments with which the function will get called with. */ abstract class FunctionApply { - // dartbug.com/16401 - // dynamic call() { throw new StateError('Use apply()'); } + dynamic call() { throw new StateError('Use apply()'); } dynamic apply(List arguments); } @@ -198,7 +197,7 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList { * - [isPure] A pure function is one which holds no internal state. This implies that the * function is idempotent. */ - _EvalWatchRecord addFunctionWatch(/* dartbug.com/16401 Function */ fn, List argsAST, + _EvalWatchRecord addFunctionWatch(Function fn, List argsAST, Map namedArgsAST, String expression, bool isPure) => _addEvalWatch(null, fn, null, argsAST, namedArgsAST, expression, isPure); @@ -214,11 +213,11 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList { _EvalWatchRecord addMethodWatch(AST lhs, String name, List argsAST, Map namedArgsAST, String expression) => - _addEvalWatch(lhs, null, name, argsAST, namedArgsAST, expression, false); + _addEvalWatch(lhs, null, name, argsAST, namedArgsAST, expression, false); - _EvalWatchRecord _addEvalWatch(AST lhsAST, /* dartbug.com/16401 Function */ fn, String name, + _EvalWatchRecord _addEvalWatch(AST lhsAST, Function fn, String name, List argsAST, Map namedArgsAST, String expression, bool isPure) { @@ -722,19 +721,19 @@ class _EvalWatchRecord implements WatchRecord<_Handler> { static const int _MODE_FUNCTION_ = 2; static const int _MODE_PURE_FUNCTION_APPLY_ = 3; static const int _MODE_NULL_ = 4; - static const int _MODE_FIELD_CLOSURE_ = 5; - static const int _MODE_MAP_CLOSURE_ = 6; - static const int _MODE_METHOD_ = 7; - static const int _MODE_METHOD_INVOKE_ = 8; + static const int _MODE_METHOD_ = 5; + static const int _MODE_FIELD_CLOSURE_ = 6; + static const int _MODE_MAP_CLOSURE_ = 7; WatchGroup watchGrp; final _Handler handler; final List args; final Map namedArgs = new Map(); final String name; int mode; - /* dartbug.com/16401 Function*/ var fn; + Function fn; FieldGetterFactory _fieldGetterFactory; bool dirtyArgs = true; + bool _checkForVaryingClosure = false; dynamic currentValue, previousValue, _object; _EvalWatchRecord _prevEvalWatch, _nextEvalWatch; @@ -789,13 +788,9 @@ class _EvalWatchRecord implements WatchRecord<_Handler> { if (value is Map) { mode = _MODE_MAP_CLOSURE_; } else { - if (_fieldGetterFactory.isMethod(value, name)) { - mode = _fieldGetterFactory.isMethodInvoke ? _MODE_METHOD_INVOKE_ : _MODE_METHOD_; - fn = _fieldGetterFactory.method(value, name); - } else { - mode = _MODE_FIELD_CLOSURE_; - fn = _fieldGetterFactory.getter(value, name); - } + mode = _MODE_FIELD_CLOSURE_; + fn = _fieldGetterFactory.getter(value, name); + _checkForVaryingClosure = true; } } } @@ -820,19 +815,28 @@ class _EvalWatchRecord implements WatchRecord<_Handler> { value = (fn as FunctionApply).apply(args); dirtyArgs = false; break; + case _MODE_METHOD_: + value = Function.apply(fn, args, namedArgs); + break; case _MODE_FIELD_CLOSURE_: var closure = fn(_object); - value = closure == null ? null : Function.apply(closure, args, namedArgs); + if (_checkForVaryingClosure) { + _checkForVaryingClosure = false; + // NOTE: Method as handled as _MODE_IDENTITY_. When Dart looks up a + // method "foo" on object "x", it returns a new closure for each lookup. + // They compare equal via "==" but are no identical(). There's no point + // getting a new value each time and decide it's the same so we'll skip + // further checking after the first time. + if (closure is Function && !identical(closure, fn(_object))) { + mode = _MODE_METHOD_; + fn = closure; + } + } + value = (closure == null) ? null : Function.apply(closure, args, namedArgs); break; case _MODE_MAP_CLOSURE_: var closure = object[name]; - value = closure == null ? null : Function.apply(closure, args, namedArgs); - break; - case _MODE_METHOD_: - value = Function.apply(fn, args, namedArgs); - break; - case _MODE_METHOD_INVOKE_: - value = fn(args, namedArgs); + value = (closure == null) ? null : Function.apply(closure, args, namedArgs); break; default: assert(false); diff --git a/test/change_detection/dirty_checking_change_detector_spec.dart b/test/change_detection/dirty_checking_change_detector_spec.dart index 5a5c01822..1107cb28d 100644 --- a/test/change_detection/dirty_checking_change_detector_spec.dart +++ b/test/change_detection/dirty_checking_change_detector_spec.dart @@ -8,89 +8,14 @@ import 'package:angular/change_detection/dirty_checking_change_detector_dynamic. import 'dart:collection'; import 'dart:math'; -void main() { - describe('DirtyCheckingChangeDetector', () { +void testWithGetterFactory(FieldGetterFactory getterFactory) { + describe('DirtyCheckingChangeDetector with ${getterFactory.runtimeType}', () { DirtyCheckingChangeDetector detector; - FieldGetterFactory getterFactory = new StaticFieldGetterFactory({ - "first": (o) => o.first, - "age": (o) => o.age, - "last": (o) => o.last, - "toString": (o) => o.toString, - "isUnderAge": (o) => o.isUnderAge, - "isUnderAgeAsVariable": (o) => o.isUnderAgeAsVariable - }); beforeEach(() { detector = new DirtyCheckingChangeDetector(getterFactory); }); - describe('StaticFieldGetterFactory', () { - DirtyCheckingChangeDetector detector; - var user = new _User('Marko', 'Vuksanovic', 30); - FieldGetterFactory getterFactory = new StaticFieldGetterFactory({ - "first": (o) => o.first, - "age": (o) => o.age, - "last": (o) => o.last, - "toString": (o) => o.toString, - "isUnderAge": (o) => o.isUnderAge, - "isUnderAgeAsVariable": (o) => o.isUnderAgeAsVariable, - "list": (o) => o.list, - "map": (o) => o.map - }); - - beforeEach(() { - detector = new DirtyCheckingChangeDetector(getterFactory); - }); - - it('should detect methods', () { - var obj = new _User(); - expect(getterFactory.isMethod(obj, 'toString')).toEqual(true); - expect(getterFactory.isMethod(obj, 'age')).toEqual(false); - }); - - it('should return true is method is real method', () { - expect(getterFactory.isMethod(user, 'isUnderAge')).toEqual(true); - }); - - it('should return false is field is a function', () { - expect(getterFactory.isMethod(user, 'isUnderAgeAsVariable')).toEqual(false); - }); - - it('should return false is field is a list', () { - expect(getterFactory.isMethod(user, 'list')).toEqual(false); - }); - - it('should return false is field is a map', () { - expect(getterFactory.isMethod(user, 'map')).toEqual(false); - }); - }); - - describe('Dynamic GetterFactory', () { - DirtyCheckingChangeDetector detector; - var user = new _User('Marko', 'Vuksanovic', 30); - FieldGetterFactory getterFactory = new DynamicFieldGetterFactory(); - - beforeEach(() { - detector = new DirtyCheckingChangeDetector(getterFactory); - }); - - it('should return true is method is real method', () { - expect(getterFactory.isMethod(user, 'isUnderAge')).toEqual(true); - }); - - it('should return false is field is a function', () { - expect(getterFactory.isMethod(user, 'isUnderAgeAsVariable')).toEqual(false); - }); - - it('should return false is field is a list', () { - expect(getterFactory.isMethod(user, 'list')).toEqual(false); - }); - - it('should return false is field is a map', () { - expect(getterFactory.isMethod(user, 'map')).toEqual(false); - }); - }); - describe('object field', () { it('should detect nothing', () { var changes = detector.collectChanges(); @@ -787,6 +712,21 @@ void main() { }); } + +void main() { + testWithGetterFactory(new DynamicFieldGetterFactory()); + + testWithGetterFactory(new StaticFieldGetterFactory({ + "first": (o) => o.first, + "age": (o) => o.age, + "last": (o) => o.last, + "toString": (o) => o.toString, + "isUnderAge": (o) => o.isUnderAge, + "isUnderAgeAsVariable": (o) => o.isUnderAgeAsVariable, + })); +} + + class _User { String first; String last; @@ -1022,7 +962,6 @@ class MapRecordMatcher extends _CollectionMatcher { } } - class FooBar { static int fooIds = 0; diff --git a/test/core/scope_spec.dart b/test/core/scope_spec.dart index 4113c7e26..fd685f227 100644 --- a/test/core/scope_spec.dart +++ b/test/core/scope_spec.dart @@ -1291,13 +1291,29 @@ void main() { it('should properly watch array of fields 3', (RootScope rootScope, Logger log) { rootScope.context['foo'] = 'abc'; - rootScope.watch('foo.contains("b")', (v, o) => log([v, o])); + String expression = 'foo.contains("b")'; + if (identical(1, 1.0)) { // dart2js + // The previous expression does not work in dart2js (dartbug 13523) + // Use a working one for now. + expression = 'foo.contains("b", 0)'; + } + rootScope.watch(expression, (v, o) => log([v, o])); expect(log).toEqual([]); rootScope.apply(); expect(log).toEqual([[true, null]]); log.clear(); }); + it('should watch closures both as a leaf and as method call', (RootScope rootScope, Logger log) { + rootScope.context['foo'] = new Foo(); + rootScope.context['increment'] = null; + rootScope.watch('foo.increment', (v, _) => rootScope.context['increment'] = v); + rootScope.watch('increment(1)', (v, o) => log([v, o])); + expect(log).toEqual([]); + rootScope.apply(); + expect(log).toEqual([[null, null], [2, null]]); + log.clear(); + }); it('should not trigger new watcher in the flush where it was added', (Scope scope) { var log = [] ; @@ -1715,3 +1731,6 @@ class UnstableList { List get list => new List.generate(3, (i) => i); } +class Foo { + increment(x) => x+1; +}