[dojo-contributors] MVC support for dijit based on dojo.Stateful

Rahul Akolkar rahul.akolkar at gmail.com
Fri Mar 4 11:58:42 EST 2011


Hi Ben,

Inline.

On Fri, Mar 4, 2011 at 10:00 AM, ben hockey <neonstalwart at gmail.com> wrote:
> hi rahul,
>
> i have another 2 quick comments/questions about StatefulModel.
>
> if you declare the default values for relevant, required, valid,
> readOnly and value on the StatefulModel class then would it be possible
> to remove _decorate?  the new models would inherit these values rather
> than need to have them decorated and any new values passed in via the
> constructor will replace these defaults - that is something already
> provided by the mixin in dojo.Stateful's postscript.  an added
> side-effect is now the existence of the "meta" properties and exactly
> which ones exist are easier to find in the code - ie self-documenting.
>
>     dojo.declare('dijit.StatefulModel', dojo.Stateful, {
>         relevant: true,
>         required: false,
>         valid: true,
>         readOnly: false,
>         value : '',    // maybe null instead?
>
>     new dijit.Stateful({ data: obj });    // inherits "meta" properties
>     new dijit.Stateful({ data: obj, required: true });    // override
> required
>
<snip/>

+1, this will be in patch++.

Bit of a sidebar, but I'd like to note that the intent is not for
these properties above to be a closed set, though the ones above are
the most commonly used. Indeed, the expectation is that applications
may also leverage application-specific meta-data for suitable purposes
using the model. As an example, say I have versioned data, and the UI
needs to be more intelligent based on this version information,
manifested as a "version" property on appropriate data model nodes.
Ofcourse, we'll need to flesh out the details with a demo/test or two.
To enable this, we have generic methods provided by the model like
bindProperties() in addition to the most common specific ones that
have more universal appeal.


> also, it looks like _createModel isn't supposed to work with a store any
> longer.  is that correct?  i assume if you're using a store, you're
> supposed to use the getStatefulModel but the StatefulModel constructor
> and _createModel still have code/comments referencing a store.
>
<snap/>

You are correct, that is a loose end in refactoring. The factory
method makes it easier to deal with immediate values or promises in a
manner that is aligned with how stores themselves work. Perhaps
_createModel would also be made to accomodate the use of stores, I'll
dig into the code a bit.

This is good feedback, thanks.

-Rahul


> thanks,
>
> ben...
>
> On 3/3/2011 8:39 PM, Rahul Akolkar wrote:
>> Hi Ben,
>>
>> Inline below.
>>
>> 2011/3/3 ben hockey<neonstalwart at gmail.com>:
>>> i'm really interested in where this is heading.  thanks for the work you've
>>> done on it.  i have quite a lot of comments but i mean for them to be
>>> constructive so please take them as such.
>> <snip/>
>>
>> Absolutely, I appreciate the time you've spent on providing this feedback.
>>
>>
>>> 1. NameSpace
>>>
>>> a) wrt to bill's comments about namespace, a small refactoring of
>>> bindModelToView in dijit.StatefulModel so that it takes a Stateful control
>>> object rather than a control_id string and removing the line that extends
>>> dijit._WidgetBase would make dijit.StatefulModel completely free of dijit.
>>> it simply becomes an extension of dojo.Stateful that is unaware of dijit.
>>> there might be some value in this.  i'll leave it up to others to figure out
>>> where the code should live but i just wanted to point this out for
>>> consideration.
>>>
>> <snap/>
>>
>> I've gone back and forth on this, I don't disagree that there'd be
>> some value to leaving _WidgetBase out of StatefulModel. To understand
>> the back and forth I speak of, lets consider this throw-away hello
>> world data binding example:
>>
>>    <div dojoType="dijit.StatefulModel" jsId="model"
>>         data="{ hello : 'Hello World' }"></div>
>>
>>    <div dojoType="dijit.form.TextBox" id="hello"
>>         ref="model.hello"></div>
>>
>> What dojo.require's would a user infer just by looking at the above?
>> Sure, we could doc it that you must use said mixin to use the ref
>> attribute here and expect them to RTM on it, but I'd like it to be
>> extremely hard to get wrong ;-) With that thought, the mixin is
>> applied in the StatefulModel.js file, which also makes it an opt-in
>> i.e. if you're using this model class, then we'll make sure your
>> dijits can bind to such a model class.
>>
>> WDYT?
>>
>> Ideally, what I want to say is: if there is a ref attribute on any
>> dijit then apply the data binding mixin to _WidgetBase. However, thats
>> trickier (ideas welcome ofcourse).
>>
>> This is related to why this work squarely belongs in dijit IMO.
>> Absolutely no more code should be needed for binding dijits to
>> application data.
>>
>> BTW, +1 that the control_id is a bit of a transgression and it'll be
>> as you suggest in patch version++ (accepting a Stateful control
>> instead).
>>
>>
>>> 2. dijit.StatefulModel
>>>
>>> a) small issue: it feels more natural to me to have source before target in
>>> function signatures.  as an example, consider dojo.connect and
>>> dojo.subscribe - they have source (provider) followed by target (consumer).
>>>
>> <snip/>
>>
>> +1, will make the change.
>>
>>
>>> b) all the calls to watch in the bindXXXX functions should probably be
>>> returned so they can be released appropriately.
>>>
>> <snap/>
>>
>> +1, good point.
>>
>>
>>> c) bindValue, bindRequired, bindReadOnly, bindRelevant, bindValid and
>>> possibly bindProperties and bindInputs have an opportunity for some code
>>> reuse/reduction.  without much change, it looks like you could reuse
>>> bindProperties to implement bindValue, bindRequired, bindReadOnly,
>>> bindRelevant and bindValid.  for example:
>>>          bindValue: function (target, source, func) {
>>>              return this.bindProperties(target, 'value', source, 'value',
>>> func);
>>>          },
>>>
>>>          bindRequired: function (target, source, func) {
>>>              return this.bindProperties(target, 'required', source, 'value',
>>> func);
>>>          },
>>>          etc.
>>>
>> <snip/>
>>
>> +1, back story here is a bit of a chicken and egg one, they weren't
>> added in the order that would naturally lead to reuse. Now its
>> apparent as you illustrate above. Will do this.
>>
>>
>>> d) further, is it possible that either the source or the target is probably
>>> the instance of the StatefulModel and so wouldn't need to be passed into all
>>> these functions?  if it was, i would think it would probably be the source.
>>> also, bindValue, bindRequired, bindReadOnly, bindRelevant, bindValid all
>>> follow the same pattern and could be generalized (even more than mentioned
>>> above) into a single bindMeta (open to different name) function.  with that
>>> in mind, you may then have the opportunity to do something like this:
>>>
>>>          // replaces bindProperties
>>>          bind: function (prop, target, target_prop, func) {
>>>              return this.watch(prop, function (p, old, value) {
>>>                  // passing all args to func simply because they're available
>>> - might be useful
>>>                  target.set(target_prop, func.apply(this, arguments));
>>>              });
>>>          }
>>>
>>>          // replaces bindValue, bindRequired, bindReadOnly, bindRelevant,
>>> bindValid
>>>          bindMeta: function (target, prop, func) {
>>>              // prop could be one of "value", "required", "readOnly",
>>> "relevant" or "valid" but doesn't need to be enforced
>>>              return this.bind('value', target, prop, func);
>>>          }
>>>
>> <snap/>
>>
>> Right, I agree that seems better. Caveat is this would break any
>> existing code, because this "protected" model API will change, see
>> before and after:
>>
>>    this.bindValid(this.foo, this.bar, this.validityFunc);
>>
>>    this.foo.bindValid(this.bar, this.validityFunc);
>>
>> But that may be a small price to pay in the overall context of where
>> this work is :-)
>>
>>
>>> e) i feel like the dependency between _DataBindingMixin and StatefulModel
>>> should be removed.  with a few small changes StatefulModel can be free of
>>> any references to dijit and exist without _DataBindingMixin.  it would mean
>>> that an app would have to explicitly require _DataBindingMixin but it would
>>> also mean that StatefulModel could exist without carrying the extra baggage
>>> of DataBindingMixin.
>>>
>> <snip/>
>>
>> Please see above response to 1a.
>>
>>
>>> f) _saveToStore - there are a number of times where you have
>>> dojo.forEach(..., dojo.hitch(this, function (...) { }));  a couple of things
>>> to note here.  |this| isn't referenced in these functions and so isn't even
>>> needed.  as an fyi, if you really did need it, you can provide it as 3rd
>>> param to dojo.forEach - dojo.forEach(..., function() { }, this);
>>>
>> <snap/>
>>
>> Will adjust.
>>
>>
>>> 3. _DataBindingMixin
>>>
>>> a) should isValid return true rather than "true"?
>> <snip/>
>>
>> Yes :-)
>>
>>
>>> b) in postCreate you have a pThis but i think it might not be necessary.
>>> where you use it, the context should already be what you'd want.
>> <snap/>
>>
>> Correct.
>>
>>
>>> c) _initControl, _setupBinding and _updateChildBindings - can _initControl
>>> and _setupBinding be merged and have _updateChildBindings call the merged
>>> function?
>> <snip/>
>>
>> No, binding can be set at dijit initialization (by reading the ref
>> attribute or data-dojo-props property) or may be changed
>> programmatically later. The bits in _initControl aren't needed when
>> the binding is programmatically updated, which is why we have the
>> above distinction.
>>
>>
>>> d) _setupBinding - would it improve performance to try the eval first - ie,
>>> try the eval and only then walk the DOM if the eval didn't produce anything?
>> <snap/>
>>
>> Existing data-binding semantics for the ref attribute are as follows:
>>   * Find if you have a data-bound parent, if so use a relative expressive
>>   * If no data-binding parent, then treat as expression to eval
>>
>> Obviously, the above approach will change semantics to:
>>   * Try as expression to eval, if meaningful (i.e. you get a model node) use it
>>   * If meaningless, look for a parent to resolve against
>>
>> The former seems more structured, the latter seems more trail-n-error.
>> I do not know if the performance will always be noticeably different
>> either. For example, if there are many relative data bindings, it may
>> actually hurt than help. So, if I have the following application data
>> structure that I create my model named "model" from:
>>
>>   { foo : { bar : { baz : { freddy : { frog : "prince" } } } } }
>>
>> One could image a view like so (ofcourse, one would only produce a
>> comparable nesting as below if each level has enough other pieces of
>> data to bind to other dijits so that you wouldn't want to collapse
>> this into one "model.foo.bar.baz.freddy.frog" expression):
>>
>>   <div dojoType="dijit.mvc.Group" ref="model.foo">
>>    <div dojoType="dijit.mvc.Group" ref="bar">
>>     <div dojoType="dijit.mvc.Group" ref="baz">
>>      <div dojoType="dijit.mvc.Group" ref="freddy">
>>       <div dojoType="dijit.form.TextBox" ref="frog">
>>
>> In the above case, the current approach will be more performant. So
>> the performance answer here is also a function of the application
>> itself, and in the absence of a clear winner in all scenarios, the
>> more structured approach may be better.
>>
>>
>>> e) also, _updateChildBindings calls down into _setupBinding on it's
>>> children.  those children are then going to walk the DOM possibly back up to
>>> the parent (who just invoked the call) to find their binding.  you could
>>> have the parent pass the binding to the child if the parent has/is it by
>>> having _setupBinding take an optional binding  _setupBinding: function (/*
>>> Stateful?*/ binding) { ... }
>> <snip/>
>>
>> +1, I will look into this.
>>
>>
>>> f) _updateBinding - cache your reference to the binding.  rather than
>>> calling this.get('binding') ~ 10 times, call it once and cache it in a local
>>> var.  you could also cache a reference of this._watches to reduce lookups
>>> and help the code compress a little better when it's built.
>> <snap/>
>>
>> +1, too much cut-n-paste there. Bill also suggested a better code pattern here.
>>
>>
>>> g) _updateBinding - could some of this code be reduced by leveraging the
>>> binding functions provided by the StatefulModel rather than calling watch?
>>> or alternatively, could many of the bindXXX functions be removed from
>>> StatefulModel?  it seems to me that there's probably some redundancy here
>>> somewhere.
>> <snip/>
>>
>> The two serve separate purposes as follows:
>>   * The watches in _DataBindingMixin are setting up the bind between
>> the model and the dijit (example: bind this textbox to the foo node
>> and make sure its value, required etc. properties stay in sync with
>> that node).
>>   * The bindXXX functions in StatefulModel are setting up binds between
>> nodes of the model - these are used for expressing application data
>> model constraints (example: in my online taxes application, which
>> ofcourse I'll build using this MVC work, if I check the "Single"
>> filing checkbox, then the spousal information UI is not "relevant"
>> i.e. do not show it). Therefore, "bindRelevance".
>>
>>
>>> i've certainly got more to add, i've only commented on 2 of the files and
>>> even those i have more comments about, but maybe i should wait for another
>>> iteration of the patch.  i don't have much more time right now and along
>>> with what bill has already commented on, there's a lot to consider.
>>>
>> <snap/>
>>
>> These are the two most important files, so I'd say you have covered
>> the core, thanks. Yes, there will be a new patch coming soon-ish based
>> on feedback so far.
>>
>>
>>> feel free to ask for clarification on anything and it's also quite possible
>>> that there are cases where i've misunderstood your intention so also feel
>>> free to defend anything.
>>>
>> <snip/>
>>
>> Thanks, I won't hesitate to defend things that are worth defending :-)
>>
>> -Rahul
>>
>>
>>> thanks,
>>>
>>> ben...
>>>
>>>
>>> On 2/18/2011 11:43 AM, Rahul Akolkar wrote:
>>>
>>> Hi all,
>>>
>>> Some of you may recollect the MVC samples from DDD in October, or from
>>> a prior discussion here and this introduction to the concepts [1].
>>>
>>> We now have a patch attached to this ticket [2] in Trac that provides
>>> this set of features to dijit as an opt-in. This work uses
>>> dojo.Stateful to build the MVC aspects upon; further details are in
>>> the description on the ticket and the patch itself.
>>>
>>> All feedback is welcomed :-)
>>>
>>> -Rahul
>>>
>>> [1] http://doughays.dojotoolkit.org/dojomvc/
>>> [2] http://bugs.dojotoolkit.org/ticket/12314


More information about the dojo-contributors mailing list