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

ben hockey neonstalwart at gmail.com
Thu Mar 3 17:00:15 EST 2011


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.


*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.

*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).

b) all the calls to watch in the bindXXXX functions should probably be 
returned so they can be released appropriately.

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.

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);
         }

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.

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);

*3. _DataBindingMixin*
a) should isValid return true rather than "true"?
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.
c) _initControl, _setupBinding and _updateChildBindings - can 
_initControl and _setupBinding be merged and have _updateChildBindings 
call the merged function?
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?
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) { ... }
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.
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.

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.

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.

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
> _______________________________________________
> dojo-contributors mailing list
> dojo-contributors at mail.dojotoolkit.org
> http://mail.dojotoolkit.org/mailman/listinfo/dojo-contributors
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.dojotoolkit.org/pipermail/dojo-contributors/attachments/20110303/86cb16c9/attachment.htm 


More information about the dojo-contributors mailing list