TemplateBinding & NodeBind refactor

[Rafael Weinstein]

Hello all,

I'd like to propose two repo/design changes:

1) Merge the NodeBind Repo into TemplateBinding.

This will basically mean just moving the source & tests from NodeBind into TemplateBinding. This doesn't really change the design of the system (at least yet), but it does acknowledge that these two things really go together. It's one less repo to deal with and it means that we can eventually write a single pseudo-spec for the whole system and have it be in one place.

2) Remove Node.prototype.unbind, Node.prototype.unbindAll.

It's somewhat less clear to me that nodes should ever be unbind-able (rebind-able, or imperatively bind-able beyond template instancing, for that matter). The only use-case we've encountered for doing this is cleaning up (shutting down observation). However, if WeakRefs become available in ES or if TemplateBinding/NodeBind get standardized (and therefore make weak references available from c++), cleaning up observation can be a concern of the node itself, and not require external interaction.

Thus, the current design where Node.prototype.bind() is returning a "close-able" object is really only internal API for the prollyfill.

The main motivation for doing this is perf. Unbinding and setting up the .bindings object during construction are significant work.

I know that Polymer is currently using unbind and unbindAll(), but my proposal is for polymer to do what TemplateBinding does, which is to keep an array of closeable objects for each fragment that will eventually need to be cleaned up, rather than traverse a fragment and unbind all nodes.

3) Make Node.prototype.bindings run-time enable-able.

Again, this is significant work for which I know of only one use case (which is tooling -- e.g. the sandbox app).

I propose that we allow the bindings of a Node to be reflectable only if some well known switch is enabled. This is analogous to devtools using internal APIs to enable reflection.

Concerns?

[Igor Minar]

On Sun, Mar 9, 2014 at 9:23 AM, Rafael Weinstein <raf...@google.com> wrote:

Hello all,

I'd like to propose two repo/design changes:

1) Merge the NodeBind Repo into TemplateBinding.

This will basically mean just moving the source & tests from NodeBind into TemplateBinding. This doesn't really change the design of the system (at least yet), but it does acknowledge that these two things really go together. It's one less repo to deal with and it means that we can eventually write a single pseudo-spec for the whole system and have it be in one place.

I think that the two are related, but have very different roles and should not be mixed in order to keep the api boundaries and responsibilities clear.

Node.bind is about binding some model to something in a node and enabling two communication with the node.

TemplateBinding is all about taking a template finding all bindings, creating a clone of the template and setting up all bindings via Node.bind.

Node.bind is a primitive that could be useful in many scenarios outside of template instantiation, for this reason I think that the two should work together really well, but should be kept apart.

 

2) Remove Node.prototype.unbind, Node.prototype.unbindAll.

It's somewhat less clear to me that nodes should ever be unbind-able (rebind-able, or imperatively bind-able beyond template instancing, for that matter). The only use-case we've encountered for doing this is cleaning up (shutting down observation). However, if WeakRefs become available in ES or if TemplateBinding/NodeBind get standardized (and therefore make weak references available from c++), cleaning up observation can be a concern of the node itself, and not require external interaction.

Thus, the current design where Node.prototype.bind() is returning a "close-able" object is really only internal API for the prollyfill.

The main motivation for doing this is perf. Unbinding and setting up the .bindings object during construction are significant work.

I know that Polymer is currently using unbind and unbindAll(), but my proposal is for polymer to do what TemplateBinding does, which is to keep an array of closeable objects for each fragment that will eventually need to be cleaned up, rather than traverse a fragment and unbind all nodes.

+1

 

3) Make Node.prototype.bindings run-time enable-able.

Again, this is significant work for which I know of only one use case (which is tooling -- e.g. the sandbox app).

I propose that we allow the bindings of a Node to be reflectable only if some well known switch is enabled. This is analogous to devtools using internal APIs to enable reflection.

What's this "significant work"? Initializing one object and populating it's fields when a binding is set up? Anything else?

I'm on the fence with this one because of the great debugging story that it enables.

\i

 

Concerns?

Follow Polymer on Google+: plus.google.com/107187849809354688692
---
You received this message because you are subscribed to the Google Groups "Polymer" group.
To unsubscribe from this group and stop receiving emails from it, send an email to polymer-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/polymer-dev/CABMdHiTQTi8hbSQzvTvAiRt9ySiL6jzRcOxVhWGV%3DbRpQkhEtg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Addy Osmani]

1) Merge the NodeBind Repo into TemplateBinding.

