Databinding Library

Databinding Library

Support for 1-way bindings from model to forms. Favors minimal updates
where possible. 

Supports:
	Models
	ModelList (limited to updating all children on change notification)

Still Needs:
	LazyModelList of Plain JS object support
		will be done with the included Object.observe polyfill
	Collision Indication/handling

https://code.launchpad.net/~bcsaller/juju-gui/databinding-library/+merge/167676

(do not edit description out of merge proposal)

[bcsaller, 2013-06-06 00:17:29]

Please take a look.

[rharding, 2013-06-06 01:26:46]

So some feedback below. Some inconsistency with comments and *, but a couple of
questions on the bind example and such.

https://codereview.appspot.com/9860052/diff/1/app/assets/javascripts/Object.o...
File app/assets/javascripts/Object.observe.poly.js (right):

https://codereview.appspot.com/9860052/diff/1/app/assets/javascripts/Object.o...
app/assets/javascripts/Object.observe.poly.js:94: setTimeout(f, 100);
We've run into using the YUI value-change event which is implemented with
polling in test runs. I'd be great if this could be tweaked to allow this to be
set and altered for tests perhaps.

https://codereview.appspot.com/9860052/diff/1/app/modules-debug.js
File app/modules-debug.js (right):

https://codereview.appspot.com/9860052/diff/1/app/modules-debug.js#newcode34
app/modules-debug.js:34: //filter: 'debug',
was this meant to be changed?

