Skip to content

[wip] Data binding, take two #386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed

[wip] Data binding, take two #386

wants to merge 8 commits into from

Conversation

lsmith
Copy link
Contributor

@lsmith lsmith commented Jan 3, 2013

Now more API driven, less automated from instantiation configuration. Split into two modules, one for the API, the other for progressive enhancement via HTML_PARSER.

Binding must always be done explicitly using _bindAttrs({ <bindings> }).

Again, this PR is for discussion, but I will fill out test suite if the design and basic implementation seem sound.

Needs unit tests and docs.
Now powered by _bindAttr method, and doesn't rely on instantiation
configuration. All binding must be done explicitly, but the data-bind-html
module provides HTML_PARSER support for a 'bindings' property on the
instantiation config object.
@juandopazo
Copy link
Member

How about binding to an actual Y.Model? For widgets we could standardize .data as in DataTable. That would make syncing to the server a lot easier and it would simplify selecting which attributes to sync.

@lsmith
Copy link
Contributor Author

lsmith commented Jan 3, 2013

The attribute host defaults to this, so augmenting a Model instance or Base.create/mixing it into a Model class will work, but the UI elements that are bound must be identified, even if the attributes are automatically determined. Plus there's that issue of MVC purity being completely ignored by combining the MV with code specifically for the UI in the model class. That's what Widget is doing today, and the discomfort with that mix was part of the impetus for the App framework family of components.

Am I misunderstanding your suggestion?

@rgrove
Copy link
Contributor

rgrove commented Jan 3, 2013

My first impression is that the name Y.DataBind implies an all-around data binding solution, but the implementation seems to only care about form fields. Is this intended to be a solution for binding data to form fields, or for binding data, period? If the latter, then I think it needs to be simpler and less verbose. If the former, then I think it needs a less greedy name. 😄

@lsmith
Copy link
Contributor Author

lsmith commented Jan 3, 2013

@rgrove The name was a compromise to get it out of the way so I could get to the coding part. Can you be specific about the verbosity? It is primarily intended for binding to form fields, but supports binding to any element, though not without specifying at least a field setter. I'd considered making the default setter and getter appropriate for non-form fields, but focused on form fields because of the bidirectional relationship. Thinking about it now, and looking at ticket http://yuilibrary.com/projects/yui3/ticket/2532124 and the linked gist https://gist.github.com/e0fbadb4b3b17733fbca, I can see the benefit of unidirectional binding. I'll make those changes, but am still curious about the verbosity you'd like removed.

@rgrove
Copy link
Contributor

rgrove commented Jan 3, 2013

Verbositywise, what I mean is that data-bind-attr is long and ugly in markup, and having to specify a custom setter for non-form fields makes your JS more verbose if you use this as a general-purpose data binding solution.

I have other reservations about this, but they're more abstract feelings than concrete items. I don't think I've ever met a data binding solution that doesn't make me feel like there must be a better way of doing things, but I haven't yet figured out what that better way is. I've been meaning to take some time and do some deep thinking about data binding, but haven't gotten around to it yet.

I think my ideal data binding solution would be simple and pragmatic and would feel magical and effortless. This one doesn't feel bad, but it doesn't feel magical or effortless either, so it's not giving me warm fuzzies. Which isn't criticism; it's just me being weird, probably.

@lsmith
Copy link
Contributor Author

lsmith commented Jan 3, 2013

data-bind-attr is unnecessary unless enhancing from existing markup, but I'm open to shorter names.

I'm increasingly in favor of making the default getter/setter for non-form fields, and have the bind operation fork on nodeName to the bidirectional logic for form fields. This could trim the core significantly by splitting off the implementation of the form logic into a separate module, though I'm not yet convinced of the value in that additional step.

I get what you're saying about uneasiness. It doesn't feel perfect to me, either, but I think that has more to do with:

  1. A want to have template tokens trigger binding
  2. A want for binding/getter/setter logic forking based on selector rather than tag name
  3. A concern over leaking subscriptions when UI is destroyed
  4. A concern over memory consumption as a result of individual DOM event subscriptions for form fields rather than delegated subscriptions

That said, I think the central idea/implementation is workable and flexible enough to build any missing magic on top of, so I think it's worth moving forward barring objections from others.

If you have an idea of what consuming the the magic, effortless API would look like, that would probably help identify what's missing.

@clarle
Copy link
Collaborator

clarle commented Jan 3, 2013

