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 f26c2f3e7e58a91655ac7981b07036be170902de..6f86e3839ad6dc35bf797ecfac69e2269485fe98 100644
--- a/pkg/polymer_expressions/lib/polymer_expressions.dart
+++ b/pkg/polymer_expressions/lib/polymer_expressions.dart
@@ -38,7 +38,6 @@ import 'expression.dart';
 import 'parser.dart';
 import 'src/globals.dart';
 
-// TODO(justin): Investigate XSS protection
 Object _classAttributeConverter(v) =>
     (v is Map) ? v.keys.where((k) => v[k] == true).join(' ') :
     (v is Iterable) ? v.join(' ') :
@@ -53,68 +52,228 @@ class PolymerExpressions extends BindingDelegate {
   /** The default [globals] to use for Polymer expressions. */
   static const Map DEFAULT_GLOBALS = const { 'enumerate': enumerate };
 
+  final ScopeFactory _scopeFactory;
   final Map<String, Object> globals;
 
+  // allows access to scopes created for template instances
+  final Expando<Scope> _scopes = new Expando<Scope>();
+  // allows access to scope identifiers (for "in" and "as")
+  final Expando<String> _scopeIdents = new Expando<String>();
+
   /**
    * Creates a new binding delegate for Polymer expressions, with the provided
    * variables used as [globals]. If no globals are supplied, a copy of the
    * [DEFAULT_GLOBALS] will be used.
    */
-  PolymerExpressions({Map<String, Object> globals})
+  PolymerExpressions({Map<String, Object> globals,
+      ScopeFactory scopeFactory: const ScopeFactory()})
       : globals = globals == null ?
-          new Map<String, Object>.from(DEFAULT_GLOBALS) : globals;
+          new Map<String, Object>.from(DEFAULT_GLOBALS) : globals,
+          _scopeFactory = scopeFactory;
 
-  prepareBinding(String path, name, node) {
+  @override
+  PrepareBindingFunction prepareBinding(String path, name, Node 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) && (name == 'bind' || name == 'repeat')) {
+      if (expr is HasIdentifier) {
+        var identifier = expr.identifier;
+        var bindExpr = expr.expr;
+        return (model, Node node, bool oneTime) {
+          _scopeIdents[node] = identifier;
+          // model may not be a Scope if it was assigned directly via
+          // template.model = x; In that case, prepareInstanceModel will
+          // be called _after_ prepareBinding and will lookup this scope from
+          // _scopes
+          var scope = _scopes[node] = (model is Scope)

[Jennifer Messerly, 2014/04/24 00:51:34]

this might be more clear if spread out over a few lines:

var scope;
if (model is Scope) {
  scope = model;
} else {
  scope = _scopeFactory.modelScope(model: model, variables: globals);
}
_scopes[node] = scope;


alternatively, maybe pull into a function since the same code is used here &
below.

[justinfagnani, 2014/05/28 00:29:37]

did both

+              ? model
+              : _scopeFactory.modelScope(model: model, variables: globals);
+          return new _Binding(bindExpr, scope);
+        };
+      } else {
+        return (model, Node node, bool oneTime) {
+          var scope = _scopes[node] = (model is Scope)
+              ? model
+              : _scopeFactory.modelScope(model: model, variables: globals);
+          if (oneTime) {
+            return _Binding._oneTime(expr, scope, null);
+          }
+          return new _Binding(expr, scope);
+        };
+      }
+    }
+
+    // For regular bindings, not bindings on a template, the model is always
+    // a Scope created by prepareInstanceModel
+    _Converter converter = null;
+    if (boundNode is Element && name == 'class') {
+      converter = _classAttributeConverter;
+    } else if (boundNode is Element && name == 'style') {
+      converter = _styleAttributeConverter;
     }
 
-    return (model, node, oneTime) {
-      if (model is! Scope) {
-        model = new Scope(model: model, variables: globals);
+    return (model, Node node, bool oneTime) {
+      var scope = _getScopeForModel(node, model);
+      if (oneTime) {
+        return _Binding._oneTime(expr, scope, converter);
       }
-      var converter = null;
-      if (node is Element && name == "class") {
-        converter = _classAttributeConverter;
+      return new _Binding(expr, scope, converter);
+    };
+  }
+
+  prepareInstanceModel(Element template) {

[Jennifer Messerly, 2014/04/24 00:51:34]

@override here? I noticed you use it above

[justinfagnani, 2014/05/28 00:29:37]

Done.

+    var ident = _scopeIdents[template];
+
+    if (ident == null) {
+      return (model) {
+        var existingScope = _scopes[template];
+        // TODO (justinfagnani): make template binding always call
+        // prepareInstanceModel first and get rid of this check
+        if (existingScope != null) {
+          // If there's an existing scope, we created it in prepareBinding
+          // If it has the same model, then we can reuse it, otherwise it's
+          // a repeat with no identifier and we create new scope to occlude
+          // the outer one
+          if (model == existingScope.model) return existingScope;
+          return _scopeFactory.modelScope(model: model, variables: globals);
+        } else {
+          return _getScopeForModel(template, model);
+        }
+      };
+    }
+
+    // We have an ident, so it's a bind/as or repeat/in expression
+    assert(templateBind(template).templateInstance == null);
+    return (model) {
+      var existingScope = _scopes[template];

[Jennifer Messerly, 2014/04/24 00:51:34]

if it were me, I'd refactor everything after this into:

var scope = _scopes[template];
// If there's not an existing scope then we have a bind/as or
// repeat/in binding enclosed in an outer scope, so we use that as
// the parent
if (scope == null) scope = _getParentScope(template);
return _scopeFactory.childScope(scope, ident, model);

essentially fold the _scopeFactory.childScope calls

[justinfagnani, 2014/05/28 00:29:37]

Done.

+      if (existingScope != null) {
+        // This only happens when a model has been assigned programatically
+        // and prepareBinding is called _before_ prepareInstanceModel.
+        // The scope assigned in prepareBinding wraps the model and is the
+        // scope of the expression. That should be the parent of the templates
+        // scope in the case of bind/as or repeat/in bindings.
+        return _scopeFactory.childScope(existingScope, ident, model);
+      } else {
+        // If there's not an existing scope then we have a bind/as or
+        // repeat/in binding enclosed in an outer scope, so we use that as
+        // the parent
+        var parentScope = _getParentScope(template);
+        return _scopeFactory.childScope(parentScope, ident, model);
       }
-      if (node is Element && name == "style") {
-        converter = _styleAttributeConverter;
+    };
+  }
+
+  /**
+   * Gets an existing scope for use as a parent, but does not create a new one.
+   */
+  Scope _getParentScope(Node node) {
+    var parent = node.parentNode;
+    if (parent == null) return null;
+
+    if (isSemanticTemplate(node)) {
+      var templateExtension = templateBind(node);
+      var templateInstance = templateExtension.templateInstance;
+      var model = templateInstance == null
+          ? templateExtension.model
+          : templateInstance.model;
+      if (model is Scope) {
+        return model;
+      } else {
+        // A template with a bind binding might have a non-Scope model
+        return _scopes[node];
       }
+    }
+    if (parent != null) return _getParentScope(parent);
+    return null;
+  }
 
-      if (oneTime) {
-        return _Binding._oneTime(expr, model, converter);
+  /**
+   * Returns the Scope to be used to evaluate expressions in the template
+   * containing [node]. Since all expressions in the same template evaluate
+   * against the same model, [model] is passed in and checked against the
+   * template model to make sure they agree.
+   *
+   * For nested templates, we might have a binding on the nested template that
+   * should be evaluated in the context of the parent template. All scopes are
+   * retreived from an ancestor of [node], since node may be establishing a new
+   * Scope.
+   */
+  Scope _getScopeForModel(Node node, model) {
+    // This only happens in bindings_test because it calls prepareBinding()
+    // directly. Fix the test and throw if node is null?
+    if (node == null) {
+      return _scopeFactory.modelScope(model: model, variables: globals);
+    }
+
+    var id = node is Element ? node.id : '';
+    if (model is Scope) {
+      return model;
+    }
+    if (_scopes[node] != null) {
+      var scope = _scopes[node];
+      assert(scope.model == model);
+      return _scopes[node];
+    } else if (node.parentNode != null) {
+      return _getContainingScope(node.parentNode, model);
+    } else {
+      // here we should be at a top-level template, so there's no parent to
+      // look for a Scope on.
+      if (!isSemanticTemplate(node)) {
+        throw "expected a template instead of $node";
       }
+      return _getContainingScope(node, model);
+    }
+  }
 
-      return new _Binding(expr, model, converter);
-    };
+  Scope _getContainingScope(Node node, model) {
+    if (isSemanticTemplate(node)) {
+      var templateExtension = templateBind(node);

[Jennifer Messerly, 2014/04/24 00:51:34]

this part is very similar to code in _getParentScope, would be great to factor
into a method

[justinfagnani, 2014/05/28 00:29:37]

Done.

+      var templateInstance = templateExtension.templateInstance;
+      var templateModel = templateInstance == null
+          ? templateExtension.model
+          : templateInstance.model;
+      assert(templateModel == model);
+      var scope = _scopes[node];
+      assert(scope != null);
+      assert(scope.model == model);
+      return scope;
+    } else if (node.parent == null) {
+      var scope = _scopes[node];
+      if (scope != null) {
+        assert(scope.model == model);
+      } else {
+        // only happens in bindings_test
+        scope = _scopeFactory.modelScope(model: model, variables: globals);
+      }
+      return scope;
+    } else {
+      return _getContainingScope(node.parentNode, model);
+    }
   }
 
-  prepareInstanceModel(Element template) => (model) =>
-      model is Scope ? model : new Scope(model: model, variables: globals);
 }
 
+typedef Object _Converter(Object);
+
 class _Binding extends Bindable {
+  static int __seq = 1;
+
+  final int _seq = __seq++;
+
   final Scope _scope;
-  final _converter;
-  Expression _expr;
+  final _Converter _converter;
+  final Expression _expr;
   Function _callback;
   StreamSubscription _sub;
   var _value;
 
   _Binding(this._expr, this._scope, [this._converter]);
 
-  static _oneTime(Expression expr, Scope scope, [converter]) {
+  static Object _oneTime(Expression expr, Scope scope, _Converter converter) {
     try {
-      return _convertValue(eval(expr, scope), scope, converter);
+      var value = eval(expr, scope);
+      return (converter == null) ? value : converter(value);
     } catch (e, s) {
       new Completer().completeError(
           "Error evaluating expression '$expr': $e", s);
@@ -122,23 +281,14 @@ class _Binding extends Bindable {
     return null;
   }
 
-  _setValue(v) {
-    _value = _convertValue(v, _scope, _converter);
+  _updateValue(v) {
+    _value = (_converter == null) ? v : _converter(v);
     if (_callback != null) _callback(_value);
   }
 
-  static _convertValue(v, scope, converter) {
-    if (v is Comprehension) {
-      // convert the Comprehension into a list of scopes with the loop
-      // variable added to the scope
-      return v.iterable.map((i) => scope.childScope(v.identifier, i))
-          .toList(growable: false);
-    } else {
-      return converter == null ? v : converter(v);
-    }
-  }
-
   get value {
+    // if there's a callback, then _value has been set, if not we need to
+    // force an evaluation
     if (_callback != null) return _value;
     return _oneTime(_expr, _scope, _converter);
   }
@@ -152,19 +302,17 @@ class _Binding extends Bindable {
     }
   }
 
-  open(callback(value)) {
+  Object open(callback(value)) {

[Jennifer Messerly, 2014/04/24 00:51:34]

just curious, why Object here?

[justinfagnani, 2014/05/28 00:29:37]

Because it does return something. I'll change it to dynamic. I know it's against
the style guide, but it make the editor warn if you don't return something.

     if (_callback != null) throw new StateError('already open');
 
     _callback = callback;
     final expr = observe(_expr, _scope);
-    _expr = expr;
-    _sub = expr.onUpdate.listen(_setValue)..onError((e, s) {
+    _sub = expr.onUpdate.listen(_updateValue)..onError((e, s) {
       new Completer().completeError(
           "Error evaluating expression '$expr': $e", s);
     });
     try {
-      update(expr, _scope);
-      _value = _convertValue(expr.currentValue, _scope, _converter);
+      _value = update(expr, _scope);
     } catch (e, s) {
       new Completer().completeError(
           "Error evaluating expression '$expr': $e", s);
@@ -177,7 +325,18 @@ class _Binding extends Bindable {
 
     _sub.cancel();
     _sub = null;
-    _expr = (_expr as ExpressionObserver).expression;
     _callback = null;
   }
 }
+
+/**
+ * Factory function used for testing.
+ */
+class ScopeFactory {
+  const ScopeFactory();
+  modelScope({Object model, Map<String, Object> variables}) =>
+      new Scope(model: model, variables: variables);
+
+  childScope(Scope parent, String name, Object value) =>
+      parent.childScope(name, value);
+}