Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

Commit 03f0a4c

Browse files
mheverymhevery
committed
fix(chenge-detection): properly watch map['key'] constructs
Fixes #824
1 parent a6cc826 commit 03f0a4c

File tree

5 files changed

+111
-23
lines changed

5 files changed

+111
-23
lines changed

‎lib/change_detection/ast.dart

+21-1
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,27 @@ class PureFunctionAST extends AST {
8484
super('$name(${_argList(argsAST)})');
8585

8686
WatchRecord<_Handler> setupWatch(WatchGroup watchGroup) =>
87-
watchGroup.addFunctionWatch(fn, argsAST, const {}, expression);
87+
watchGroup.addFunctionWatch(fn, argsAST, const {}, expression, true);
88+
}
89+
90+
/**
91+
* SYNTAX: fn(arg0, arg1, ...)
92+
*
93+
* Invoke a pure function. Pure means that the function has no state, and
94+
* therefore it needs to be re-computed only if its args change.
95+
*/
96+
class ClosureAST extends AST {
97+
final String name;
98+
final /* dartbug.com/16401 Function */ fn;
99+
final List<AST> argsAST;
100+
101+
ClosureAST(name, this.fn, argsAST)
102+
: argsAST = argsAST,
103+
name = name,
104+
super('$name(${_argList(argsAST)})');
105+
106+
WatchRecord<_Handler> setupWatch(WatchGroup watchGroup) =>
107+
watchGroup.addFunctionWatch(fn, argsAST, const {}, expression, false);
88108
}
89109

90110
/**

‎lib/change_detection/watch_group.dart

+30-21
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ typedef void ChangeLog(String expression, current, previous);
1111

1212
/**
1313
* Extend this class if you wish to pretend to be a function, but you don't know
14-
* number of arguments with which the function will get called.
14+
* number of arguments with which the function will get called with.
1515
*/
1616
abstract class FunctionApply {
1717
// dartbug.com/16401
@@ -183,11 +183,13 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {
183183
* - [fn] function to evaluate.
184184
* - [argsAST] list of [AST]es which represent arguments passed to function.
185185
* - [expression] normalized expression used for caching.
186+
* - [isPure] A pure function is one which holds no internal state. This implies that the
187+
* function is idempotent.
186188
*/
187189
_EvalWatchRecord addFunctionWatch(/* dartbug.com/16401 Function */ fn, List<AST> argsAST,
188190
Map<Symbol, AST> namedArgsAST,
189-
String expression) =>
190-
_addEvalWatch(null, fn, null, argsAST, namedArgsAST, expression);
191+
String expression, bool isPure) =>
192+
_addEvalWatch(null, fn, null, argsAST, namedArgsAST, expression, isPure);
191193

192194
/**
193195
* Watch a method [name]ed represented by an [expression].
@@ -200,18 +202,18 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {
200202
_EvalWatchRecord addMethodWatch(AST lhs, String name, List<AST> argsAST,
201203
Map<Symbol, AST> namedArgsAST,
202204
String expression) =>
203-
_addEvalWatch(lhs, null, name, argsAST, namedArgsAST, expression);
205+
_addEvalWatch(lhs, null, name, argsAST, namedArgsAST, expression, false);
204206

205207

206208

207209
_EvalWatchRecord _addEvalWatch(AST lhsAST, /* dartbug.com/16401 Function */ fn, String name,
208210
List<AST> argsAST,
209211
Map<Symbol, AST> namedArgsAST,
210-
String expression) {
212+
String expression, bool isPure) {
211213
_InvokeHandler invokeHandler = new _InvokeHandler(this, expression);
212214
var evalWatchRecord = new _EvalWatchRecord(
213215
_rootGroup._fieldGetterFactory, this, invokeHandler, fn, name,
214-
argsAST.length);
216+
argsAST.length, isPure);
215217
invokeHandler.watchRecord = evalWatchRecord;
216218

