googlearchive / TemplateBinding

TemplateBinding Prolyfill
290 stars 61 forks source link

.model isn't a property of a node, artifact from old MDV design #133

Closed sethladd closed 11 years ago

sethladd commented 11 years ago

I have concerns with the existing API design, and wish to suggest a change to make things more obvious.

Today, templates have what appears to be a setter for a model.

template.model = thing

This is historical from a previous design of MDV, and today's design no longer matches the apparent intent of .model. The template element does not have a property/field for the model.

To make it very clear that the developer is performing an action, I propose to remove .model = thing and replace it with .provideModel(thing)

Benefits:

(note: I care less about the name of the provideModel function. There's probably a better name.)

jmesserly commented 11 years ago

Today, templates have what appears to be a setter for a model.

it has a getter as well

This is historical from a previous design of MDV, and today's design no longer matches the apparent intent of .model. The template element does not have a property/field for the model.

That's not the accurate history :). MDV v2 got rid of .model and introduced HTMLTemplateElement.bindAll (citation). It was changed back to a "model" setter as an improvement. I like it because it makes a more sense to have a getter and a setter. Otherwise you have no way of knowing what a <template> is bound to.

To make it very clear that the developer is performing an action, I propose to remove .model = thing and replace it with .provideModel(thing)

The DOM has setters that perform actions, and JS has setters (and Dart does to) so I'm not sure I understand the objection to setters.

jmesserly commented 11 years ago

fyi here is the source for model getter/setter: https://github.com/Polymer/mdv/blob/master/src/template_element.js#L825

sethladd commented 11 years ago

Thanks for the feedback. Here's another thing that's weird with .model as a getter/setter. Calling .model on a template can return undefined which means two things: nothing is bound, or undefined is bound. (or did I get that wrong?)

So let me try to list the things I want to do, and then maybe there's already an API to do it:

so:

Is that accurate?

jmesserly commented 11 years ago

I don't understand why you want to do either of these things:

  • take a model away from a template, which deactivates the template, and removes it from the dom
  • ask if a template has a model

The normal way to use MDV is to bind a model and leave it.

  • ask if a template has a model

if you want to know "was this template node ever bound to a model", node.bindings will tell you if it has bindings and node.bindings.bind for the bind property specifically (in Dart, node.bindings.length and node.bindings.containsKey('bind')). The "bindings" property is really cool, you can reflect over all of the node's bindings.

rafaelw commented 11 years ago

Now that instances are only created for defined model values, I think this issue goes away. The .model now behaves how you wanted (i.e. assigning a model is reversable by assigning undefined).

Please re-open if I'm misunderstanding.

sethladd commented 11 years ago

Thanks for following up!

I opened https://code.google.com/p/dart/issues/detail?id=12687 to track on the polymer.dart side.