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

Rahul Akolkar rahul.akolkar at gmail.com
Thu Mar 3 20:39:20 EST 2011


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