Node.bind(name, observable)

Node.bind(name, observable)

This patch changes the Node.bind so that bind always takes a binding name and an observable. The implication of this is that creating observers is now always the concern of TemplateBinding (and in some case the registered delegate).

Note that NodeBinding is now gone. The observable is now serving the same purpose the NodeBinding was.
 
R=arv
BUG=

Committed: https://github.com/Polymer/NodeBind/commit/0a09c0c

[arv, 2013-12-11 15:52:27]

Nice. I like the direction this is going.

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js
File src/NodeBind.js (right):

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode66
src/NodeBind.js:66: bindings[name] = undefined;
delete?

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode89
src/NodeBind.js:89: return value === undefined || value === null ? '' : value;
return value == null ? '' : value;

has same semantics

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode191
src/NodeBind.js:191: function updateInput(input, property, value, santizeFn) {
typo: sanitizeFn

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode192
src/NodeBind.js:192: input[property] = (santizeFn || sanitizeValue)(value);
Where did sanitizeValue come from?

Can you add a test that covers this?

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode195
src/NodeBind.js:195: function inputBinding(input, property, santizeFn) {
typo

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode198
src/NodeBind.js:198: }
semicolon

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode214
src/NodeBind.js:214: var capturedClose = observable.close;
This seems a bit strange. Is the main goal that we want a call to close done
outside of our control to remove the listener?

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode279
src/NodeBind.js:279: if (!this.bindings)
These 4 lines are same everywhere. Can you create a function?

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode316
src/NodeBind.js:316: if (option.parentNode instanceof HTMLSelectElement &&
var parentNode

[rafaelw, 2013-12-11 23:36:27]

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js
File src/NodeBind.js (right):

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode66
src/NodeBind.js:66: bindings[name] = undefined;
This is critical path. I don't want to put the object in slow mode. I actually
think that maybe bindings as public API on node isn't a good idea, but that's
another issue.

On 2013/12/11 15:52:27, arv wrote:
> delete?

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode89
src/NodeBind.js:89: return value === undefined || value === null ? '' : value;
On 2013/12/11 15:52:27, arv wrote:
> return value == null ? '' : value;
> 
> has same semantics

Done.

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode195
src/NodeBind.js:195: function inputBinding(input, property, santizeFn) {
On 2013/12/11 15:52:27, arv wrote:
> typo

Done.

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode198
src/NodeBind.js:198: }
On 2013/12/11 15:52:27, arv wrote:
> semicolon

Done.

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode214
src/NodeBind.js:214: var capturedClose = observable.close;
Yes. We need to remove the event listener when the binding is closed. This
avoids having to create a wrapper around the observable.

On 2013/12/11 15:52:27, arv wrote:
> This seems a bit strange. Is the main goal that we want a call to close done
> outside of our control to remove the listener?
>

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode279
src/NodeBind.js:279: if (!this.bindings)
On 2013/12/11 15:52:27, arv wrote:
> These 4 lines are same everywhere. Can you create a function?

Done.

https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode316
src/NodeBind.js:316: if (option.parentNode instanceof HTMLSelectElement &&
On 2013/12/11 15:52:27, arv wrote:
> var parentNode

Done.

[rafaelw, 2013-12-12 18:09:34]

Committed patchset #6 manually as r0a09c0c (presubmit successful).

[John Messerly, 2013-12-19 19:47:41]

(some comments I had from porting this)

https://codereview.appspot.com/40490043/diff/80001/src/NodeBind.js
File src/NodeBind.js (right):

https://codereview.appspot.com/40490043/diff/80001/src/NodeBind.js#newcode93
src/NodeBind.js:93: return value === undefined || value === null ? '' : value;
I was about to make the same comment as arv@ here:
https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode89

looks like maybe it wasn't uploaded

https://codereview.appspot.com/40490043/diff/80001/src/NodeBind.js#newcode212
src/NodeBind.js:212: observable.close = function() {
any idea how this design would work if you couldn't monkeypatch "close"?

I'm creating a wrapper object that forwards all the method calls except close
(basically keeping most of the old InputBinding type around). Does that seem
reasonable?

https://codereview.appspot.com/40490043/diff/80001/src/NodeBind.js#newcode310
src/NodeBind.js:310: var selectBinding = select.bindings.value;
not a big deal but I find it cleaner to put the "var" outside like the old code
had, if it is going to be used outside of the block. Someday if/when the world
moves to "let" and block scoping it will be nice to use that over "var" whenever
possible.

https://codereview.appspot.com/40490043/diff/80001/src/NodeBind.js#newcode314
src/NodeBind.js:314: option.value = sanitizeValue(value);;
extra semicolon

https://codereview.appspot.com/40490043/diff/80001/src/NodeBind.js#newcode317
src/NodeBind.js:317: selectBinding.setValue(select.value);
Should this test if setValue is present? In other words, if an observer is used
for a two-way bindable property and it doesn't implement setValue, is it an
error? PathObserver was pretty forgiving about that; it just dropped the new
value.

(I'm wondering if my base implementation of observer.setValue should do nothing,
or throw an error indicating an unsupported operation...)

https://codereview.appspot.com/40490043/diff/80001/src/NodeBind.js#newcode318
src/NodeBind.js:318: selectBinding.reset();
out of curiosity, do we ever want to setValue and not follow it immediately with
a reset?

[rafaelw, 2013-12-21 01:39:03]

https://codereview.appspot.com/40490043/diff/80001/src/NodeBind.js
File src/NodeBind.js (right):

https://codereview.appspot.com/40490043/diff/80001/src/NodeBind.js#newcode93
src/NodeBind.js:93: return value === undefined || value === null ? '' : value;
On 2013/12/19 19:47:43, John Messerly wrote:
> I was about to make the same comment as arv@ here:
> https://codereview.appspot.com/40490043/diff/60001/src/NodeBind.js#newcode89
> 
> looks like maybe it wasn't uploaded

Done.

https://codereview.appspot.com/40490043/diff/80001/src/NodeBind.js#newcode212
src/NodeBind.js:212: observable.close = function() {
Yup. That's how to do it (and how this code recently was doing it). This way
just avoids a little bit of allocation.

On 2013/12/19 19:47:43, John Messerly wrote:
> any idea how this design would work if you couldn't monkeypatch "close"?
> 
> I'm creating a wrapper object that forwards all the method calls except close
> (basically keeping most of the old InputBinding type around). Does that seem
> reasonable?

https://codereview.appspot.com/40490043/diff/80001/src/NodeBind.js#newcode310
src/NodeBind.js:310: var selectBinding = select.bindings.value;
On 2013/12/19 19:47:43, John Messerly wrote:
> not a big deal but I find it cleaner to put the "var" outside like the old
code
> had, if it is going to be used outside of the block. Someday if/when the world
> moves to "let" and block scoping it will be nice to use that over "var"
whenever
> possible.

Done.

https://codereview.appspot.com/40490043/diff/80001/src/NodeBind.js#newcode314
src/NodeBind.js:314: option.value = sanitizeValue(value);;
On 2013/12/19 19:47:43, John Messerly wrote:
> extra semicolon

Done.

https://codereview.appspot.com/40490043/diff/80001/src/NodeBind.js#newcode317
src/NodeBind.js:317: selectBinding.setValue(select.value);
I'm not sure. This is probably a question which will need to be answered in the
spec. I would feel free to implement what you think makes sense.

The JS code is taking a "let things fail when they fail approach" rather than
"detect possible failures early" -- mostly out of a perf concern.

On 2013/12/19 19:47:43, John Messerly wrote:
> Should this test if setValue is present? In other words, if an observer is
used
> for a two-way bindable property and it doesn't implement setValue, is it an
> error? PathObserver was pretty forgiving about that; it just dropped the new
> value.
> 
> (I'm wondering if my base implementation of observer.setValue should do
nothing,
> or throw an error indicating an unsupported operation...)

https://codereview.appspot.com/40490043/diff/80001/src/NodeBind.js#newcode318
src/NodeBind.js:318: selectBinding.reset();
For two-way bindings at least, we'll always want to reset it. The reason is a
worry that some other code will set the bound value back to what it's state
immediately preceding the user input. In that case, the observer will think that
nothing had changed, but the binding will now be out of sync with the input
value.

On 2013/12/19 19:47:43, John Messerly wrote:
> out of curiosity, do we ever want to setValue and not follow it immediately
with
> a reset?