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

Bill Keese bill at dojotoolkit.org
Wed Mar 2 01:00:01 EST 2011


I checked over the patch (
http://bugs.dojotoolkit.org/attachment/ticket/12314/dijit-mvc-using-dojo.Stateful.patch)
once again, as best I could, given the size.   Thanks for adding in all the
documentation etc.   OK, here's my feedback:

 *1. location*

I don't want to put this in dijit, since dijit (until this point) has just
been for swing-ish widgets, not for framework stuff like MVC.   I'm willing
to debate it though if there are technical (ie, non-marketing) reasons it
makes sense in dijit.

*2. DataBindingMixin*

a) file structure

The root of the patch is _DataBindingMixin.js, which gets mixed into
_WidgetBase.   Current code for that is split across two files as:

dojo.declare("dijit._DataBindingMixin", null, { ... });

dojo.extend(dijit._WidgetBase, new dijit._DataBindingMixin());


It would be simpler like this:

dojo.extend(dijit._WidgetBase, {...});


You can still pull _DataBindingMixin.js into StatefulModel.js if you don't
want apps to need to explicitly dojo.require(DataBindingMixin).

I do think that _DataBindingMixin.js should be renamed to
WidgetBase-MVC-ext.js or something like that, since we have a naming
convention for modules that extend existing modules (in this case, adding
support to _WidgetBase for ref and binding attributes).


b) _watches, _updateBinding

_watches holds the connections from the widget to the StatefulModel object.
  Might wan't to name the variable something else as _watches is generic and
could mean watching anything.

Also there's also no reason to have the _watches: null attribute declaration
in the prototype.

You could slim down the code a bit in _updateBinding by getting rid of the
push()'s, just do:

this._watches = [
    this.get("binding").watch( ... ),
    this.get("binding").watch( .... ),
    ...
]


c) isValid()

Not sure if this can always function synchronously, seems like sometimes
you'd need to access the server to check validity.   I guess that's
something to think about for the future.

Also, It's a bit strange to have isValid() on all widgets rather than just
form widgets, but maybe there's a reason.


d) postCreate()

This is overwriting _WidgetBase.postCreate(), which is not good.   I guess
you can get away with it since _WidgetBase.postCreate() is empty, but you
shouldn't assume that.   Probably you need to dojo.connect() to
_WidgetBase.postCreate() or something like that.


e) _initControl()

This is run from postCreate(), yet it assumes that the widget is anchored to
the document.   You shouldn't assume that until startup() is called; in
particular it's not true for programatically created widgets ex:  new
dijit.form.TextBox().placeAt(...)

Also noticed the typeof binding.get === "function" code, wonder if it should
be dojo.isFunction(binding.get)


*3. StatefulModel*

a) any reason there's both a toPlainObject() and _toPlainObject() methods?

*4. NumberTextBox, ValidationTextBox*

These methods are now assuming a super-class isValid() method, but that
method only exists when _DataBindingMixin is included.   I guess we need to
define an empty isValid() method in _WidgetBase?   I don't know what happens
when you call this.inherited(arguments) and there's no such super-class
function, but even if it works, it seems fragile.

*5. dijit.mvc._Container*

This has >100 lines of cut-pasted code from _Templated and doesn't support
the new data-dojo-attach-point etc. syntax.   Can you do better?   See
dijit.Declaration, maybe you can have similar code.

Also, for ContentPane-ish widgets like this one it's typical to have a
content parameter for programmatic creation, ex: new dijit.mvc._Container({
content: "..."})   content can be a DOMNode, DOMfragment, etc.... or, I
notice the Repeat has a templateString parameter, maybe that should be
promoted to _Container.


*6. dijit.mvc._Generate*

a) Need to fix the spacing as per coding standards.
b) the declaredClass references here and in other parts of the code aren't
future-proof.   declaredClass will likely go away in 2.0.
c) if speed is an issue here consider replacing all the += by pushing
strings onto an array, and then ary.join("") at the end.   That would
require changing the functions to accept an output parameter, rather than
returning a value.
d) I don't know what b, r, and p stand for, those parameters (like all
parameters) should be documented

*7. dijit.mvc.Repeat*

a) more spacing issues
b) not sure what the name.toString().search(/^[0-9]+$/) === 0 test is for,
could it be /^[0-9]+$/.test(name.toString()) instead?

*8. test files*
*
*
Just looked briefly at one of these, but I noticed that you don't have hints
on your  doh.t(), doh.f(), doh.is() calls.   When one of those fails it's
hard to tell which one failed unless there's a hint string as the last
parameter.



On Tue, Mar 1, 2011 at 9:32 AM, Rahul Akolkar <rahul.akolkar at gmail.com>wrote:

> On Mon, Feb 28, 2011 at 6:56 PM, Bill Keese <bill at dojotoolkit.org> wrote:
> > Maybe we can talk about this in this week's Wednesday dojo meeting (6PM
> ET).
> <snip/>
>
> OK, I'll try to attend -- I need to check if I can move something else
> on my calendar.
>
>
> >   I have some comments but hopefully I'm not the only one.   (I'll send
> > comments on this thread when I get a chance, hopefully in the next few
> > days.)
> >
> <snap/>
>
> Please look at the latest patch on ticket #12314 (link below), as
> there have been some improvements as compared to the first patch.
>
> WRT other comments -- besides large amount of useful feedback at $work
> (i.e. IBM) -- I tend to get email once in a while from folks looking
> at the bits in Doug's ~, but they are mostly one or two liners about
> "is it in? how can I use it today?". So yes, community feedback++
> please.
>
> -Rahul
>
>
> > On Sat, Feb 19, 2011 at 1:43 AM, Rahul Akolkar <rahul.akolkar at gmail.com>
> > 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/20110302/ac868012/attachment.htm 


More information about the dojo-contributors mailing list