Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

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

14342.patch (6.7 KB) - added by cjolif 8 years ago.
draft patch
parser.html (32.4 KB) - added by cjolif 8 years ago.
updated test with "ala" ComposeJS extend method
14342-type-list.patch (2.3 KB) - added by ykami 8 years ago.
A patch to allow type list

Download all attachments as: .zip

Change History (27)

comment:1 Changed 8 years ago by 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.

comment:2 Changed 8 years ago by bill

Milestone: tbd

comment:3 Changed 8 years ago by 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.

Changed 8 years ago by cjolif

Attachment: 14342.patch added

draft patch

comment:4 Changed 8 years ago by 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.

Last edited 8 years ago by bill (previous) (diff)

Changed 8 years ago by cjolif

Attachment: parser.html added

updated test with "ala" ComposeJS extend method

comment:5 Changed 8 years ago by cjolif

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

comment:6 Changed 8 years ago by bill

Milestone: tbd1.8
Owner: changed from bill to cjolif
Status: newassigned
Type: enhancementfeature

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

comment:8 Changed 8 years ago by cjolif

Resolution: fixed
Status: assignedclosed

In [27667]:

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

comment:9 Changed 8 years ago by cjolif

In [27668]:

refs #14342. Add a missing file.

comment:10 Changed 8 years ago by 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.

comment:11 in reply to:  10 Changed 8 years ago by cjolif

Resolution: fixed
Status: closedreopened

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.

Last edited 8 years ago by cjolif (previous) (diff)

comment:12 Changed 8 years ago by 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.

comment:13 Changed 8 years ago by cjolif

Resolution: fixed
Status: reopenedclosed

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.

comment:14 in reply to:  12 Changed 8 years ago by 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.

comment:15 Changed 8 years ago by 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?

comment:16 in reply to:  15 Changed 8 years ago by 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?

comment:17 Changed 8 years ago by 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.

comment:18 Changed 8 years ago by 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"

comment:19 Changed 8 years ago by 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.

Last edited 8 years ago by cjolif (previous) (diff)

comment:20 Changed 8 years ago by 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"

Changed 8 years ago by ykami

Attachment: 14342-type-list.patch added

A patch to allow type list

comment:21 Changed 8 years ago by 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.

Last edited 8 years ago by bill (previous) (diff)

comment:22 Changed 8 years ago by 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.

comment:23 Changed 7 years ago by 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!

comment:24 Changed 7 years ago by bill

In [28341]:

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

Note: See TracTickets for help on using tickets.