Forking on nodeName does seem to be what a lot of the other frameworks supporting data-binding do nowadays. Here's how Batman (https://github.com/Shopify/batman) handles each of the individual tags:

  • <input type="checkbox">: the binding will edit the checked property of the checkbox and populate the keypath with a boolean.
  • <input type="text"> and similar, <textarea>: the binding will edit the value property of the input and populate the keypath with the string found at value.
  • <input type="file">: the binding will +not+ edit the value property of the input, but it will update the keypath with a host File object or objects if the node has the multiple attribute.
  • <select>: the binding will edit the selected property of each tag within the match the property at the keypath. If the has the multiple attribute, the value at the keypath can be an array of selected values. You can also use data-bind-selected bindings on the individual options to toggle option selectedness.
  • All other tags: the binding will edit the innerHTML property of the tag and will not populate the keypath.

I think it's useful, since you'll probably be handling the most common use cases there. I don't think you can have magic and effortless together with data-binding, though. It's either you abstract as much as you can, or not at all.

@rgrove
Copy link
Contributor

rgrove commented Jan 3, 2013

@lsmith If you see this as a foundational implementation that magic can be built on, then it seems like it should be split into two modules: one for unidirectional binding (data to DOM) and another that adds bidirectional sync (DOM to data). Most of the use cases I'd personally consider it for are unidirectional, which I think is why I feel uneasy about all the bidirectional complexity being baked in.

I'll keep thinking about what my ideal magical, effortless API might look like, but I can tell you it would definitely have to be something that makes it easy to add or remove attributes, attribute values, and class names based on data changes, in addition to setting simple element values. This stuff could be done with the current API, but it wouldn't be easy or effortless.

* 'data-bind' has core infrastructure, no out-of-the-box form field support,
  though has capability for writing and reading from DOM
* 'data-bind-html' for progressive enhancement (data attr name not changed)
* 'data-bind-form' bidirectional binding out-of-the-box for form fields
@lsmith
Copy link
Contributor Author

lsmith commented Jan 4, 2013

We haven't talked scope yet, but this commit incorporates non-naming related feedback so far. 'data-bind' module is smaller, and is focused on writing to non-form elements, though the code is there to read from them, and the binding logic is set up to support custom binding by either configured binding type or by DOM nodeName. Form element support, including bidirectionality is in 'data-bind-form'. The API didn't change.

@param {String} attr Attribute name
@param {Object} config Bind configuration
**/
_bindCheckbox: bindClick,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this handle keyboard changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, keyboard changes fire the click event.

@mattparker
Copy link

In Luke's comment about validation the answer seems to require replicating a lot of what Attribute is doing - republishing events, mimicking methods etc. And there's getters and setters and things again.

Now I don't know the inner workings of Attribute so I don't know if this is a possibility, but could this actually be a mixin or something to Attribute itself, rather than a separate layer between the DOM and the Base owning attribute?

So in your ATTRS you'd be able to add a config value 'bind' : Node | NodeList | selector and it'd all happen from there.

Now I guess that most of the time you wouldn't actually want to do that because it's linking the DOM too closely to the widget/model/whatever definition. So presumably you'd actually do myModel.modifyAttr('name', {bind: "'#inputName"});.

Now that's pretty clunky, I admit. But it wouldn't take much to add a sugar method myModel.bindToDom({name: "#inputName", email: "#inputEmail"}); or something.

Doing this would also require adding support for parser and formatter on Attribute.

Just thinkin'...

@lsmith
Copy link
Contributor Author

lsmith commented Jan 15, 2013

I previously started down the road of an Attribute mixin that supported this, but stopped because it violates the notion of decoupling the model from the view. Up to the point of validation failure, the relationships can be built on events, not object/behavior mutation.

This raises the question of whether the binding should be something that the model performs on itself (aka hosts the methods), or other code, such as a view, performs on the model/attribute(s) "from outside". My position is "from outside" for academic reasons, but it certainly feels more hacky/invasive to change the model behavior from outside than from within. Maybe there's a clean (or clean enough) way to hook into the Attribute lifecycle that hasn't occurred to me.

I'd like @sdesai's opinion on this.

@mattparker
Copy link

One other thought: could this also work for Widgets (or anything else that has get(), set() and fires 'change')? For example, binding a Slider to a Model attribute. I know it's not hard to do anyway, but it might be nice if either 1. this could do both, (model.bindAttr('numberOfThings', mySlider, 'value')) or 2. a separate widget binder module could use the same API. If 2, then it might be better to use something more generic (say 'Control') instead of 'UI' in method names?