This will basically mean just moving the source & tests from NodeBind into TemplateBinding. This doesn't really change the design of the system (at least yet), but it does acknowledge that these two things really go together. It's one less repo to deal with and it means that we can eventually write a single pseudo-spec for the whole system and have it be in one place.

Thanks for explaining, Raf. So, last year MDV was was broken down into smaller efforts because some of the other primitives involved (<template>, Object.observe etc) were being pursued on their own. You mentioned that the last two - Node.bind() and TemplateBinding were considered sufficiently distinct to split them into their own primitives. Is the main reason for the merge that we feel it would be more plausible to get vendor buy-in to a single spec that contains both rather than the individual pieces? 

I share Igor's sentiments below that the two pieces seem useful enough on their own and would be interested in hearing more about the reasoning for not keeping them apart. I'm sure there's a good set of reasons :)

 

2) Remove Node.prototype.unbind, Node.prototype.unbindAll.

It's somewhat less clear to me that nodes should ever be unbind-able (rebind-able, or imperatively bind-able beyond template instancing, for that matter). The only use-case we've encountered for doing this is cleaning up (shutting down observation). However, if WeakRefs become available in ES or if TemplateBinding/NodeBind get standardized (and therefore make weak references available from c++), cleaning up observation can be a concern of the node itself, and not require external interaction.

Thus, the current design where Node.prototype.bind() is returning a "close-able" object is really only internal API for the prollyfill.

The main motivation for doing this is perf. Unbinding and setting up the .bindings object during construction are significant work.

I know that Polymer is currently using unbind and unbindAll(), but my proposal is for polymer to do what TemplateBinding does, which is to keep an array of closeable objects for each fragment that will eventually need to be cleaned up, rather than traverse a fragment and unbind all nodes.

 

SGTM.

 

[Rafael Weinstein]

On Mon, Mar 10, 2014 at 5:04 AM, Addy Osmani <ad...@google.com> wrote:

1) Merge the NodeBind Repo into TemplateBinding.

This will basically mean just moving the source & tests from NodeBind into TemplateBinding. This doesn't really change the design of the system (at least yet), but it does acknowledge that these two things really go together. It's one less repo to deal with and it means that we can eventually write a single pseudo-spec for the whole system and have it be in one place.

Thanks for explaining, Raf. So, last year MDV was was broken down into smaller efforts because some of the other primitives involved (<template>, Object.observe etc) were being pursued on their own. You mentioned that the last two - Node.bind() and TemplateBinding were considered sufficiently distinct to split them into their own primitives. Is the main reason for the merge that we feel it would be more plausible to get vendor buy-in to a single spec that contains both rather than the individual pieces? 

I share Igor's sentiments below that the two pieces seem useful enough on their own and would be interested in hearing more about the reasoning for not keeping them apart. I'm sure there's a good set of reasons :)

Basically it's just unnecessary abstraction that we're paying a on-going maintenance cost for and there are presently no known customers for one or the other separately.

Also, I just no longer believe that there's a credible path to standardization for Node.bind separate from TemplateBinding. Putting them in one repo means we can write a single pseudo-spec which lives with the impl & describes the entire design and have a single reference point for if/when another UA becomes interested in standardizing.

 

 

2) Remove Node.prototype.unbind, Node.prototype.unbindAll.

It's somewhat less clear to me that nodes should ever be unbind-able (rebind-able, or imperatively bind-able beyond template instancing, for that matter). The only use-case we've encountered for doing this is cleaning up (shutting down observation). However, if WeakRefs become available in ES or if TemplateBinding/NodeBind get standardized (and therefore make weak references available from c++), cleaning up observation can be a concern of the node itself, and not require external interaction.

Thus, the current design where Node.prototype.bind() is returning a "close-able" object is really only internal API for the prollyfill.

The main motivation for doing this is perf. Unbinding and setting up the .bindings object during construction are significant work.

I know that Polymer is currently using unbind and unbindAll(), but my proposal is for polymer to do what TemplateBinding does, which is to keep an array of closeable objects for each fragment that will eventually need to be cleaned up, rather than traverse a fragment and unbind all nodes.

 

SGTM.

 

Follow Polymer on Google+: plus.google.com/107187849809354688692
---
You received this message because you are subscribed to the Google Groups "Polymer" group.
To unsubscribe from this group and stop receiving emails from it, send an email to polymer-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/polymer-dev/06921a0e-491f-4acf-a6bc-591ffe5721c6%40googlegroups.com.

[Rafael Weinstein]

I've implemented (2) & (3) and created a new branch which contains the changes:

https://github.com/Polymer/NodeBind/tree/noUnbind

