#14342 closed feature (fixed)
The parser should be able to create classes on the fly by mixing up various classes
Reported by: | cjolif | Owned by: | cjolif |
---|---|---|---|
Priority: | high | Milestone: | 1.8 |
Component: | Parser | Version: | 1.7.0 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
With mixins being more and more used as runtime plugins (dgrid, Treemap) I'd really like to leverage them in markup.
For now I have to do:
var MyTreeMap = declare([TreeMap, Keyboard, DrillDownUp]); <div data-dojo-type="MyTreeMap" ... ></div>
Ideally I'd like to be able to do something like:
<div data-dojo-type="TreeMap" data-dojo-mixins="Keyboard, DrillDownUp" ... ></div>
or
<div data-dojo-type="TreeMap,Keyboard, DrillDownUp" ... ></div>
This would be easy to implement but would create a dependency on dojo/_base/declare in dojo/parser.
See mailing list discussion here:
http://mail.dojotoolkit.org/pipermail/dojo-contributors/2011-November/025930.html
Attachments (3)
Change History (27)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
Milestone: | → tbd |
---|
comment:3 Changed 8 years ago by
I've just attached a patch that as suggested above relies on a dojo.declare() new createSubclass() method or on ComposeJS extend if that method is not found. I have followed the two attributes solution thus it introduces a new data-dojo-mixins attribute. That way if that attribute is not present the code path is identical to what we had before, only when data-dojo-mixins is present something specific is done. Note that maybe the attribute should named data-dojo-extend instead. Note sure.
That patch can still certainly be improved, but it is I think a first good step. Please let me know what you think. This is really something I would find very useful for 1.8 (find yet another case where we missed that recently).
Thanks.
comment:4 Changed 8 years ago by
Looks cool. Could you add a test for extend()?... it's impractical to include ComposeJS but you could define a simple non dojo.declare()'d "class" with an extend() method on it. It could even be a stub method, just enough to make sure that it was called with the right arguments, and the returned value was used to instantiate the widget.
Changed 8 years ago by
Attachment: | parser.html added |
---|
updated test with "ala" ComposeJS extend method
comment:5 Changed 8 years ago by
Thanks Bill, I added a new version of the test along the lines you suggested.
comment:6 Changed 8 years ago by
Milestone: | tbd → 1.8 |
---|---|
Owner: | changed from bill to cjolif |
Status: | new → assigned |
Type: | enhancement → feature |
It looks good to me, feel free to check in. Oh, one minor thing I noticed was that mixins.split(" ").join("");
is probably better to do as mixins.replace(/ /g, "")
comment:7 Changed 8 years ago by
Ok, thanks Bill. Actually I see the parser code changed significantly since I've made the patch 4 days ago so I will have first to merge this.
comment:10 follow-up: 11 Changed 8 years ago by
There is a logic error with this patch:
._instantiate()
there is the following code:
if(mixins){ var map = _ctorMap[type]; // remove whitespaces mixins = mixins.replace(/ /g, ""); ctor = (map && map[mixins]) || (map[mixins] = extend(getCtor(type), darray.map(mixins.split(","), getCtor))); }else{ ctor = getCtor(type); }
When _ctorMap[type]
is undefined, it throws at the second map[mixins]
when the _ctorMap[type]
hasn't been resolved yet. It doesn't manifest its self in the unit cases, but as I have been working on some changes into scan, I removed the ctor mapping that happens there and it manifested the issue. I suspect in certain cases when .instantiate()
is called directly, it would also throw.
comment:11 Changed 8 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Replying to kitsonk:
There is a logic error with this patch:
That's exact even though indeed I don't see it happening in a real use-case (_instanciate is not meant to be called directly right?). Anyway I'll commit a fix.
Thanks for reporting this.
comment:12 follow-up: 14 Changed 8 years ago by
While ._instantiate()
isn't, .instantiate()
can and does by some other libraries, although the unit test cases only recently added it, and this wouldn't trigger unless .instantiate()
was called with nodes that contained the data-dojo-mixins
attribute (because .instantiate()
doesn't call the getCtor()
). Don't know if it makes sense to add something to the .instantiate()
or data-dojo-mixins
unit test cases though. It is a pretty strange scenario I only stumbled upon by accident.
comment:14 Changed 8 years ago by
Replying to kitsonk:
While
._instantiate()
isn't,.instantiate()
can and does by some other libraries, although the unit test cases only recently added it, and this wouldn't trigger unless.instantiate()
was called with nodes that contained thedata-dojo-mixins
attribute (because.instantiate()
doesn't call thegetCtor()
). Don't know if it makes sense to add something to the.instantiate()
ordata-dojo-mixins
unit test cases though. It is a pretty strange scenario I only stumbled upon by accident.
Ok. Right I thought about _instanciate but not instanciate. The fix is committed anyway. Let me know if you still experience the issue, but from what I see you should not. If you have an enhanced test for that it would be good as well.
comment:15 follow-up: 16 Changed 8 years ago by
I really like this cool feature. One thing I noticed is that sometimes we may need to use data-dojo-type and data-dojo-mixins in an opposite way when we want to combine a main widget and a sub widget. This is because when there are conflicts, (e.g. baseClass property), the ones in the mixins win. Strictly speaking, "a sub widget" may not be "a mixin", but I think it is possible to augment a widget's capability by mixing-in a sub widget. For example, if TitlePane didn't have a capability of loading external content, you could mix-in ContentPane into it. (Not sure this example is appropriate, but actually dijit.TitlePane mixins dijit.layout.ContentPane.) The point is, in such a case, we need to write like
data-dojo-type="ContentPane" data-dojo-mixins="TitlePane"
to have the main TitlePane widget win. Doesn't it look a bit odd?
comment:16 Changed 8 years ago by
comment:17 Changed 8 years ago by
Well, no, I don't think "extend" can avoid the confusion.
data-dojo-type="ContentPane" data-dojo-extend="TitlePane"
Assuming TitlePane is the main class, it still looks strange, doesn't it?
I still haven't deeply looked at the code, but from the behavior I saw, if I write a markup like this:
data-dojo-type="Main" data-dojo-extend="Sub1, Sub2"
although my expected result is something like this:
declare("Main", [Sub1, Sub2], {...})
but what I actually get looks something equivalent to this.
declare("", [Main, Sub1, Sub2], {...})
(Please correct me if I am wrong.)
As you know, if you do multiple inheritance and have some conflicts, the last class in the list wins. In the above example, Sub2 wins, and overrides Main and Sub1. That's just what I saw from a quick experiment. Maybe a possible workaround would be
data-dojo-type="Sub1" data-dojo-extend="Sub2, Main"
but that's weird...
Also, this is just my guess, but possible problem of the above workaround is Main's stopParser flag is probably not effective, since it's scanned before mixing-in.
comment:18 Changed 8 years ago by
ok, I started to understand the meaning of this feature. From the name of "data-dojo-mixin", I thought this was something that creates this for me.
declare("Main", [Sub1, Sub2], {...})
But in fact it is equivalent to applying lang.extend() for each sub module to the main module, right?
lang.extend(Main, new Sub1); lang.extend(Main, new Sub2);
ok, then, as you mentioned, "data-dojo-extend" may be a more proper naming...
Also, for my TitlePane example scenario above, the second syntax, i.e. comma delimited types in data-dojo-type, might be better, because in that case I could write as below to give Main the highest precedence.
data-dojo-type="Sub1, Sub2, Main"
comment:19 Changed 8 years ago by
ykami,
The result for Dojo classes is the following:
var MyClass = declare([Main, Sub1, Sub2]);
(see http://livedocs.dojotoolkit.org/dojo/parser#parser-parameters)
That's how dgrid or Treemap components use mixins to plugin optional behavior in their component and the main reason why this was implemented like this.
For those using ComposeJS:
var MyClass = Main.extend([Sub1, Sub2]);
To come back to your problem, from what I understand that mechanism does not suit your needs because you have cases where the Main class doesn't want to be overriden by its mixin classes. In order for me to better understand the implications of this and possibly adjust the implementation do you have a concrete example of where you would need that?
Thanks.
comment:20 Changed 8 years ago by
var MyClass = declare([Main, Sub1, Sub2]);
ok, probably this is fine even for the case I mentioned, that is, cases where we want to make Main inherit from Sub1 and Sub2, rather than extending Main with Sub1 and Sub2.
A (small) problem is that, in order to get this result,
var MyClass = declare([Sub1, Sub2, Main]);
we need to write this way:
data-dojo-type="Sub1" data-dojo-mixins="Sub2, Main"
We can certainly get what we want, but the semantics does not seem to make sense.
How about allowing both syntax in the description above?
Then, in the example case, we can write more straightforward way like this:
data-dojo-type="Sub1, Sub2, Main"
comment:21 Changed 8 years ago by
I'm against having two syntaxes to do the same thing. We could switch to just a data-dojo-type syntax (this is also what Kris wanted), but I'm not quite buying the argument from Kamiyama-san's example. The example was:
data-dojo-type="ContentPane" data-dojo-extend="TitlePane"
Besides the fact that TitlePane already extends ContentPane, this example wouldn't work because both ContentPane and TitlePane extend _WidgetBase. I think what you would need (assuming that TitlePane didn't extend ContentPane already) is a ContentPaneMixin class that didn't extend _WidgetBase, and didn't define baseClass. Then you would do:
data-dojo-type="TitlePane" data-dojo-mixin="ContentPaneMixin"
And that's clear, right?
BTW, if you want to split lists (possibly just "lists" with one item), myString.split(/, */)
is the canonical way.
PS: Theoretically it's possible to combine multiple classes that all extend _WidgetBase. According to C3MRO it should work. I thought I had problems before but maybe I was just confused. In any case though it's not how we typically design class hierarchies, at least not in dijit. So I still think the data-dojo-mixin="ContentPaneMixin"
makes sense.
comment:22 Changed 8 years ago by
In my experiment, the example works fine. (BTW, dijit.Dialog also inherits from ContentPane.) I liked that idea, but I can understand what you mean. ContentPaneMixin makes sense. OK, I'll think about the mobile parser based on the current spec.
Summary from mailing list:
syntax
Kris voted for the second syntax, saying:
The counterargument is that ComposeJS users typically *will* distinguish between the "base" class and "mixins":
dependence on dojo.declare()
Kris also expressed concerns that the feature shouldn't be limited to classes created with dojo.declare(), or even introduce a parser dependency on declare.js. Rather, he wants it to [also] work with classes from ComposeJS, or any class with an extend() method matching the API of ComposeJS classes.
Although dojo.declare()'d classes already have an extend() method with a different API, we could address the issue with a duck-typing solution, by making dojo.declare() add a new method to it's classes (ex: createSubclass()) matching ComposeJS's extend() API.