NodeBind: oneTime

NodeBind: oneTime.

This patch changes the call signature of bind(), which now takes: name, value, oneTime.

oneTime is expected to be true if the caller wishes the bound value updated but doesn't require it to track an observable. if oneTime is true, it is expected that value will NOT be an observable, otherwise it will be.

R=arv
BUG=

Committed: https://github.com/Polymer/NodeBind/commit/b9266f7

[arv, 2013-12-18 20:18:34]

https://codereview.appspot.com/43700043/diff/20001/src/NodeBind.js
File src/NodeBind.js (right):

https://codereview.appspot.com/43700043/diff/20001/src/NodeBind.js#newcode108
src/NodeBind.js:108: return Node.prototype.bind.call(this, name, value);
Pass along oneTime?

https://codereview.appspot.com/43700043/diff/20001/src/NodeBind.js#newcode343
src/NodeBind.js:343: return HTMLElement.prototype.bind.call(this, name, value);
here too

https://codereview.appspot.com/43700043/diff/20001/src/NodeBind.js#newcode379
src/NodeBind.js:379: return HTMLElement.prototype.bind.call(this, name, value);
and here

[rafaelw, 2013-12-18 20:46:19]

https://codereview.appspot.com/43700043/diff/20001/src/NodeBind.js
File src/NodeBind.js (right):

https://codereview.appspot.com/43700043/diff/20001/src/NodeBind.js#newcode108
src/NodeBind.js:108: return Node.prototype.bind.call(this, name, value);
Good catch. Thank you. Fixed here and everywhere else.

On 2013/12/18 20:18:35, arv wrote:
> Pass along oneTime?

https://codereview.appspot.com/43700043/diff/20001/src/NodeBind.js#newcode343
src/NodeBind.js:343: return HTMLElement.prototype.bind.call(this, name, value);
On 2013/12/18 20:18:35, arv wrote:
> here too

Done.

https://codereview.appspot.com/43700043/diff/20001/src/NodeBind.js#newcode343
src/NodeBind.js:343: return HTMLElement.prototype.bind.call(this, name, value);
On 2013/12/18 20:18:35, arv wrote:
> here too

Done.

https://codereview.appspot.com/43700043/diff/20001/src/NodeBind.js#newcode379
src/NodeBind.js:379: return HTMLElement.prototype.bind.call(this, name, value);
On 2013/12/18 20:18:35, arv wrote:
> and here

Done.

[rafaelw, 2013-12-18 20:46:38]

Committed patchset #3 manually as rb9266f7 (presubmit successful).

[John Messerly, 2013-12-19 03:53:34]

https://codereview.appspot.com/43700043/diff/40001/src/NodeBind.js
File src/NodeBind.js (right):

