Refactor of PolymerExpressions. Adds "as" expressions.

pkg/polymer_expressions/lib/polymer_expressions.dart

Index: pkg/polymer_expressions/lib/polymer_expressions.dart
diff --git a/pkg/polymer_expressions/lib/polymer_expressions.dart b/pkg/polymer_expressions/lib/polymer_expressions.dart
index 0d4d06192fe05ad9292a8e0f768e7f5b427f2d3e..513a38344075ae174cad452948f960a2dce47e65 100644
--- a/pkg/polymer_expressions/lib/polymer_expressions.dart
+++ b/pkg/polymer_expressions/lib/polymer_expressions.dart
@@ -29,6 +29,7 @@ library polymer_expressions;
 
 import 'dart:async';
 import 'dart:html';
+import 'dart:collection' show HashMap;
 
 import 'package:logging/logging.dart';
 import 'package:observe/observe.dart';
@@ -41,17 +42,6 @@ import 'src/globals.dart';
 
 final Logger _logger = new Logger('polymer_expressions');
 
-// TODO(justin): Investigate XSS protection
-Object _classAttributeConverter(v) =>
-    (v is Map) ? v.keys.where((k) => v[k] == true).join(' ') :
-    (v is Iterable) ? v.join(' ') :
-    v;
-
-Object _styleAttributeConverter(v) =>
-    (v is Map) ? v.keys.map((k) => '$k: ${v[k]}').join(';') :
-    (v is Iterable) ? v.join(';') :
-    v;
-
 class PolymerExpressions extends BindingDelegate {
   /** The default [globals] to use for Polymer expressions. */
   static const Map DEFAULT_GLOBALS = const { 'enumerate': enumerate };
@@ -67,73 +57,75 @@ class PolymerExpressions extends BindingDelegate {
       : globals = (globals == null) ?
           new Map<String, Object>.from(DEFAULT_GLOBALS) : globals;
 
-  prepareBinding(String path, name, node) {
+  prepareBinding(String path, name, boundNode) {
     if (path == null) return null;
     var expr = new Parser(path).parse();
 
-    // For template bind/repeat to an empty path, just pass through the model.
-    // We don't want to unwrap the Scope.
-    // TODO(jmesserly): a custom element extending <template> could notice this
-    // behavior. An alternative is to associate the Scope with the node via an
-    // Expando, which is what the JavaScript PolymerExpressions does.
-    if (isSemanticTemplate(node) && (name == 'bind' || name == 'repeat') &&
-        expr is EmptyExpression) {
-      return null;
+    if (isSemanticTemplate(boundNode)) {
+      // <template bind> expressions don't pass through prepareInstanceModel

[Jennifer Messerly, 2014/01/31 02:48:50]

interesting. I wonder if it is intended, or happens to be a side effect of
having templates not go through node.bind anymore

[justinfagnani, 2014/01/31 21:02:07]

not sure, but I'm finding that I have to do this same logic in the rest of the
binding closures, even in cases where there's no binding on <template>, like
with a simple custom element. This seems really counter to my understanding of
the purpose of prepareInstanceModel.

[justinfagnani, 2014/03/12 23:21:30]

What's really happening here is that if you set the model programmatically, the
order of prepareInstanceModel and prepareBinding are reversed. I have to talk to
Raf about this, but it doesn't seem like great behavior. I worked around it
though.

+      // first so we have to ensure the model is wrapped in a Scope.
+      // prepareInstanceModel
+      var wrapModel = prepareInstanceModel(boundNode);
+      if (name == 'bind') {
+        if (expr is AsExpression) {
+          var identifier = expr.right.value;
+          var bindExpr = expr.left;
+          return (model, node) {
+            var scope = wrapModel(model);

[Jennifer Messerly, 2014/01/31 02:48:50]

silly comment: i'd probably just fold this into the next line instead of having
a local... same for the other case below

[justinfagnani, 2014/03/12 23:21:30]

obsolete

+            return new _AsBinding(bindExpr, identifier, scope);
+          };
+        }
+        return (model, node) {
+          var scope = wrapModel(model);
+          return new _Binding(expr, scope);
+        };
+      } else if (name == 'repeat' && expr is InExpression) {
+        var identifier = expr.left.value;
+        var iterableExpr = expr.right;
+        return (scope, node) => new _InBinding(iterableExpr, identifier, scope);
+      }
     }
 
-    return (model, node) {
-      if (model is! Scope) {
-        model = new Scope(model: model, variables: globals);
-      }
-      if (node is Element && name == "class") {
-        return new _Binding(expr, model, _classAttributeConverter);
-      }
-      if (node is Element && name == "style") {
-        return new _Binding(expr, model, _styleAttributeConverter);
-      }
-      return new _Binding(expr, model);
-    };
+    if ((boundNode is Element && name == 'class')) {

[Jennifer Messerly, 2014/01/31 02:48:50]

nit: extra parens

[justinfagnani, 2014/03/12 23:21:30]

Done.

+      return (scope, node) => new _ClassBinding(expr, scope);
+    } else if ((boundNode is Element && name == 'style')) {

[Jennifer Messerly, 2014/01/31 02:48:50]

here too

[justinfagnani, 2014/03/12 23:21:30]

Done.

+      return (scope, node) => new _StyleBinding(expr, scope);
+    } else {
+      return (scope, node) => new _Binding(expr, scope);
+    }
   }
 
-  prepareInstanceModel(Element template) => (model) =>
-      model is Scope ? model : new Scope(model: model, variables: globals);
+  prepareInstanceModel(Element template) {
+    var templateInstance = templateBind(template).templateInstance;

[Jennifer Messerly, 2014/01/31 02:48:50]

maybe a shorter variable name here?

[justinfagnani, 2014/03/12 23:21:30]

obsolete

+    Scope parentScope = (templateInstance != null) ? templateInstance.model : null;

[Jennifer Messerly, 2014/01/31 02:48:50]

long line

[justinfagnani, 2014/03/12 23:21:30]

Done.

+    var variables = (parentScope == null) ? globals : null;
+    return (model) {
+      return model is Scope ? model : new Scope(parent: parentScope, model: model, variables: variables);

[Jennifer Messerly, 2014/01/31 02:48:50]

long line

[justinfagnani, 2014/03/12 23:21:30]

Done.

+    };
+  }
 }
 
 class _Binding extends ChangeNotifier {
   final Scope _scope;
   final ExpressionObserver _expr;
-  final _converter;
   var _value;
 
-  _Binding(Expression expr, Scope scope, [this._converter])
+  _Binding(Expression expr, Scope scope)
       : _expr = observe(expr, scope),
         _scope = scope {
-    _expr.onUpdate.listen(_setValue).onError((e) {
+    _expr.onUpdate.listen(_updateValue).onError((e) {
       _logger.warning("Error evaluating expression '$_expr': ${e.message}");
     });
     try {
       update(_expr, _scope);
-      _setValue(_expr.currentValue);
+      _updateValue(_expr.currentValue);
     } on EvalException catch (e) {
       _logger.warning("Error evaluating expression '$_expr': ${e.message}");
     }
   }
 
-  _setValue(v) {
-    var oldValue = _value;
-    if (v is Comprehension) {
-      // convert the Comprehension into a list of scopes with the loop
-      // variable added to the scope
-      _value = v.iterable.map((i) {
-        var vars = new Map();
-        vars[v.identifier] = i;
-        Scope childScope = new Scope(parent: _scope, variables: vars);
-        return childScope;
-      }).toList(growable: false);
-    } else {
-      _value = (_converter == null) ? v : _converter(v);
-    }
-    notifyPropertyChange(#value, oldValue, _value);
+  _updateValue(newValue) {
+    _value = notifyPropertyChange(#value, _value, newValue);
   }
 
   @reflectable get value => _value;
@@ -146,3 +138,84 @@ class _Binding extends ChangeNotifier {
     }
   }
 }
+
+class _ClassBinding extends _Binding {

[Jennifer Messerly, 2014/01/31 02:48:50]

hmmm. these will all change a bit after
https://codereview.chromium.org/132403010/ ... maybe not too bad tho

[justinfagnani, 2014/03/12 23:21:30]

I ended up not needing AsBinding and the static oneTime method wasn't so nice to
handle this way, so I went back to converters.

+  _ClassBinding(Expression expr, Scope scope) : super(expr, scope);
+
+  _updateValue(v) {
+    // TODO(justinfagnani): Investigate XSS protection
+    // TODO(justinfagnani): observe collection changes
+    var newValue =
+        (v is Map) ? v.keys.where((k) => v[k] == true).join(' ') :
+        (v is Iterable) ? v.join(' ') :
+        v;
+    _value = notifyPropertyChange(#value, _value, newValue);
+  }
+}
+
+class _StyleBinding extends _Binding {
+  _StyleBinding(Expression expr, Scope scope) : super(expr, scope);
+
+  _updateValue(v) {
+    // TODO(justinfagnani): observe collection changes
+    var newValue =
+        (v is Map) ? v.keys.map((k) => '$k: ${v[k]}').join(';') :
+        (v is Iterable) ? v.join(';') :
+        v;
+    _value = notifyPropertyChange(#value, _value, newValue);
+  }
+}
+
+class _AsBinding extends _Binding {
+  final String _identifier;
+
+  _AsBinding(Expression expr, this._identifier, Scope scope)
+      : super(expr, scope);
+
+  _updateValue(v) {
+    // what does "this" in template bind="..as.." eval to?
+    _value = new Scope(parent: _scope, variables: {_identifier: v});
+  }
+}
+
+/*
+ * A binding for a repeat="a in b" expression. In addition to the right-side of
+ * the expression, we store the indentifier that should be introduced in child
+ * scopes, and we observe observable lists, modifying the list of child scopes
+ * according to the input data's changes.
+ */
+class _InBinding extends _Binding {
+  final String _identifier;
+  StreamSubscription _subscription;
+  ObservableList<Scope> _scopes;
+
+  _InBinding(Expression expr, this._identifier, Scope scope)
+      : super(expr, scope);
+
+  _updateValue(v) {
+    assert(v is Iterable || v == null);
+
+    _makeScope(value) =>
+        new Scope(parent: _scope, variables: {_identifier: value});
+
+    if (_subscription != null) {
+      _subscription.cancel();
+      _subscription = null;
+    }
+
+    if (v is ObservableList) {
+      _subscription = v.listChanges.listen((List<ListChangeRecord> changes) {
+        for (var change in changes) {
+          var start = change.index;
+          var length = change.removed.length;
+          var newItems = change.object.sublist(start, start + length);
+          var newScopes = newItems.map(_makeScope);
+          _scopes.replaceRange(change.index, change.removed.length, newScopes);
+        }
+      });
+    }
+
+    var newValue = v == null ? null : new ObservableList.from(v.map(_makeScope));

[Jennifer Messerly, 2014/01/31 02:48:50]

long line.

also, could this be just:

var newValue = v == null ? null : 
   new List.from(v.map(_makeScope), growable: false);

unless you plan to edit "newValue" it doesn't need to be ObservableList...

[justinfagnani, 2014/03/12 23:21:30]

obsolete

+    _value = notifyPropertyChange(#value, _value, newValue);
+  }
+}