switch Node.bind to interop

pkg/template_binding/lib/src/node.dart

Index: pkg/template_binding/lib/src/node.dart
diff --git a/pkg/template_binding/lib/src/node.dart b/pkg/template_binding/lib/src/node.dart
index fe6f9e6c611db4ba0ba485d9572a4521ab3e83c3..297f9587ec98941b0b728ba87307db92d4c92fc5 100644
--- a/pkg/template_binding/lib/src/node.dart
+++ b/pkg/template_binding/lib/src/node.dart
@@ -7,6 +7,11 @@ part of template_binding;
 /** Extensions to the [Node] API. */
 class NodeBindExtension {
   final Node _node;
+  final JsObject _js;
+
+  NodeBindExtension._(node)
+      : _node = node,
+        _js = new JsObject.fromBrowserObject(node);
 
   /**
    * Gets the data bindings that are associated with this node, if any.
@@ -17,9 +22,24 @@ class NodeBindExtension {
   // Dart note: in JS this has a trailing underscore, meaning "private".
   // But in dart if we made it _bindings, it wouldn't be accessible at all.
   // It is unfortunately needed to implement Node.bind correctly.
-  Map<String, Bindable> bindings;
-
-  NodeBindExtension._(this._node);
+  Map<String, Bindable> get bindings {
+    var b = _js['bindings_'];
+    if (b == null) return null;
+    // TODO(jmesserly): should cache this for identity.
+    return new _NodeBindingsMap(b);
+  }
+  set bindings(Map<String, Bindable> value) {

[Siggi Cherem (dart-lang), 2014/06/27 18:40:52]

nit: +empty line above

[Jennifer Messerly, 2014/06/27 19:10:03]

Done.

+    if (value == null) {
+      _js.deleteProperty('bindings_');
+      return;
+    }
+    var b = bindings;
+    if (b == null) {
+      _js['bindings_'] = new JsObject.jsify({});
+      b = bindings;
+    }
+    b.addAll(value);
+  }
 
   /**
    * Binds the attribute [name] to the [path] of the [model].

[Siggi Cherem (dart-lang), 2014/06/27 18:40:53]

I independently sent you a CL to update this outdated comment, but feel free to
update it here instead

[Jennifer Messerly, 2014/06/27 19:10:03]

lgtm!

@@ -27,50 +47,116 @@ class NodeBindExtension {
    * Returns the [Bindable] instance.
    */
   Bindable bind(String name, value, {bool oneTime: false}) {
-    // TODO(jmesserly): in Dart we could deliver an async error, which would
-    // have a similar affect but be reported as a test failure. Should we?
-    window.console.error('Unhandled binding to Node: '
-        '$this $name $value $oneTime');
-    return null;
+    name = _dartToJsName(name);
+
+    if (!oneTime && value is Bindable) {
+      value = bindableToJsObject(value);
+    }
+    return jsObjectToBindable(_js.callMethod('bind', [name, value, oneTime]));
   }
 
   /**
    * Called when all [bind] calls are finished for a given template expansion.
    */
-  void bindFinished() {}
-
-  /**
-   * Dispatch support so custom HtmlElement's can override these methods.
-   *
-   * A public method like [this.bind] should not call another public method.
-   *
-   * Instead it should dispatch through [_self] to give the "overridden" method
-   * a chance to intercept.
-   */
-  NodeBindExtension get _self => _node is NodeBindExtension ? _node : this;
+  bindFinished() => _js.callMethod('bindFinished');
 
+  // Note: confusingly this is on NodeBindExtension because it can be on any
+  // Node. It's really an API added by TemplateBinding. Therefore it stays
+  // implemented in Dart because TemplateBinding still is.
   TemplateInstance _templateInstance;
 
   /** Gets the template instance that instantiated this node, if any. */
   TemplateInstance get templateInstance =>
       _templateInstance != null ? _templateInstance :
       (_node.parent != null ? nodeBind(_node.parent).templateInstance : null);
+}
+
+class _NodeBindingsMap extends MapBase<String, Bindable> {
+  final JsObject _bindings;
+
+  _NodeBindingsMap(this._bindings);
+
+  // TODO(jmesserly): this should be lazy
+  Iterable<String> get keys =>
+      js.context['Object'].callMethod('keys', [_bindings]).map(_jsToDartName);
+
+  Bindable operator[](String name) =>
+      jsObjectToBindable(_bindings[_dartToJsName(name)]);
+
+  operator[]=(String name, Bindable value) {
+    _bindings[_dartToJsName(name)] = bindableToJsObject(value);
+  }
 
-  _open(Bindable bindable, callback(value)) =>
-      callback(bindable.open(callback));
+  @override Bindable remove(String name) {
+    name = _dartToJsName(name);
+    var old = this[name];
+    _bindings.deleteProperty(name);
+    return old;
+  }
 
-  Bindable _maybeUpdateBindings(String name, Bindable binding) {
-    return enableBindingsReflection ? _updateBindings(name, binding) : binding;
+  @override void clear() {
+    keys.forEach(remove);

[Siggi Cherem (dart-lang), 2014/06/27 18:40:52]

since this is pretty much private, could we instead hold here the _js object and
delete _js.binding entirely?

[Jennifer Messerly, 2014/06/27 19:10:04]

that's an interesting idea. Certainly in practice, nulling out .bindings is
better for the user to freeing it. I think maybe I'd hesitate to that from
.clear(), because it means node.bindings.clear(), followed node.bindings['foo']
= bar, would blow up because node.bindings is now null. Or anyone that had saved
the node.bindings map -- it would suddenly be invalid rather than empty.

Instead of delete entirely, we could say _js['bindings_'] = new
JsObject.jsify({});, which would avoid the above problems and be an O(1) clear
operation.

Honestly though, I only implemented this method because MapBase's comment says
I'm supposed to :). I doubt anyone will ever call it. IMHO, not worth
optimizing.

[Siggi Cherem (dart-lang), 2014/06/27 19:22:45]

sgtm - let's just add a comment to not forget that it's possible, but I agree
it's not really worth it.

   }
+}
+
+// TODO(jmesserly): perhaps we should switch Dart back to 'textContent' for
+// consistency? This only affects the raw Node.bind API. Seems like a lot of
+// magic to support it.

[Siggi Cherem (dart-lang), 2014/06/27 18:40:53]

not sure if it's worth to have this TODO here. Maybe create a bug for dart:html,
but honestly I don't think it's worth the breaking change there.

[Jennifer Messerly, 2014/06/27 21:03:37]

Oh -- definitely did *not* mean to suggest a change dart:html :)
Clarified comment.

