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 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#778#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

Search Discussions

Discussion Posts

Previous

Follow ups

Related Discussions

Discussion Navigation
viewthread | post
posts ‹ prev | 7 of 30 | next ›
Discussion Overview
groupdojo-contributors @
categoriesdojo
postedFeb 18, '11 at 11:43a
activeMar 11, '11 at 6:14p
posts30
users4
websitedojotoolkit.org

People

Translate

site design / logo © 2022 Grokbase