217219
if (lhsAST != null) {
@@ -701,15 +703,17 @@ class _InvokeHandler extends _Handler implements _ArgHandlerList {
701703

702704

703705
class _EvalWatchRecord implements WatchRecord<_Handler>, Record<_Handler> {
704-
static const int _MODE_DELETED_ = -1;
705-
static const int _MODE_MARKER_ = 0;
706-
static const int _MODE_FUNCTION_ = 1;
707-
static const int _MODE_FUNCTION_APPLY_ = 2;
708-
static const int _MODE_NULL_ = 3;
709-
static const int _MODE_FIELD_CLOSURE_ = 4;
710-
static const int _MODE_MAP_CLOSURE_ = 5;
711-
static const int _MODE_METHOD_ = 6;
712-
static const int _MODE_METHOD_INVOKE_ = 7;
706+
static const int _MODE_INVALID_ = -2;
707+
static const int _MODE_DELETED_ = -1;
708+
static const int _MODE_MARKER_ = 0;
709+
static const int _MODE_PURE_FUNCTION_ = 1;
710+
static const int _MODE_FUNCTION_ = 2;
711+
static const int _MODE_PURE_FUNCTION_APPLY_ = 3;
712+
static const int _MODE_NULL_ = 4;
713+
static const int _MODE_FIELD_CLOSURE_ = 5;
714+
static const int _MODE_MAP_CLOSURE_ = 6;
715+
static const int _MODE_METHOD_ = 7;
716+
static const int _MODE_METHOD_INVOKE_ = 8;
713717
WatchGroup watchGrp;
714718
final _Handler handler;
715719
final List args;
@@ -724,13 +728,13 @@ class _EvalWatchRecord implements WatchRecord<_Handler>, Record<_Handler> {
724728
_EvalWatchRecord _prevEvalWatch, _nextEvalWatch;
725729

726730
_EvalWatchRecord(this._fieldGetterFactory, this.watchGrp, this.handler,
727-
this.fn, this.name, int arity)
731+
this.fn, this.name, int arity, bool pure)
728732
: args = new List(arity)
729733
{
730734
if (fn is FunctionApply) {
731-
mode = _MODE_FUNCTION_APPLY_;
735+
mode = pure ? _MODE_PURE_FUNCTION_APPLY_: _MODE_INVALID_;
732736
} else if (fn is Function) {
733-
mode = _MODE_FUNCTION_;
737+
mode = pure ? _MODE_PURE_FUNCTION_ : _MODE_FUNCTION_;
734738
} else {
735739
mode = _MODE_NULL_;
736740
}
@@ -763,7 +767,8 @@ class _EvalWatchRecord implements WatchRecord<_Handler>, Record<_Handler> {
763767
assert(mode != _MODE_DELETED_);
764768
assert(mode != _MODE_MARKER_);
765769
assert(mode != _MODE_FUNCTION_);
766-
assert(mode != _MODE_FUNCTION_APPLY_);
770+
assert(mode != _MODE_PURE_FUNCTION_);
771+
assert(mode != _MODE_PURE_FUNCTION_APPLY_);
767772
_object = value;
768773

769774
if (value == null) {
@@ -789,12 +794,16 @@ class _EvalWatchRecord implements WatchRecord<_Handler>, Record<_Handler> {
789794
case _MODE_MARKER_:
790795
case _MODE_NULL_:
791796
return false;
792-
case _MODE_FUNCTION_:
797+
case _MODE_PURE_FUNCTION_:
793798
if (!dirtyArgs) return false;
794799
value = Function.apply(fn, args, namedArgs);
795800
dirtyArgs = false;
796801
break;
797-
case _MODE_FUNCTION_APPLY_:
802+
case _MODE_FUNCTION_:
803+
value = Function.apply(fn, args, namedArgs);
804+
dirtyArgs = false;
805+
break;
806+
case _MODE_PURE_FUNCTION_APPLY_:
798807
if (!dirtyArgs) return false;
799808
value = (fn as FunctionApply).apply(args);
800809
dirtyArgs = false;

‎lib/core/scope.dart

+1-1
Original file line numberDiff line numberDiff line change
@@ -1047,7 +1047,7 @@ class ExpressionVisitor implements Visitor {
10471047
visit(exp.no)]);
10481048
}
10491049
void visitAccessKeyed(AccessKeyed exp) {
1050-
ast = new PureFunctionAST('[]', _operation_bracket,
1050+
ast = new ClosureAST('[]', _operation_bracket,
10511051
[visit(exp.object), visit(exp.key)]);
10521052
}
10531053
void visitLiteralPrimitive(LiteralPrimitive exp) {

‎test/change_detection/watch_group_spec.dart

+54
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,60 @@ void main() {
353353
});
354354

355355

356+
it('should eval closure', () {
357+
context['a'] = {'val': 1};
358+
context['b'] = {'val': 2};
359+
var innerState = 1;
360+
361+
var watch = watchGrp.watch(
362+
new ClosureAST('sum',
363+
(a, b) { logger('+'); return innerState+a+b; },
364+
[parse('a.val'), parse('b.val')]
365+
),
366+
(v, p) => logger(v)
367+
);
368+
369+
// a; a.val; b; b.val;
370+
expect(watchGrp.fieldCost).toEqual(4);
371+
// add
372+
expect(watchGrp.evalCost).toEqual(1);
373+
374+
watchGrp.detectChanges();
375+
expect(logger).toEqual(['+', 4]);
376+
377+
// extra checks should trigger closures
378+
watchGrp.detectChanges();
379+
watchGrp.detectChanges();
380+
expect(logger).toEqual(['+', 4, '+', '+']);
381+
logger.clear();
382+
383+
// multiple arg changes should only trigger function once.
384+
context['a']['val'] = 3;
385+
context['b']['val'] = 4;
386+
387+
watchGrp.detectChanges();
388+
expect(logger).toEqual(['+', 8]);
389+
logger.clear();
390+
391+
// inner state change should only trigger function once.
392+
innerState = 2;
393+
394+
watchGrp.detectChanges();
395+
expect(logger).toEqual(['+', 9]);
396+
logger.clear();
397+
398+
watch.remove();
399+
expect(watchGrp.fieldCost).toEqual(0);
400+
expect(watchGrp.evalCost).toEqual(0);
401+
402+
context['a']['val'] = 0;
403+
context['b']['val'] = 0;
404+
405+
watchGrp.detectChanges();
406+
expect(logger).toEqual([]);
407+
});
408+
409+
356410
it('should eval chained pure function', () {
357411
context['a'] = {'val': 1};
358412
context['b'] = {'val': 2};

‎test/core/scope_spec.dart

+5
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,11 @@ void main() {
150150
logger.clear();
151151
rootScope.digest();
152152
expect(logger).toEqual([]);
153+
logger.clear();
154+
155+
context['a']['b'] = 234;
156+
rootScope.digest();
157+
expect(logger).toEqual([234]);
153158
});
154159

155160

0 commit comments

Comments
 (0)