The parser should be able to create classes on the fly by mixing up various classes

[cjolif]

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

[bill]

Summary from mailing list:

syntax

Kris voted for the second syntax, saying:

We don't have to distinguish between the "base" class and the "mixins" with dojo.declares. If we are going to add this feature to the parser (and I think it is a good idea), why should we have to in markup? It should be done by simply allowing comma delimited types in data-dojo-type:

The counterargument is that ComposeJS users typically *will* distinguish between the "base" class and "mixins":

TreeMap.extend(Keyboard, DrillDownUp)

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.

[cjolif]

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.

[cjolif]

draft patch

[bill]

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.

[cjolif]

updated test with "ala" ComposeJS extend method

[cjolif]

Thanks Bill, I added a new version of the test along the lines you suggested.

[bill]

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, "")

[cjolif]

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.

[cjolif]

In [27667]:

fixes #14342. Implements data-dojo-mixins in parser for declare & composeJS without forcing dojo/_base/declare dependency. !strict.

[cjolif]

In [27668]:

refs #14342. Add a missing file.

[Kitson Kelly]

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.

[cjolif]

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.

[Kitson Kelly]

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.

[cjolif]

In [27680]:

fixes #14342. If parser._instanciate is called outside of parser.parse/scan context the ctorMap[type] might not be constructed yet. Make sure it is constructed before key/pair values are set. !strict.

[cjolif]

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

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.

[ykami]

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?

[cjolif]

Replying to ykami:

In comment 3, I wrote:

Note that maybe the attribute should be named data-dojo-extend instead.

That would avoid the notion of mixins and just tell that we extend data-dojo-type (which is true in any case), even if that is not with a mixin?

Any other opinion?

[ykami]

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.

[ykami]

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"

[cjolif]

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.

[ykami]

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"

[ykami]

A patch to allow type list

[bill]

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.

[ykami]

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.

[bill]

In [28339]:

Change getCtor() to take array of types (generated from data-dojo-type, data-dojo-mixins), rather than having that logic in instantiate(), refs #14342 !strict. Extracted from Kitson's patch (CLA on file) on #14591, thanks!

[bill]

In [28341]:

Code reduction and lint fixes, refs #14342 !strict.