@sdesai
Copy link
Contributor

sdesai commented Jan 16, 2013

I'll take a look at it, and post back

bindings: function (srcNode) {
var bindings = {};

srcNode.all('[data-bind-attr]').each(function (node) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look for 'data-bind-type' for config.type

@mattparker
Copy link

There was a question in the hangout about whether this should require Attribute (or AttributeObservable), even though it doesn't use it itself: if just hooks into something Attribute-like (i.e. with on, get, set).

My view: no. It doesn't require it so don't require it. It's a documentation thing: Y.DataBind binds to an Attribute-like instance, or expects something that implements (a subset of) the Attribute interface.

@sdesai
Copy link
Contributor

sdesai commented Jan 23, 2013

Regarding the core DataBind class and goal of this implementation:

I think it still needs to leave the generic DataBind space (both in terms of module name/class name, but also in terms of anything generic it does which could be re-used) to non-UI based DataBinding. That is, we may want to bind a collection of Attributes to a remote data source, or local storage, or a database table etc. It seems like that should be the base layer, and then we can introduce the idea of binding to something in the DOM (UI DataBinding) on top of that.

Regarding extending ATTRS meta-data:

In general I think that would be nice to consolidate all meta-data around ATTRS in one place [ in fact I've been thinking it would be nice to normalize HTML_PARSER to use that same infrastructure also for Widget ]. I don't think it implies a tight binding between State/Model and how they're rendered. That's still decoupled by the implementation (the code which sets the state of an attribute is still in a different method from the code which updates the UI). You can think of it as sugar for event binding. It's really just controller sugar which binds the State and the UI side of things.

However I'm not sure it has any value in practice for this particular set of functionality when it's mixed in free form - that is, wouldn't you set the binding differently for each instance for the 80% use case anyway? Or would there be value in having bindings defined statically (that is using .class, or attributeName->formFieldName type mapping - anything not instance specific). For HTML_PARSER, it's a little more clear cut - since the Widget is likely controlling the markup also - so it can makes sense to define static .class based lookups within the contentBox.

If there is value in static definitions, we can go from there to see what it would take to have mixins add Attribute meta-data properties. All in all though, calling bindAttrs({}) in an initializer/bindUI phase isn't so bad either without needing to add additional complexity (although the name of the method should indicate what you're binding them to - Form/UI etc. - bindAttrs is too generic - for reasons mentioned above) as a first pass.

NOTE: Widget already has a _bindAttrUI, and _syncAttrUI - which does some of this - which we'd need to reconcile. The goal was to make those protected for custom widget use. They're mainly used a utils for Widget itself currently.

Regarding Validation Events:

This has come up before. I think it's a useful feature to have - especially in this context (form binding), but I'm hesitant to add more events to the Attribute setting stack, until we get through some of the performance optimization we're looking at to fast-path "no-listener" Events (which I'm looking at currently), at which point, hopefully, we should be able to add validation events at "zero cost" if no-one is listening for them. In the interim, we can see if we can hack something up, with say a "did not get set" hook for implementations to override in AttributeCore/Observable's final "set" implementations. That seems like a decent/trivial/low performance cost middle-ground to evaluate.

Regarding "could this also work for Widgets":

I think we should code these mixins to work on the lowest common denominator of functionality - that is, it should work on "anything which has AttributeChange events" in it's base form. A more specialized version of "things with Attributes" could add additional value as required. So, for example, the code you have in your BoundWidget initializer/bindUI example, could be genericized and moved into a WidgetUIDataBind implementation.

Thats all I had at the high level.

@ericf
Copy link
Member

ericf commented Feb 12, 2013

@lsmith are we shooting for inclusion in the next release? If so, the deadline is EOD today for getting this pull request ready for review.

@lsmith
Copy link
Contributor Author

lsmith commented Feb 12, 2013

There's too much discussion outstanding for inclusion in this release.

@davglass
Copy link
Member

Updated title with [wip] to show it's still active.

js/bindable.js needs (only) bindAttr recoded. Then dom.js needs to add support
for unidirectional DOM binding.
@lsmith
Copy link
Contributor Author

lsmith commented Apr 18, 2013

Closing this as it's getting far behind the tip of tree and I don't have time in the short term to work on it. I will revive the work after my eventx work concludes. This will likely be in a couple months time.

@lsmith lsmith closed this Apr 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants