big update to observe, template_binding, polymer

pkg/template_binding/lib/src/element.dart

Index: pkg/template_binding/lib/src/element.dart
diff --git a/pkg/template_binding/lib/src/element.dart b/pkg/template_binding/lib/src/element.dart
index 1ee43f9317cab70eefa81464d32ba762bc29a58b..bf9a036123ae94b34be2ee5f0f9381d4fbcf3e0e 100644
--- a/pkg/template_binding/lib/src/element.dart
+++ b/pkg/template_binding/lib/src/element.dart
@@ -8,76 +8,63 @@ part of template_binding;
 class _ElementExtension extends NodeBindExtension {
   _ElementExtension(Element node) : super._(node);
 
-  NodeBinding bind(String name, model, [String path]) {
+  bind(String name, value, {bool oneTime: false}) {

[justinfagnani, 2014/02/04 20:16:08]

what's the point of returning the value? can we annotate bind() as returning
dynamic to document that there's a return value.

[Jennifer Messerly, 2014/02/04 23:00:31]

This is a question for https://github.com/polymer/nodebind, but here's my
understanding:

returning the Bindable is useful; it's not guaranteed to be the same one that is
passed in, and returning it avoids a lookup. This fact is exploited to quickly
collect the list of bindings-to-be-closed when a template instance is torn down.

returning a value in the oneTime binding case is a bit more dubious -- I think
it always returns null. As I noted in the code review (and maybe a github issue)
I feel like oneTime flag is combining two APIs into one. Rafael is sympathetic
but since the oneTime flag is currently experimental, prefers keeping it as a
flag.

Possibly the return type annotation can be "Bindable". I will check.

In general "dynamic" should never be used in Dart as a type annotation:
https://www.dartlang.org/articles/style-guide/#avoid-annotating-with-dynamic-...

It is used mainly for generic type arguments, like Map<String, dynamic>

[justinfagnani, 2014/02/04 23:34:49]

Yeah... I'm not sure I like that part of the style guide that much. I like to
know whether a function means to return values or not, separate from the type.
But consistency...

[Jennifer Messerly, 2014/02/04 23:40:41]

yeah, I try to use "void" for that, at least in public API things, but I dunno
how consistent most ppl are

     _self.unbind(name);
 
-    var binding;
-    if (_node is OptionElement && name == 'value') {
+    Element node = _node;

[justinfagnani, 2014/02/04 20:16:08]

This won't be necessary if you also add a check for "node is Element" below

[Jennifer Messerly, 2014/02/04 23:00:31]

sure, but since node is always guaranteed to be an element if we're in
_ElementExtension, I'd rather have a type assertion :)

+
+    if (node is OptionElement && name == 'value') {
       // Note: because <option> can be a semantic template, <option> will be
       // a TemplateBindExtension sometimes. So we need to handle it here.
-      (_node as OptionElement).attributes.remove(name);
-      binding = new _OptionValueBinding(_node, model, path);
-    } else {
-      binding = new _AttributeBinding(_node, name, model, path);
-    }
-    return bindings[name] = binding;
-  }
-}
-
-class _AttributeBinding extends NodeBinding {
-  final bool conditional;
-
-  _AttributeBinding._(node, name, model, path, this.conditional)
-      : super(node, name, model, path);
-
-  factory _AttributeBinding(Element node, name, model, path) {
-    bool conditional = name.endsWith('?');
-    if (conditional) {
       node.attributes.remove(name);
-      name = name.substring(0, name.length - 1);
-    }
-    return new _AttributeBinding._(node, name, model, path, conditional);
-  }
-
-  Element get node => super.node;
 
-  void valueChanged(value) {
-    if (conditional) {
-      if (_toBoolean(value)) {
-        node.attributes[property] = '';
-      } else {
-        node.attributes.remove(property);
-      }
+      if (oneTime) return _updateOption(value);
+      _open(value, _updateOption);
     } else {
-      // TODO(jmesserly): escape value if needed to protect against XSS.
-      // See https://github.com/polymer-project/mdv/issues/58
-      node.attributes[property] = sanitizeBoundValue(value);
-    }
-  }
-}
+      bool conditional = name.endsWith('?');
+      if (conditional) {
+        node.attributes.remove(name);

[justinfagnani, 2014/02/04 20:16:08]

.attributes will fail if Node isn't an Element. Add a check in the if?

[Jennifer Messerly, 2014/02/04 23:00:31]

it should not be possible. See the type check on the _ElementExtension ctor.

+        name = name.substring(0, name.length - 1);
+      }
 
-class _OptionValueBinding extends _ValueBinding {
-  _OptionValueBinding(node, model, path) : super(node, model, path);
+      if (oneTime) return _updateAttribute(_node, name, conditional, value);
 
-  OptionElement get node => super.node;
+      _open(value, (x) => _updateAttribute(_node, name, conditional, x));
+    }
+    return bindings[name] = value;
+  }
 
-  void valueChanged(newValue) {
+  void _updateOption(newValue) {
+    OptionElement node = _node;
     var oldValue = null;
     var selectBinding = null;
-    var select = node.parent;
+    var select = node.parentNode;
     if (select is SelectElement) {
       var valueBinding = nodeBind(select).bindings['value'];
-      if (valueBinding is _SelectBinding) {
+      if (valueBinding is _InputBinding) {
         selectBinding = valueBinding;
         oldValue = select.value;
       }
     }
 
-    super.valueChanged(newValue);
+    node.value = _sanitizeValue(newValue);
 
-    if (selectBinding != null && !selectBinding.closed &&
-        select.value != oldValue) {
-      selectBinding.nodeValueChanged(null);
+    if (selectBinding != null && select.value != oldValue) {
+      selectBinding.value = select.value;
+    }
+  }
+}
+
+void _updateAttribute(Element node, String name, bool conditional, value) {
+  if (conditional) {
+    if (_toBoolean(value)) {
+      node.attributes[name] = '';
+    } else {
+      node.attributes.remove(name);
     }
+  } else {
+    // TODO(jmesserly): escape value if needed to protect against XSS.
+    // See https://github.com/polymer-project/mdv/issues/58
+    node.attributes[name] = _sanitizeValue(value);
   }
 }