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

Alex Russell alex at dojotoolkit.org
Wed Mar 2 14:55:53 EST 2011


On Mar 2, 2011, at 11:37 AM, Rahul Akolkar wrote:

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

FWIW, I agree. Evolving to handle at least the protocol by which data can plug into widgets is a natural place for Dijit (or any UI framework) to go.

> 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
> _______________________________________________
> 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