(here's the CL: https://codereview.appspot.com/76140044/).

This change improved the binding benchmark (at 100% density with O.o enabled, but no compound bindings or expressions) by about 35%:

https://github.com/Polymer/TemplateBinding/blob/master/benchmark/index.html

and the codereview-diff.html benchmark(with O.o enabled)  by about 15%:

https://github.com/Polymer/TemplateBinding/blob/master/benchmark/codereview-diff.html

I leave it to Scott & Steve to let me know when/if Polymer-dev would like to integrate this change (by not using unbind/unbindAll anymore).

[Rafael Weinstein]

Ok. These changes are now on trunk (master) of NodeBind.

[Marcin Warpechowski]

Hi Rafael,

thanks for describing the motives behind this change and your work on improving the performance and the API.

Actually, I have a use case for point 3) - a need to programatically change Node binding value. 

In my app, I need to have buttons that change the value of a model property. With the exposed `bindings_` property, it is quite easy to achieve that. See the fiddle: http://jsfiddle.net/warpech/RZtLA/ . The HTML is not the cleanest, but the best we came up with.

Your recent changes to move `bindings_` behind a flag made me realise that using it is a poor way to achieve what I need. Yet, I haven't found a better solution. Would you propose more reliable solution to change bound values with a button?

cheers

[Scott Miles]

Sorry for butting in, but why not do this the more obvious way?

http://jsfiddle.net/warpech/RZtLA/

Follow Polymer on Google+: plus.google.com/107187849809354688692
---
You received this message because you are subscribed to the Google Groups "Polymer" group.
To unsubscribe from this group and stop receiving emails from it, send an email to polymer-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/polymer-dev/bd348702-cc39-4f29-9172-fa902eb0b134%40googlegroups.com.

[Marcin Warpechowski]

It seems that you pasted the wrong link. I am curious to see your solution!

[Scott Miles]

Er, http://jsfiddle.net/RZtLA/5/

[Marcin Warpechowski]

I see your point. Indeed, it looks simple until you want to bind to a deeper level value, say bind="user.firstName". In such case there would need to be a parser to find the correct path in the model. Something like: http://jsfiddle.net/warpech/RZtLA/6/

In my original code, I tried to reuse the binding facilities of NodeBind and its parser. Also, I liked the fact of consistent usage of {{ }} for property binding. 

[Scott Miles]

Sorry for the duplicate Marcin, I messed up on 'reply-all'.

On Mon, Mar 24, 2014 at 1:05 PM, Marcin Warpechowski <war...@gmail.com> wrote:

I see your point. Indeed, it looks simple until you want to bind to a deeper level value, say bind="user.firstName". In such case there would need to be a parser to find the correct path in the model. Something like: http://jsfiddle.net/warpech/RZtLA/6/

In my original code, I tried to reuse the binding facilities of NodeBind and its parser. Also, I liked the fact of consistent usage of {{ }} for property binding. 

IMO, you are using a sledge-hammer where you need a fly-swatter. 

(1) Using binding reflection so you can pass a static value in mustaches and pull it back out is wildly inefficient.

(2) Setting a property from a string path is a basic operation, you don't really need a 'parser'. There used to be a public method for doing this provided by MDV, but I can't find it now.

If you used Polymer, you would have two-way binding support, and this would be much easier.

 

To view this discussion on the web visit https://groups.google.com/d/msgid/polymer-dev/540d6fd3-dc16-4a4b-becf-394062f24b8f%40googlegroups.com.

[John Messerly]

On Mon, Mar 24, 2014 at 1:22 PM, Scott Miles <sjm...@google.com> wrote:

<snip>

(2) Setting a property from a string path is a basic operation, you don't really need a 'parser'. There used to be a public method for doing this provided by MDV, but I can't find it now.

Were you thinking of something like:

    Path.get('foo.bar.baz').setValueFrom(obj, value);

(source: https://github.com/Polymer/observe-js/blob/master/src/observe.js#L241)

[Marcin Warpechowski]

> If you used Polymer, you would have two-way binding support, and this would be much easier.

Scott,

Could you please elaborate how would Polymer solve that? I tried to keep low-level with my implementation (using only TemplateBinding) but I am all ears to this.

Anyway, to wrap up, you suggest that "templateInstance" is here to stay for the foreseeable future and it is OK to rely on it in code that uses TemplateBinding. After transforming your code, I see that I can make it even easier: http://jsfiddle.net/warpech/RZtLA/8/

@John Messerly thanks for your input but I think the above link has even cleaner solution now.