https://codereview.appspot.com/43700043/diff/40001/src/NodeBind.js#newcode106
src/NodeBind.js:106: Text.prototype.bind = function(name, value, oneTime) {
It seems like the behavior of oneTime vs not one time is fairly different. (The
fact that the "value" parameter can be two distinct kinds of things is a good
indication.) Is there a way to structure this differently, perhaps as two
methods?

Currently the pattern seems to be:

    if (... matches a property ...) {
      if (oneTime) return updateProperty(name, value);

      this.unbind(name);
      // note: the actual code uses more optimized closures
      var self = this;
      updateProperty(name, value.open(function(x) {
        updateProperty(self, name, x);
      }));
      return this.bindings[name] = value;
    } else {
      return super.bind(...);
    }

Could updateProperty be a distinct method? That way, oneTime bindings are
handled without needing a boolean, and observable bindings build on the
updateProperty primitive.

(Something would have to be done about the removeAttribute logic. It feels very
boilerplate-ish right now. IMHO, I'd move it out of either method and make it a
concern of TemplateBinding, since it has the job of parsing the attributes.)

[rafaelw1, 2014-01-07 23:28:43]

On Wed, Dec 18, 2013 at 7:53 PM, <jmesserly@google.com> wrote:

>
> https://codereview.appspot.com/43700043/diff/40001/src/NodeBind.js
> File src/NodeBind.js (right):
>
> https://codereview.appspot.com/43700043/diff/40001/src/
> NodeBind.js#newcode106
> src/NodeBind.js:106: Text.prototype.bind = function(name, value,
> oneTime) {
> It seems like the behavior of oneTime vs not one time is fairly
> different. (The fact that the "value" parameter can be two distinct
> kinds of things is a good indication.) Is there a way to structure this
> differently, perhaps as two methods?
>
>
I totally understand the criticism. I can also see how this API makes less
sense in a typed world. I'm somewhat disinclined to polish this API at the
moment -- mainly out of fear or regressing perf but also because I haven't
seen any actual uses of one-time bindings in the wild yet.

If no one uses the feature, we should just remove it -- rather than
investing more time in it.


> Currently the pattern seems to be:
>
>     if (... matches a property ...) {
>       if (oneTime) return updateProperty(name, value);
>
>       this.unbind(name);
>       // note: the actual code uses more optimized closures
>       var self = this;
>       updateProperty(name, value.open(function(x) {
>         updateProperty(self, name, x);
>       }));
>       return this.bindings[name] = value;
>     } else {
>       return super.bind(...);
>     }
>
> Could updateProperty be a distinct method? That way, oneTime bindings
> are handled without needing a boolean, and observable bindings build on
> the updateProperty primitive.
>
> (Something would have to be done about the removeAttribute logic. It
> feels very boilerplate-ish right now. IMHO, I'd move it out of either
> method and make it a concern of TemplateBinding, since it has the job of
> parsing the attributes.)
>
> https://codereview.appspot.com/43700043/
>

[John Messerly, 2014-01-07 23:58:39]

Yeah that sounds like a good approach.


On Tue, Jan 7, 2014 at 3:28 PM, Rafael Weinstein <rafaelw@google.com> wrote:

> On Wed, Dec 18, 2013 at 7:53 PM, <jmesserly@google.com> wrote:
>
>>
>> https://codereview.appspot.com/43700043/diff/40001/src/NodeBind.js
>> File src/NodeBind.js (right):
>>
>> https://codereview.appspot.com/43700043/diff/40001/src/
>> NodeBind.js#newcode106
>> src/NodeBind.js:106: Text.prototype.bind = function(name, value,
>> oneTime) {
>> It seems like the behavior of oneTime vs not one time is fairly
>> different. (The fact that the "value" parameter can be two distinct
>> kinds of things is a good indication.) Is there a way to structure this
>> differently, perhaps as two methods?
>>
>>
> I totally understand the criticism. I can also see how this API makes less
> sense in a typed world. I'm somewhat disinclined to polish this API at the
> moment -- mainly out of fear or regressing perf but also because I haven't
> seen any actual uses of one-time bindings in the wild yet.
>
> If no one uses the feature, we should just remove it -- rather than
> investing more time in it.
>
>
>> Currently the pattern seems to be:
>>
>>     if (... matches a property ...) {
>>       if (oneTime) return updateProperty(name, value);
>>
>>       this.unbind(name);
>>       // note: the actual code uses more optimized closures
>>       var self = this;
>>       updateProperty(name, value.open(function(x) {
>>         updateProperty(self, name, x);
>>       }));
>>       return this.bindings[name] = value;
>>     } else {
>>       return super.bind(...);
>>     }
>>
>> Could updateProperty be a distinct method? That way, oneTime bindings
>> are handled without needing a boolean, and observable bindings build on
>> the updateProperty primitive.
>>
>> (Something would have to be done about the removeAttribute logic. It
>> feels very boilerplate-ish right now. IMHO, I'd move it out of either
>> method and make it a concern of TemplateBinding, since it has the job of
>> parsing the attributes.)
>>
>> https://codereview.appspot.com/43700043/
>>
>
>