https://codereview.appspot.com/9860052/diff/1/app/modules-debug.js#newcode40
app/modules-debug.js:40: combine: true,
and this

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode1
app/views/databinding.js:1: /*
per commenting lint this should be ** and indented two spaces.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode22
app/views/databinding.js:22: * Provide the DataBinding library.
and no leading * here.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode52
app/views/databinding.js:52: are specified all  bindings are updated.
double space

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode54
app/views/databinding.js:54: @method deltaFromChange
args, return values?

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode76
app/views/databinding.js:76: this.model = null;
these are basically ATTRS. Should we have a standard for documenting them? It's
easy with the multiple BindingEngine's around to lost track/hard to find them.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode99
app/views/databinding.js:99: *
stray *

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:112: * Bind a model to a DOM tree. This is a one-way
more *

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:119: *    - key: {String} name to index the model
under. This
key is mentioned as an option but I don't see how it gets in? It's either the
model.id or model.get('id')

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:123: *    - bindings: {Object} Bindings array, see
addBinding
same, where is bindings?

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:138: this._viewlets[viewlet.name] = viewlet;
if there was a previous binding, can there be a case where a viewlet needs to be
removed?

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:176: var self = this;
self is created but never used.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:238: views.BindingEngine = BindingEngine;
Can this be done initially so that upon loading the file I don't have to scroll
to the bottom to see it's stuck on the views namespace vs any of the other
created at the top?

https://codereview.appspot.com/9860052/diff/1/test-server.sh
File test-server.sh (right):

https://codereview.appspot.com/9860052/diff/1/test-server.sh#newcode8
test-server.sh:8: xdg-open http://localhost:8084/test/index.html
please revert

https://codereview.appspot.com/9860052/diff/1/test-server.sh#newcode11
test-server.sh:11: mocha-phantomjs -t 40000 http://localhost:8084/test/
same as ^^

https://codereview.appspot.com/9860052/diff/1/test/test_databinding.js
File test/test_databinding.js (right):

https://codereview.appspot.com/9860052/diff/1/test/test_databinding.js#newcode59
test/test_databinding.js:59: it('maintain proper binding references', function()
{
maintains

https://codereview.appspot.com/9860052/diff/1/test/test_databinding.js#newcode69
test/test_databinding.js:69: engine.bind(new Y.Model({foo: 'bar'}), container);
this is part of what I don't follow. You recall bind, only this time you're
passing a Node instance vs an object with container/bindings properties?

https://codereview.appspot.com/9860052/diff/1/test/test_databinding.js#newcod...
test/test_databinding.js:101: fieldShould('should be able to bind string to
inputs',
can we remove the should?

https://codereview.appspot.com/9860052/diff/1/test/test_databinding.js#newcod...
test/test_databinding.js:104: fieldShould('should be able to bind strings to
textareas',
dupe should

https://codereview.appspot.com/9860052/diff/1/test/test_databinding.js#newcod...
test/test_databinding.js:107: fieldShould('should be able to bind numbers to
inputs',
dupe should

[gary.poster, 2013-06-06 02:17:23]

ran out of time.  will read more tomorrow (when not on iPad :-P).  Very excited
by where this is going.

https://codereview.appspot.com/9860052/diff/1/app/assets/javascripts/Object.o...
File app/assets/javascripts/Object.observe.poly.js (right):

https://codereview.appspot.com/9860052/diff/1/app/assets/javascripts/Object.o...
app/assets/javascripts/Object.observe.poly.js:2: Tested against Chromium build
with Object.observe and acts EXACTLY the same,
I love this approach.  I think it is perfect for our project and the goals we've
stated lately for going forward.

I don't love this polyfill implenentation.  I like the older one
(https://github.com/tvcutsem/harmony-reflect/blob/master/examples/observer.js)
much better at first look, but I assume this is a better practical choice.

I think we need to include a reference as to where to find the original.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode62
app/views/databinding.js:62: return Y.Array.filter(this._bindings,
function(binding) {
this._bindings.filter?

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode65
app/views/databinding.js:65: }
how about replacing the above three lines with
return (modelChangeKeys.indexOf(binding.name) > -1);

[rharding, 2013-06-06 12:46:05]

Doh

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode1
app/views/databinding.js:1: /*
On 2013/06/06 01:26:46, rharding wrote:
> per commenting lint this should be ** and indented two spaces.

Ignore me, I've learned this morning that there's a legit diff between /* and
/** and had never realized it before.

[gary.poster, 2013-06-06 16:01:00]

Will review tests later but like where this is going

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode22
app/views/databinding.js:22: * Provide the DataBinding library.
On 2013/06/06 01:26:46, rharding wrote:
> and no leading * here.

That's my preference, but Ben is leading the charge towards making this
optional. :-)

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode76
app/views/databinding.js:76: this.model = null;
On 2013/06/06 01:26:46, rharding wrote:
> these are basically ATTRS. Should we have a standard for documenting them?
It's
> easy with the multiple BindingEngine's around to lost track/hard to find them.

I'm not sure I agree that these are ATTRS--ISTM that you want these to be
semi-private/protected, and I think of ATTRS as part of the interface.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode77
app/views/databinding.js:77: this._viewlets = {};   // {viewlet.name, viewlet}
I'm guessing you mean {viewlet.name: viewlet}?

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode86
app/views/databinding.js:86: * @param {Object} config A bindings Object, see
description.
See what description?  I'd really like to be able to read the intended usage
here.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode91
app/views/databinding.js:91: defaultBinding.get = function(model) { return
model.get(this.name);};
I assume this is (one of the places?) where you would need to add lazy list
support, with direct attrs.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode98
app/views/databinding.js:98: * Bind a model or modellist to one or more
viewlets.
Please reference the nicer docs in _bind

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:100: @method bind
Heh, at least be consistent with your * usage within a single docstring :-)

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:112: * Bind a model to a DOM tree. This is a one-way
Yeah, one-way is our model, isn't it.  Cool that we only have what we need.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:119: *    - key: {String} name to index the model
under. This
On 2013/06/06 01:26:46, rharding wrote:
> key is mentioned as an option but I don't see how it gets in? It's either the
> model.id or model.get('id')

AIUI the default is model.id, but it can be any arbitrary string that the caller
wants, and it gets in by using this exact "key" mechanism.  OTOH, I don't
actually understand the use case, particularly because below we have 

 135       // If we have a model, unbind it.
 136       if (this.model) {this.unbind();}

That implies only one model at a time.  I don't get it.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:123: *    - bindings: {Object} Bindings array, see
addBinding
Bindings are not actually documented anywhere afaict

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:123: *    - bindings: {Object} Bindings array, see
addBinding
Why would you specify bindings in addition to those provided on the viewlet?

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:138: this._viewlets[viewlet.name] = viewlet;
On 2013/06/06 01:26:46, rharding wrote:
> if there was a previous binding, can there be a case where a viewlet needs to
be
> removed? 
Same q from me.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:141: if (!Y.Lang.isFunction(model.size)) {
While I like duck typing on quacking when quacking is what I'm interested in, in
this case a size method is not clearly indicative of what you want to rely on. 
I suggest we actually look to see if the thing inherits from modellist or not.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:146: this._events.push(model.on('change',
this._modelChangeHandler, this));
We'll also want to support POJOs with observe, yeah?

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:155: // are currently read-only so this work ok.
I don't follow this story.  Maybe you could talk it through after the call?

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:168: BindingEngine.prototype.unbind = function() {
I'm guessing this will need the model in the future, or will get it from .model,
to unobserve

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:181: if (viewlet.update) {
ah ok.  so, if the viewlet knows how to update, do it, but otherwise, reset the
html.  So...that bypasses all of the nice binding stuff, right?  There is no
possibility to bind a field for a modellist?  If that's the case, let's handle
it elsewhere, and not muddy this "data binding" story, yeah?  Maybe this is
worth a discussion.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:228: // Do conflict detection
TODO, IIUC?

https://codereview.appspot.com/9860052/diff/1/test-server.sh
File test-server.sh (right):

https://codereview.appspot.com/9860052/diff/1/test-server.sh#newcode8
test-server.sh:8: xdg-open http://localhost:8084/test/index.html
On 2013/06/06 01:26:46, rharding wrote:
> please revert

Or share why you want/need this!

https://codereview.appspot.com/9860052/diff/1/test/test_databinding.js
File test/test_databinding.js (right):

https://codereview.appspot.com/9860052/diff/1/test/test_databinding.js#newcod...
test/test_databinding.js:101: fieldShould('should be able to bind string to
inputs',
On 2013/06/06 01:26:46, rharding wrote:
> can we remove the should?

+1

[bcsaller, 2013-06-06 18:29:43]

Please take a look.

[rharding, 2013-06-06 18:39:11]

LGTM thanks. The comments on bind are still a bit confusing, but I assume that
it'll make more sense with the viewlets actually providing the configuration
mentioned.

[benjamin.saller, 2013-06-06 18:48:02]

Thanks for the reviews, think I addressed things.

https://codereview.appspot.com/9860052/diff/1/app/assets/javascripts/Object.o...
File app/assets/javascripts/Object.observe.poly.js (right):

https://codereview.appspot.com/9860052/diff/1/app/assets/javascripts/Object.o...
app/assets/javascripts/Object.observe.poly.js:2: Tested against Chromium build
with Object.observe and acts EXACTLY the same,
On 2013/06/06 02:17:23, gary.poster wrote:
> I love this approach.  I think it is perfect for our project and the goals
we've
> stated lately for going forward.
> 
> I don't love this polyfill implenentation.  I like the older one
> (https://github.com/tvcutsem/harmony-reflect/blob/master/examples/observer.js)
> much better at first look, but I assume this is a better practical choice.
> 
> I think we need to include a reference as to where to find the original.

I suppose I'm fine with either impl. For this branch I'd like to just leave this
as is, the other introduces another dep (reflect) and since this code isn't used
yet, I'll defer doing that dance till its needed if thats ok.

https://codereview.appspot.com/9860052/diff/1/app/modules-debug.js
File app/modules-debug.js (right):

https://codereview.appspot.com/9860052/diff/1/app/modules-debug.js#newcode34
app/modules-debug.js:34: //filter: 'debug',
On 2013/06/06 01:26:46, rharding wrote:
> was this meant to be changed? 

No, it was just taking longer to do TDD w/o than I'd like. reverted

https://codereview.appspot.com/9860052/diff/1/app/modules-debug.js#newcode40
app/modules-debug.js:40: combine: true,
On 2013/06/06 01:26:46, rharding wrote:
> and this

Done.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode1
app/views/databinding.js:1: /*
On 2013/06/06 01:26:46, rharding wrote:
> per commenting lint this should be ** and indented two spaces.

I thought ** was only for YUIdoc comments, might be wrong though.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode22
app/views/databinding.js:22: * Provide the DataBinding library.
On 2013/06/06 01:26:46, rharding wrote:
> and no leading * here.

Done.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode52
app/views/databinding.js:52: are specified all  bindings are updated.
On 2013/06/06 01:26:46, rharding wrote:
> double space

Done.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode54
app/views/databinding.js:54: @method deltaFromChange
On 2013/06/06 01:26:46, rharding wrote:
> args, return values?

I don't always worry about the docstrings for private methods as YUI shouldn't
show it anyway, its not part of the interface, but I'll add them since you
pointed it out.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode62
app/views/databinding.js:62: return Y.Array.filter(this._bindings,
function(binding) {
On 2013/06/06 02:17:23, gary.poster wrote:
> this._bindings.filter?

Since we are reducing our dependencies on YUI the native method works here, I
can change that throughout.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode65
app/views/databinding.js:65: }
On 2013/06/06 02:17:23, gary.poster wrote:
> how about replacing the above three lines with
> return (modelChangeKeys.indexOf(binding.name) > -1);

Done.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode76
app/views/databinding.js:76: this.model = null;
On 2013/06/06 01:26:46, rharding wrote:
> these are basically ATTRS. Should we have a standard for documenting them?
It's
> easy with the multiple BindingEngine's around to lost track/hard to find them.

I see them as private/protected and not part of the interface. I may allow model
to be a property that automatically calls bind with the existing viewlets in
place though.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode77
app/views/databinding.js:77: this._viewlets = {};   // {viewlet.name, viewlet}
On 2013/06/06 16:01:01, gary.poster wrote:
> I'm guessing you mean {viewlet.name: viewlet}?

Done.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode86
app/views/databinding.js:86: * @param {Object} config A bindings Object, see
description.
On 2013/06/06 16:01:01, gary.poster wrote:
> See what description?  I'd really like to be able to read the intended usage
> here.

I expanded the docs on bind and reference that here.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode91
app/views/databinding.js:91: defaultBinding.get = function(model) { return
model.get(this.name);};
On 2013/06/06 16:01:01, gary.poster wrote:
> I assume this is (one of the places?) where you would need to add lazy list
> support, with direct attrs.

Yes, its to support that, I may have to juggle how its spelled a little but that
is the intention.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode98
app/views/databinding.js:98: * Bind a model or modellist to one or more
viewlets.
On 2013/06/06 16:01:01, gary.poster wrote:
> Please reference the nicer docs in _bind

Changed as noted above.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcode99
app/views/databinding.js:99: *
On 2013/06/06 01:26:46, rharding wrote:
> stray *

Done.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:100: @method bind
On 2013/06/06 16:01:01, gary.poster wrote:
> Heh, at least be consistent with your * usage within a single docstring :-)

Done.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:112: * Bind a model to a DOM tree. This is a one-way
On 2013/06/06 01:26:46, rharding wrote:
> more *

Done.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:119: *    - key: {String} name to index the model
under. This
On 2013/06/06 16:01:01, gary.poster wrote:
> On 2013/06/06 01:26:46, rharding wrote:
> > key is mentioned as an option but I don't see how it gets in? It's either
the
> > model.id or model.get('id')
> 
> AIUI the default is model.id, but it can be any arbitrary string that the
caller
> wants, and it gets in by using this exact "key" mechanism.  OTOH, I don't
> actually understand the use case, particularly because below we have 
> 
>  135       // If we have a model, unbind it.
>  136       if (this.model) {this.unbind();}
> 
> That implies only one model at a time.  I don't get it.

You get it, that wasn't always the case. Initially we wanted to support binding
around child elements in modellists which meant more than one 'cursor' being
associated with the binding engine and models keyed under those names. The D3
prototype made the model list styled bindings very easy as it kept proper
context but we can't do that here w/o tons of code. To support model lists we
only intercept change notifications and re-render its content. This works in
practice because our modellist (collection) view are read only.

The 'key' stuff has been removed.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:123: *    - bindings: {Object} Bindings array, see
addBinding
On 2013/06/06 16:01:01, gary.poster wrote:
> Why would you specify bindings in addition to those provided on the viewlet?

addBinding is part of the interface to support dynamically generated forms where
we create both the form and the bindings from data (such as with service
config).

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:138: this._viewlets[viewlet.name] = viewlet;
On 2013/06/06 16:01:01, gary.poster wrote:
> On 2013/06/06 01:26:46, rharding wrote:
> > if there was a previous binding, can there be a case where a viewlet needs
to
> be
> > removed? 
> Same q from me.

Unbind would have been called cleaning the events. There isn't state with the
viewlet that needs special handling that the overwrite here wouldn't handle. I
don't support removing viewlets at this time (but easily could) in which case we
could remove it from the associative array.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:146: this._events.push(model.on('change',
this._modelChangeHandler, this));
On 2013/06/06 16:01:01, gary.poster wrote:
> We'll also want to support POJOs with observe, yeah?

I suspect we will, observe is a clear win for that, but its outside our use case
now

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:155: // are currently read-only so this work ok.
On 2013/06/06 16:01:01, gary.poster wrote:
> I don't follow this story.  Maybe you could talk it through after the call?

We had a call about this, outcome is basically whats here is fine for now and
there are possible paths forward towards better support.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:168: BindingEngine.prototype.unbind = function() {
On 2013/06/06 16:01:01, gary.poster wrote:
> I'm guessing this will need the model in the future, or will get it from
.model,
> to unobserve

It clears the event handles now, when we add Object.observe, yes it will switch
on each handler in the events list and do the right thing.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:176: var self = this;
On 2013/06/06 01:26:46, rharding wrote:
> self is created but never used.

Done.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:181: if (viewlet.update) {
On 2013/06/06 16:01:01, gary.poster wrote:
> ah ok.  so, if the viewlet knows how to update, do it, but otherwise, reset
the
> html.  So...that bypasses all of the nice binding stuff, right?  There is no
> possibility to bind a field for a modellist?  If that's the case, let's handle
> it elsewhere, and not muddy this "data binding" story, yeah?  Maybe this is
> worth a discussion.

Discussed on call, we have options if this becomes more of a requirement.

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:228: // Do conflict detection
On 2013/06/06 16:01:01, gary.poster wrote:
> TODO, IIUC?

Yes, this will likely represent a convention established with viewlets. Like in
my prototype I'll annotate the DOM with data-modelValue and add/remove classes
on conflict here. 

I currently don't know if its worth having a conflict callback as I'd like to
keep that level of things pretty simple, we don't have use cases for complex
interactive resolution (yet).

https://codereview.appspot.com/9860052/diff/1/app/views/databinding.js#newcod...
app/views/databinding.js:238: views.BindingEngine = BindingEngine;
On 2013/06/06 01:26:46, rharding wrote:
> Can this be done initially so that upon loading the file I don't have to
scroll
> to the bottom to see it's stuck on the views namespace vs any of the other
> created at the top?

Its a pretty common pattern, but yes, I'll make the change.

https://codereview.appspot.com/9860052/diff/1/test-server.sh
File test-server.sh (right):

https://codereview.appspot.com/9860052/diff/1/test-server.sh#newcode8
test-server.sh:8: xdg-open http://localhost:8084/test/index.html
On 2013/06/06 16:01:01, gary.poster wrote:
> On 2013/06/06 01:26:46, rharding wrote:
> > please revert
> 
> Or share why you want/need this!

0.0.0.0 usually either means 'no IP address', 'default gateway' or 'bind socket
to all interfaces'. It is not usually a resolvable address. 

Was this change made to make interacting with VM's simpler? In that case I might
recommend that we use an env variable for the host so 
it can be localhost || 192.168.x.1.

I can revert this for now, but I don't think this its a good solution to target
the default gateway directly like this.

https://codereview.appspot.com/9860052/diff/1/test/test_databinding.js
File test/test_databinding.js (right):

https://codereview.appspot.com/9860052/diff/1/test/test_databinding.js#newcode69
test/test_databinding.js:69: engine.bind(new Y.Model({foo: 'bar'}), container);
On 2013/06/06 01:26:46, rharding wrote:
> this is part of what I don't follow. You recall bind, only this time you're
> passing a Node instance vs an object with container/bindings properties?

The value isnt' actually being checked or used in this example. It was incorrect
and from a previous interface as you picked up on, fixed.

https://codereview.appspot.com/9860052/diff/1/test/test_databinding.js#newcod...
test/test_databinding.js:101: fieldShould('should be able to bind string to
inputs',
On 2013/06/06 01:26:46, rharding wrote:
> can we remove the should?

Done.

[bcsaller, 2013-06-06 20:11:02]

Please take a look.

[gary.poster, 2013-06-06 21:04:21]

LGTM with trivials.  some comments are from older version.  thank you!

Gary

https://codereview.appspot.com/9860052/diff/11001/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/9860052/diff/11001/app/views/databinding.js#ne...
app/views/databinding.js:51: are specified all  bindings are updated.
trivial punctuation/spacing:

are specified, all bindings are updated.

https://codereview.appspot.com/9860052/diff/11001/app/views/databinding.js#ne...
app/views/databinding.js:92: if (typeof binding === 'string') {
I think you must mean

if (typeof binding.target === 'string') {

https://codereview.appspot.com/9860052/diff/11001/app/views/databinding.js#ne...
app/views/databinding.js:118: elements bound to this model attribute.
You can define "get" also in order to change the default getter, right?

https://codereview.appspot.com/9860052/diff/21001/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/9860052/diff/21001/app/views/databinding.js#ne...
app/views/databinding.js:69: * CHeck if a target name is in classes bases
trivial: Check

https://codereview.appspot.com/9860052/diff/21001/undocumented
File undocumented (right):

https://codereview.appspot.com/9860052/diff/21001/undocumented#newcode81
undocumented:81: app/views/databinding.js:44 "set"
Can't you make these go away with some trick or other?  Like a /* explain what
is going on */ or something?  I'd really prefer to try to see this file get
smaller, not bigger, and have worked to do so.

[bcsaller, 2013-06-06 21:38:35]

*** Submitted:

Databinding Library

Support for 1-way bindings from model to forms. Favors minimal updates
where possible. 

Supports:
	Models
	ModelList (limited to updating all children on change notification)

Still Needs:
	LazyModelList of Plain JS object support
		will be done with the included Object.observe polyfill
	Collision Indication/handling

R=rharding, gary.poster, benjamin.saller
CC=
https://codereview.appspot.com/9860052