+String _dartToJsName(String name) {

[Siggi Cherem (dart-lang), 2014/06/27 18:40:53]

nit: how about just a 1 liner:
String _dartToJsName(String name) => name == 'text' ? 'textContent' : name;

(same below)

[Jennifer Messerly, 2014/06/27 21:03:36]

Done.

+  if (name == 'text') name = 'textContent';
+  return name;
+}
+
+
+String _jsToDartName(String name) {
+  if (name == 'textContent') name = 'text';
+  return name;
+}
 
-  Bindable _updateBindings(String name, Bindable binding) {
-    if (bindings == null) bindings = {};
-    var old = bindings[name];
-    if (old != null) old.close();
-    return bindings[name] = binding;
+
+/// Given a bindable [JsObject], wraps it in a Dart [Bindable].
+/// See [bindableToJsObject] to go in the other direction.
+Bindable jsObjectToBindable(JsObject obj) {
+  if (obj == null) return null;
+  var b = obj['__dartBindable'];
+  return b is Bindable ? b : new _JsBindable(obj);

[Siggi Cherem (dart-lang), 2014/06/27 18:40:53]

could 'b' ever be non-null & not Bindable? How about:

if (b != null) {
  assert (b is Bindable);
  return b;
}
return new _JsBindable(obj);

[Jennifer Messerly, 2014/06/27 19:10:04]

yeah. I think that's essentially:

    return b != null ? b : new _JsBindable(obj);

(since we have a return type annotation)

I went back and forth. I guess I was thinking: if JS code messes the
__dartBindable property, we don't really care and can still work correctly, "is
Bindable" gives us the null check and also lets us fall back easily. But you're
right that it's extremely unlikely to happen either way.

[Siggi Cherem (dart-lang), 2014/06/27 19:22:45]

makes sense. let's keep it as it is, maybe mention that we expect that
invariant.

[Jennifer Messerly, 2014/06/27 21:03:36]

Done.

+}
+
+class _JsBindable extends Bindable {
+  final JsObject _js;
+  _JsBindable(JsObject obj) : _js = obj;
+
+  open(callback) => _js.callMethod('open', [callback]);
+
+  close() => _js.callMethod('close');
+  
+  get value => _js.callMethod('discardChanges');
+
+  set value(newValue) {
+    _js.callMethod('discardChanges');
+    _js.callMethod('setValue', [newValue]);
   }
+
+  deliver() => _js.callMethod('deliver');
 }
 
+/// Given a [bindable], create a JS object proxy for it.
+/// This is the inverse of [jsObjectToBindable].
+JsObject bindableToJsObject(Bindable bindable) {
+  if (bindable is _JsBindable) return bindable._js;
+
+  return new JsObject.jsify({
+    'open': (callback) => bindable.open((x) => callback.apply([x])),
+    'close': () => bindable.close(),
+    'discardChanges': () => bindable.value,
+    'setValue': (x) => bindable.value = x,
+    // NOTE: this is not used by Node.bind, but it's used by Polymer:
+    // https://github.com/Polymer/polymer-dev/blob/ba2b68fe5a5721f60b5994135f3270e63588809a/src/declaration/properties.js#L130

[Siggi Cherem (dart-lang), 2014/06/27 18:40:53]

would this be an issue anymore if we had the js-exporter?

I mean - polymer assumes that the object is a CompundObserver which happens to
have the `deliver` method, so if we are jsifying using the exporter API,
`deliver` would be included in that case, right?

[Jennifer Messerly, 2014/06/27 21:03:36]

That's a really good point. We could actually remove "deliver" from Bindable,
and only add it dynamically here. However, it doesn't give me a good feeling
since Polymer still assumes anything that is passed to Node.bind has the method.
I expanded the comment to add your note and mine about how this is ultimately a
Polymer-JS issue.

+    'deliver': () => bindable.deliver(),
+    // Save this so we can return it from [jsObjectToBindable]
+    '__dartBindable': bindable
+  });
+}
 
 /** Information about the instantiated template. */
 class TemplateInstance {