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

ben hockey neonstalwart at gmail.com
Fri Mar 4 10:00:33 EST 2011


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

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.

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
> _______________________________________________
> dojo-contributors mailing list
> dojo-contributors at mail.dojotoolkit.org
> http://mail.dojotoolkit.org/mailman/listinfo/dojo-contributors


More information about the dojo-contributors mailing list