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

Rahul Akolkar rahul.akolkar at gmail.com
Wed Mar 2 14:37:35 EST 2011


Hi Bill,

Thanks a lot for your time, the detailed feedback is appreciated. More
inline below (moved email to plain text, so some formatting may be
lost).

On Wed, Mar 2, 2011 at 1:00 AM, Bill Keese <bill at dojotoolkit.org> wrote:
> 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.
<snip/>

Will be a bit of a recap -- an important part of many modern webapps
is presenting data and binding it to UI elements where applicable.
There is an existing gap between the edge of the data store and dijit
which creates potential confusion and loss of productivity for its
users, and dijit needs a dirt simple way to specify application data
bindings in order to close this gap (I say this as a user ofcourse).

What constitutes a framework is somewhat subjective. You mention size
of the patch, its mostly docs and tests -- I think its good return for
the amount of code for dijit as it makes dijit immediately useful for
many more applications. I see this as support for an application model
and one additional attribute for dijit to specify data binding
references. I don't see it to be any more a framework than, say,
dijit.form.Form or support for cached template-based widget
instantiation.

In the era of modern JavaScript, I don't know what constitutes a
technical reason for co-location. The work clearly builds on or works
with dijit interfaces (_WidgetBase, _Templated). I think its upon
dijit and its gatekeepers to address said data gap in a manner that is
plainly evident and visible to its users in order to remain
competitive beyond the shiny UIs (which are great, BTW, thanks all
involved).


> 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).
>
<snap/>

Correct, thats indeed the objective. The data binding mixin is not
something we'd want apps to explicitly dojo.require(). By "pull into",
I'm thinking you mean inline the source into the other file? Thats an
option.


> 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).
>
<snip/>

OK, that'd only matter if the file exists, given the above. BTW, I'd
prefer WidgetBase-DataBinding-ext.js, even though its longer.


> 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.
>
<snap/>

+1, _modelWatches maybe?


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

+1


> 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( .... ),
>     ...
> ]
>
<snap/>

+1, indeed nicer.


> 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.
>
<snip/>

Its synchronous, life is good as far as it is concerned. Its simply
checking the valid property on the StatefulModel (not going to the
server). That property itself may be refreshed from the server, but
its a model concern.


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

Validity could be an application concern for things other than form
widgets, though clearly its easiest for form widgets.


> 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(...)
>
<snip/>

OK, I'll look into (d) and (e) above.


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

+1


> 3. StatefulModel
> a) any reason there's both a toPlainObject() and _toPlainObject() methods?
<snip/>

At some point there was, but now you're correct that they can be folded in.


> 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.
<snap/>

Nothing happens, see line 190 (linking to a few lines before that for
context): http://bugs.dojotoolkit.org/browser/dojo/trunk/_base/declare.js?rev=23778#L186

That aside, yes, this was slightly tricky because there was no
corresponding isValid() in _WidgetBase whereas I think there should
be. Also, not all dijits check base class validity (where applicable)
before deciding their isValid() status and once its added to
_WidgetBase, they should (and thereby, the model validity will be
accounted for in cases where the data binding mixin is applied).


> 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.
<snip/>

Yes, its listed as a TODO in code -- it'd be good to remove that
duplication. The issue as I remember was that I wasn't able to reuse
_Templated directly as the way it deals with templates and template
caches is different from, say repeat, where the template is inline
(body). I'll take another look at dijit.Declaration, but ISTR some
other differences there.

Not supporting data-dojo-attach-point is inadvertent, I reused the
attach points and WAI code from _Templated without too close of an
inspection.


> 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.
>
<snap/>

May make sense to do so, will look into this.


> 6. dijit.mvc._Generate
>
> a) Need to fix the spacing as per coding standards.
<snip/>

Will inspect and fix.


> b) the declaredClass references here and in other parts of the code aren't
> future-proof.   declaredClass will likely go away in 2.0.
<snap/>

OK, any future-proof equivalent? I'll investigate or use a bit more of
the duck typing sauce if needed.


> 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.
<snip/>

Not been an issue so far, but sure.


> d) I don't know what b, r, and p stand for, those parameters (like all
> parameters) should be documented
<snap/>

Its binding, repeat heading and property respectively, what else ;-)
Right then, I'll add better names.


> 7. dijit.mvc.Repeat
>
> a) more spacing issues
<snip/>

Will inspect and fix.


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

Indeed, will change.


> 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.
>
<snip/>

+1, those need adding.

-Rahul


More information about the dojo-contributors mailing list