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 36ea3524adbc8d7acfc4d1e2e8598353872a4eb7..2c50e762190014898c313aebca6c63b2c83d4b24 100644
--- a/pkg/polymer_expressions/lib/polymer_expressions.dart
+++ b/pkg/polymer_expressions/lib/polymer_expressions.dart
@@ -41,109 +41,276 @@ 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;
+typedef Scope ScopeFactory({model, Map<String, Object> variables, Scope parent});

[Jennifer Messerly, 2014/03/17 23:10:08]

doh, slightly long line :)

[justinfagnani, 2014/04/24 00:26:13]

Done.

 
-Object _styleAttributeConverter(v) =>
-    (v is Map) ? v.keys.map((k) => '$k: ${v[k]}').join(';') :
-    (v is Iterable) ? v.join(';') :
-    v;
+Scope _createScope({model, Map<String, Object> variables, Scope parent}) =>
+    new Scope(model: model, variables: variables, parent: parent);
 
 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: _createScope})
       : 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)) {
+      if (name == 'bind') {
+        if (expr is AsExpression) {
+          // bind/as bindings add a new name to a child scope, but still
+          // allow access to names in the enclosing scope
+          var identifier = expr.right.value;
+          var bindExpr = expr.left;
+
+          return (model, Node node, bool oneTime) {
+            _scopeIdents[node] = identifier;
+            // model may not be a scope if it was assigned directly via
+            // template.model = ...; In that case, prepareInstanceModel will
+            // be called _after_ and will lookup the same scope
+            var scope = _scopes[node] = (model is Scope)
+                ? model
+                : _scopeFactory(model: model, parent: _getParentScope(node));
+            return new _Binding(bindExpr, scope);
+          };
+        } else {
+          // non-as bindings completely occlude the enclosing scope
+          return (model, Node node, bool oneTime) {
+            // even though we assign the scope to _scopes we're going to
+            // immediately override in it prepareInstanceModel with a new scope with the result of

[Jennifer Messerly, 2014/03/17 23:10:08]

couple of long lines in this method

[justinfagnani, 2014/04/24 00:26:13]

Done.

+            // expr.
+            // TODO: This scope should probably have a parent
+            var scope = _scopes[node] = (model is Scope)
+                ? model
+                : _scopeFactory(model: model, variables: globals);
+            return new _Binding(expr, scope);
+          };
+        }
+      } else if (name == 'repeat' && expr is InExpression) {
+        var identifier = expr.left.value;
+        var iterableExpr = expr.right;
+        return (model, Node node, bool oneTime) {
+          _scopeIdents[node] = identifier;
+          return new _InBinding(iterableExpr, identifier, _getScopeForModel(node, model));
+        };
+      }
     }
 
-    return (model, node, oneTime) {
-      if (model is! Scope) {
-        model = new Scope(model: model, variables: globals);
+    // 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 = (v) =>
+          (v is Map) ? v.keys.where((k) => v[k] == true).join(' ') :

[Jennifer Messerly, 2014/03/17 23:10:08]

this is identical to code in _ClassBinding belwo (and similar issue with
_StyleBinding), can the shared code be extracted into a method?

[justinfagnani, 2014/04/24 00:26:13]

Done.

+          (v is Iterable) ? v.join(' ') :
+          v;
+    } else if (boundNode is Element && name == 'style') {
+      converter = (v) =>
+          (v is Map) ? v.keys.map((k) => '$k: ${v[k]}').join(';') :
+          (v is Iterable) ? v.join(';') :
+          v;
+    }
+    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) {
+    var ident = _scopeIdents[template];
+
+    if (ident == null) return (model) {
+      var existingScope = _scopes[template];
+      if (existingScope != null) {
+        // if there's an existing scope, we created it in prepareBinding
+        // and should be occluding containing scopes
+        return _scopes[template] =
+            _scopeFactory(model: model, variables: globals);
+      } else {
+        return _getScopeForModel(template, model);
       }
-      if (node is Element && name == "style") {
-        converter = _styleAttributeConverter;
+    };
+
+    // bind/as or repeat/in cases:
+    var templateExtension = templateBind(template);
+    TemplateInstance templateInstance = templateExtension.templateInstance;
+    if (templateInstance == null) {
+      return (model) {
+        var scope = _scopes[template];
+        if (scope != 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, and we replace
+          // it in _scopes so that bindings in the template can use the new
+          // scope.
+          return _scopes[template] = _scopeFactory(model: model, parent: scope,
+              variables: {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 _scopes[template] = _scopeFactory(model: model,
+              parent: parentScope, variables: {ident: model});
+        }
+      };
+    } else {
+      assert(false);
+      String scopeIdent = _scopeIdents[template];
+      // A template's TemplateTnstance's model is its parent's scope
+      Scope parentScope = templateInstance.model;
+      return (model) {
+        return _scopes[template] = _scopeFactory(parent: parentScope,
+            variables: {scopeIdent: model});
+      };
+    }
+  }
+
+  /**
+   * 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;
+    var id = node is Element ? node.id : '';
+    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) {
+    // only in tests? throw?
+    if (node == null) {
+      return _scopeFactory(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);
+      var templateInstance = templateExtension.templateInstance;
+      var templateModel = templateInstance == null
+          ? templateExtension.model
+          : templateInstance.model;
+      assert(templateModel == model);
+      var scope = _scopes[node];
+      if (scope != null) {

[Jennifer Messerly, 2014/03/17 23:10:08]

there's some repetition here & below, could it be factored out?

[justinfagnani, 2014/04/24 00:26:13]

Done.

+        assert(scope.model == model);
+      } else {
+        scope = _scopes[node] = _scopeFactory(model: model, variables: globals);
+      }
+      return scope;
+    } else if (node.parent == null) {
+      var scope = _scopes[node];
+      if (scope != null) {
+        assert(scope.model == model);
+      } else {
+        scope = _scopes[node] = _scopeFactory(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);
     } on EvalException catch (e) {
       _logger.warning("Error evaluating expression '$expr': ${e.message}");
       return null;
     }
   }
 
-  _setValue(v) {
-    _value = _convertValue(v, _scope, _converter);
+  _updateValue(v) {
+    _value = 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) {
-        var vars = new Map();
-        vars[v.identifier] = i;
-        Scope childScope = new Scope(parent: scope, variables: vars);
-        return childScope;
-      }).toList(growable: false);
-    } else {
-      return converter == null ? v : converter(v);
-    }
-  }
-
   get value {
     if (_callback != null) return _value;
     return _oneTime(_expr, _scope, _converter);
@@ -157,18 +324,19 @@ class _Binding extends Bindable {
     }
   }
 
-  open(callback(value)) {
+  Object open(callback(value)) {
     if (_callback != null) throw new StateError('already open');
 
     _callback = callback;
     final expr = observe(_expr, _scope);
-    _expr = expr;
-    _sub = expr.onUpdate.listen(_setValue)..onError((e) {
+//    _expr = expr;

[Jennifer Messerly, 2014/03/17 23:10:08]

remove?

[justinfagnani, 2014/04/24 00:26:13]

Done.

+    _sub = expr.onUpdate.listen(_updateValue)..onError((e) {
       _logger.warning("Error evaluating expression '$_expr': ${e.message}");
     });
     try {
       update(expr, _scope);
-      _value = _convertValue(expr.currentValue, _scope, _converter);
+      // _updateValue?
+      _value = expr.currentValue;
     } on EvalException catch (e) {
       _logger.warning("Error evaluating expression '$_expr': ${e.message}");
     }
@@ -180,7 +348,86 @@ class _Binding extends Bindable {
 
     _sub.cancel();
     _sub = null;
-    _expr = (_expr as ExpressionObserver).expression;
     _callback = null;
   }
 }
+
+class _ClassBinding extends _Binding {
+  _ClassBinding(Expression expr, Scope scope) : super(expr, scope);
+
+  static Object _oneTime(Expression expr, Scope scope) {
+    try {
+      var value = eval(expr, scope);
+    } on EvalException catch (e) {
+      _logger.warning("Error evaluating expression '$expr': ${e.message}");
+      return null;
+    }
+  }
+
+
+  _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;
+    super._updateValue(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;
+    super._updateValue(newValue);
+  }
+}
+
+/*
+ * 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;

[Jennifer Messerly, 2014/03/17 23:10:08]

does this need to be observable? maybe worth a comment about who will observe
it.

[justinfagnani, 2014/04/24 00:26:13]

removed this whole class and added a test to ensure the items are observed

+
+  _InBinding(Expression expr, this._identifier, Scope scope)
+      : super(expr, scope);
+
+  _updateValue(v) {
+    assert(v is Iterable || v == null);
+
+    _makeScope(value) =>
+        _createScope(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 _value = v == null ? null : new ObservableList.from(v.map(_makeScope));
+    if (_callback != null) _callback(_value);
+  }
+
+}